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

Simplify mend region selection #1606

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Simplify mend region selection #1606

wants to merge 5 commits into from

Conversation

mkeeter
Copy link
Contributor

@mkeeter mkeeter commented Jan 16, 2025

I spent a while staring at find_source, convincing myself that it was just picking the highest (gen, flush, dirty) tuple. We can express it more compactly by leaning into #[derive(Ord)].

Similarly, find_dest is picking clients which are not the source client, picking all such clients if the source is dirty, and clients which differ from the source otherwise.

@mkeeter mkeeter requested review from jmpesp and leftwo January 16, 2025 16:33
@mkeeter mkeeter force-pushed the mkeeter/simplify-mend branch from 05aef15 to a4f7593 Compare January 16, 2025 17:09
@mkeeter
Copy link
Contributor Author

mkeeter commented Jan 17, 2025

I found some more cleanup:

  • Picking which extents need reconciliation was... complicated. It's now simpler (027cdc5).
  • RegionMetadata previously maintained three separate Vec<..>, and we had to check that they were the same length whenever we used them. Now, it has a single Vec<ExtentMetadata>, which is correct by construction (9ddc9a5)

Copy link
Contributor

@leftwo leftwo left a comment

Choose a reason for hiding this comment

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

Changing this makes me a little nervous as it has been working, but the tests we have should have found any issues if their were some.

/// - Highest flush number (if generation numbers are equal)
/// - Dirty bit set (if generation and flush numbers are equal)
///
/// If there is still a tie at the end, then the earliest source is chosen
Copy link
Contributor

Choose a reason for hiding this comment

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

What does earliest source mean here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Numerically lowest, I updated the comment


info!(
log,
"find dest for source {} for extent at index {}", source, i
"find dest for source {source} for extent at index {i} => {out:?}"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can update this message to indicate we have found our result. This was printing before we did the search, but now this log comes after we have found our answer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, changed to "found dest ..."

@mkeeter mkeeter requested a review from faithanalog January 17, 2025 20:17
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