-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[BUG] Mark dirty on rollback of cursor to guarantee compaction picks it up. #5265
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
Conversation
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
This pull request modifies the log service to explicitly mark a collection as dirty when a rollback of the compaction cursor occurs, ensuring that downstream compaction will correctly detect and process the collection as needing compaction. This change is a response to tests flaking due to compaction running faster, and also aligns the dirty log (DR) behavior to match compaction requirements. The change is limited to a conditional insertion of a 'dirty' marker during rollback scenarios. This summary was automatically generated by @propel-code-bot |
if allow_rollback { | ||
let mark_dirty = MarkDirty { | ||
collection_id, | ||
dirty_log: Arc::clone(&self.dirty_log), | ||
}; | ||
let _ = mark_dirty | ||
.mark_dirty(LogPosition::from_offset(adjusted_log_offset as u64), 1usize) | ||
.await; | ||
} |
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.
[BestPractice]
Consider extracting the MarkDirty
creation into a helper method or storing it as a field in the struct, since this exact pattern (creating MarkDirty
with collection_id
and Arc::clone(&self.dirty_log)
) appears multiple times throughout the codebase.
if allow_rollback { | |
let mark_dirty = MarkDirty { | |
collection_id, | |
dirty_log: Arc::clone(&self.dirty_log), | |
}; | |
let _ = mark_dirty | |
.mark_dirty(LogPosition::from_offset(adjusted_log_offset as u64), 1usize) | |
.await; | |
} | |
if allow_rollback { | |
let mark_dirty = MarkDirty { | |
collection_id, | |
dirty_log: Arc::clone(&self.dirty_log), | |
}; | |
if let Err(err) = mark_dirty | |
.mark_dirty(LogPosition::from_offset(adjusted_log_offset as u64), 1usize) | |
.await | |
{ | |
tracing::warn!("Failed to mark dirty during rollback: {}", err); | |
} | |
} |
This change adds error logging since marking dirty during rollback operations could be important for debugging if it fails.
⚡ Committable suggestion
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Description of changes
The tests flaked because compaction is now faster. This will make the
test robust, but also make it so that DR does the right thing.
Test plan
CI
pytest
for python,yarn test
for js,cargo test
for rustMigration plan
N/A
Observability plan
N/A
Documentation Changes
N/A