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

fix(provider): move consistency check methods from NippyJarWriter to NippyJarChecker #10633

Merged
merged 12 commits into from
Sep 2, 2024

Conversation

joshieDo
Copy link
Collaborator

@joshieDo joshieDo commented Aug 30, 2024

redoes: #10628
closes #10631

NippyJarWriter was being reused for checking consistency, even in read-only modes, by using ConsistencyFailStrategy::ThrowError. However, as NippyJarWriter::new was being called...it was actually modifying/updating the file. Even in read-only mode. Which explains the race behaviour from the issue linked above.

This PR refactors the consistency verification/healing, so we stop using NippyJarWriter/StaticFileProviderRW in read-only mode:

  • introduces NippyJarChecker which decouples this logic from NippyJarWriter.
    • NippyJarChecker::check_consistency will throw error (read-only)
    • NippyJarChecker::ensure_consistency will attempt to heal (read-write)
  • removes ensure_file_consistency from StaticFileWriter/StaticFileProvider. This is now done automatically by NippyJarWriter at a low level, and StaticFileProviderRW on the SegmentHeader level.
  • adds StaticFileProvider::check_segment_consistency which is a read-only method used for example for reth db stats

Still need to test it on a node

@joshieDo joshieDo added C-bug An unexpected or incorrect behavior A-static-files Related to static files labels Aug 30, 2024
@joshieDo joshieDo changed the title fix(static-file): move consistency check methods from NippyJarWriter to NippyJarChecker fix(provider): move consistency check methods from NippyJarWriter to NippyJarChecker Aug 30, 2024
Comment on lines -1077 to -1080
StaticFileAccess::RO => {
let latest_block = self.get_highest_static_file_block(segment).unwrap_or_default();

let mut writer = StaticFileProviderRW::new(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

bye

Copy link
Collaborator

@shekhirin shekhirin left a comment

Choose a reason for hiding this comment

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

LGTM, only nits that were present before

crates/storage/nippy-jar/src/consistency.rs Outdated Show resolved Hide resolved
crates/storage/nippy-jar/src/consistency.rs Outdated Show resolved Hide resolved
crates/storage/nippy-jar/src/consistency.rs Outdated Show resolved Hide resolved
Comment on lines +62 to +89
if mode.should_err() &&
expected_offsets_file_size.cmp(&actual_offsets_file_size) != Ordering::Equal
{
return Err(NippyJarError::InconsistentState)
}

// Offsets configuration wasn't properly committed
match expected_offsets_file_size.cmp(&actual_offsets_file_size) {
Ordering::Less => {
// Happened during an appending job
// TODO: ideally we could truncate until the last offset of the last column of the
// last row inserted
self.offsets_file().get_mut().set_len(expected_offsets_file_size)?;
}
Ordering::Greater => {
// Happened during a pruning job
// `num rows = (file size - 1 - size of one offset) / num columns`
self.jar.rows = ((actual_offsets_file_size.
saturating_sub(1). // first byte is the size of one offset
saturating_sub(OFFSET_SIZE_BYTES as u64) / // expected size of the data file
(self.jar.columns as u64)) /
OFFSET_SIZE_BYTES as u64) as usize;

// Freeze row count changed
self.jar.freeze_config()?;
}
Ordering::Equal => {}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: would be nice to refactor it into something like

if expected_offsets_file_size.cmp(&actual_offsets_file_size) != Ordering::Equal {
   if mode.should_err() {
      // return err
   } else {
      // heal
   }
}

and same for the check below, makes the code easier to read

@joshieDo joshieDo added this pull request to the merge queue Sep 2, 2024
Merged via the queue into main with commit d58cf53 Sep 2, 2024
34 checks passed
@joshieDo joshieDo deleted the joshie/consistency branch September 2, 2024 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-static-files Related to static files C-bug An unexpected or incorrect behavior
Projects
None yet
2 participants