[ENH] Hard delete wal3 collections. - #4840
Conversation
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
|
Implement Hard Delete for wal3 Collections & Enhance Log Contention Handling This PR introduces a comprehensive 'destroy' mechanism for wal3 logs, enabling hard deletion of all WAL3-managed files in object storage tied to a collection. It modifies, extends, and thoroughly tests the delete path: recursively traversing manifest, snapshots, fragments, cursors, and GC records, ensuring all are removed and reporting failure if any remain. In addition, major changes rework the log writer's contention/retry logic to robustly handle concurrent writers and garbage collection, notably making contention failovers more predictable and recoverable. S3 delete error handling and several test suites are also updated to reflect these new semantics and to harden durability against edge cases. Key Changes: Affected Areas: This summary was automatically generated by @propel-code-bot |
| pub fn prefixed_fragment_path(prefix: &str, fragment_seq_no: FragmentSeqNo) -> String { | ||
| format!( | ||
| "{}/log/Bucket={:016x}/FragmentSeqNo={:016x}.parquet", | ||
| "{}{}/Bucket={:016x}/FragmentSeqNo={:016x}.parquet", |
There was a problem hiding this comment.
[BestPractice]
Path concatenation is missing a separator - resulting paths will look like "<prefix>log/..." instead of "<prefix>/log/...".
format!(
- "{}{}/Bucket={:016x}/FragmentSeqNo={:016x}.parquet",
- prefix,
- fragment_prefix(),
+ "{}/{}/Bucket={:016x}/FragmentSeqNo={:016x}.parquet",
+ prefix,
+ fragment_prefix(),
fragment_seq_no.bucket(),
fragment_seq_no.0,
)08b3261 to
966390c
Compare
5bff3ed to
f3f6d28
Compare
|
|
||
| for collection_id in ordered_soft_deleted_to_hard_delete_collections { | ||
| // TODO(rescrv): add wal3 hard delete here. | ||
| if let Err(err) = wal3::destroy( |
There was a problem hiding this comment.
[PerformanceOptimization]
[performance] Arc::new(self.storage.clone()) is rebuilt for every iteration even though the Storage instance is immutable. Construct the Arc once before the loop to avoid repeated allocation and reference-count churn.
| if let Err(err) = wal3::destroy( | |
| let storage = Arc::new(self.storage.clone()); | |
| for collection_id in ordered_soft_deleted_to_hard_delete_collections { | |
| if let Err(err) = wal3::destroy( | |
| Arc::clone(&storage), | |
| collection_id.storage_prefix_for_log(), | |
| ) | |
| .await | |
| { | |
| tracing::error!("could not destroy/hard delete wal3 instance: {err:?}"); | |
| // NOTE(rescrv): The alternative is to have a dead letter queue for this | |
| // collection. Once hard deleted from sysdb it'll be "impossible" to find the | |
| // garbage later, so leave the soft-deleted state to drive this state machine | |
| // to resolution, probably with operator involvement as this should never | |
| // happen and is tested. | |
| continue; | |
| } | |
| self.sysdb_client | |
| .finish_collection_deletion( | |
| self.tenant.clone().ok_or(GarbageCollectorError::InvariantViolation( | |
| "Expected tenant to be set".to_string(), | |
| ))?, | |
| self.database_name.clone().ok_or(GarbageCollectorError::InvariantViolation( | |
| "Expected database to be set".to_string(), | |
| ))?, | |
| collection_id, | |
| ) | |
| .await?; | |
| } |
⚡ 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.
966390c to
4487dfa
Compare
| // garbage later, so leave the soft-deleted state to drive this state machine | ||
| // to resolution, probably with operator involvement as this should never | ||
| // happen and is tested. | ||
| continue; |
There was a problem hiding this comment.
q: if log delete fails should we continue instead of return Err(...)? I suppose if we let it pass here then this collection entry got removed from sysdb, and the log dangles in object store
There was a problem hiding this comment.
I chose to let it hard delete the next collection in the list. Returning error stops that possibility. What would you do instead?
| async move { | ||
| storage.delete(&file).await.unwrap(); | ||
| storage | ||
| .delete(&file, DeleteOptions::default()) |
There was a problem hiding this comment.
q: do we need to think more carefully about what delete options to use here (and in other places as well)
4487dfa to
85bde07
Compare
3b98e8e to
97e142a
Compare
## Description of changes The LogContention Error could manifest if two concurrent writers or a writer and a garbage collector race. This would fail CI cluster tests. To remedy this problem, this PR reworks the retry logic to make sure that when there is contention the writers will always reset to a good place. It does this by making a pair of writers alternate. If either writer gets stuck the test fails. ## Test plan CI ## Documentation Changes N/A
## Description of changes This PR introduces code to destroy a wal3 log by deleting everything it reasonably could be known to contain. ## Test plan Two new integration tests passed. ## Documentation Changes N/A
Description of changes
This PR introduces code to destroy a wal3 log by deleting everything it reasonably could be known to
contain.
Test plan
Two new integration tests passed.
Documentation Changes
N/A