-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Conversation
NippyJarWriter
to NippyJarChecker
NippyJarWriter
to NippyJarChecker
StaticFileAccess::RO => { | ||
let latest_block = self.get_highest_static_file_block(segment).unwrap_or_default(); | ||
|
||
let mut writer = StaticFileProviderRW::new( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bye
There was a problem hiding this 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
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 => {} | ||
} |
There was a problem hiding this comment.
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
redoes: #10628
closes #10631
NippyJarWriter
was being reused for checking consistency, even in read-only modes, by usingConsistencyFailStrategy::ThrowError
. However, asNippyJarWriter::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:NippyJarChecker
which decouples this logic fromNippyJarWriter
.NippyJarChecker::check_consistency
will throw error (read-only)NippyJarChecker::ensure_consistency
will attempt to heal (read-write)ensure_file_consistency
fromStaticFileWriter/StaticFileProvider
. This is now done automatically byNippyJarWriter
at a low level, andStaticFileProviderRW
on theSegmentHeader
level.StaticFileProvider::check_segment_consistency
which is a read-only method used for example forreth db stats
Still need to test it on a node