Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

storcon: use the SchedulingPolicy enum in SafekeeperPersistence #10897

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

arpad-m
Copy link
Member

@arpad-m arpad-m commented Feb 19, 2025

We don't want to serialize to/from string all the time, so use SchedulingPolicy in SafekeeperPersistence via the use of a wrapper.

Stacked atop #10891

@arpad-m arpad-m requested a review from arssher February 19, 2025 16:23
Base automatically changed from arpad/sk_initial_heartbeat_timeout to main February 19, 2025 17:03
Copy link

github-actions bot commented Feb 19, 2025

7557 tests run: 7184 passed, 0 failed, 373 skipped (full report)


Code coverage* (full report)

  • functions: 32.9% (8621 of 26205 functions)
  • lines: 48.8% (72725 of 148913 lines)

* collected from Rust tests only


The comment gets automatically updated with the latest test results
a1f4091 at 2025-02-20T16:53:56.751Z :recycle:

Copy link
Contributor

@arssher arssher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we implement FromSql directly on SkSchedulingPolicy to avoid messing with wrapper type?

Also please rebase/merge so for easier review, it seems to have code from prev PR.

@arpad-m
Copy link
Member Author

arpad-m commented Feb 20, 2025

Both FromSql and SkSchedulingPolicy come from third party crates, so we can't implement it in our crate. I don't think that adding a "heavy" dependency like diesel to safekeeper_api is a good idea, so that's why I added a wrapper type.

@arpad-m arpad-m force-pushed the arpad/scheduling_policy_enum branch from 7d4c308 to a1f4091 Compare February 20, 2025 15:54
@arpad-m arpad-m requested a review from a team as a code owner February 20, 2025 15:54
@arpad-m arpad-m requested a review from VladLazar February 20, 2025 15:54
@arpad-m
Copy link
Member Author

arpad-m commented Feb 20, 2025

I've added a doc comment to explain the reason why we have a wrapper.

@arpad-m arpad-m requested a review from arssher February 20, 2025 16:03
@arssher
Copy link
Contributor

arssher commented Feb 21, 2025

Hang on, why SkSchedulingPolicy is even in pageserver_api crate? Let's just move it straightly to storcon. Safekeepers themselves don't need to be aware how they are scheduled, even more pageservers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants