fix: cleanup external staging manifests #2792
Conversation
wjones127
commented
Aug 26, 2024
wjones127
commented
- Fixes Temp manifest files are not cleaned up after commit using external manifest store #2059
- Fixes add logic to clean up stray staged manifests #1201
wjones127
force-pushed
the
fix/cleanup-external-manifests
branch
from
1753b3f to
7a69608
Compare
wjones127
force-pushed
the
fix/cleanup-external-manifests
branch
from
7a69608 to
b4026a0
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2792 +/- ##
==========================================
+ Coverage 78.52% 78.53% +0.01%
==========================================
Files 228 228
Lines 68641 68677 +36
Branches 68641 68677 +36
==========================================
+ Hits 53902 53938 +36
- Misses 11679 11683 +4
+ Partials 3060 3056 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| match store | ||
| .copy(staging_manifest_path, &final_manifest_path) | ||
| .await | ||
| { | ||
| Ok(_) => {} | ||
| Err(ObjectStoreError::NotFound { .. }) => return Ok(final_manifest_path), // Another writer beat us to it. | ||
| Err(e) => return Err(e.into()), | ||
| }; | ||
|
|
||
| // step 2: flip the external store to point to the final location | ||
| self.external_manifest_store | ||
| .put_if_exists(base_path.as_ref(), version, final_manifest_path.as_ref()) | ||
| .await?; |
There was a problem hiding this comment.
It looks like you have store.copy and then external_manifest_store.put_if_exists.
However, in the original code blocks it looks like it was store.copy and then store.rename and then external_manifest_store.put_if_exists. What happened to the rename?
There was a problem hiding this comment.
I'm assuming that I could do this given the TODO:
// TODO: remove copy-rename once we upgrade object_store crate
This is likely from:
