Do not create empty .vac files in metadata consolidation #5453
+71
−3
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
teo-tsirpanis
approved these changes
Feb 13, 2025
| @@ -80,6 +80,12 @@ Status ArrayMetaConsolidator::consolidate( | |||
| encryption_key, | |||
| key_length)); | |||
|
|
|||
| // Check if there's actually more than 1 file to consolidate | |||
| auto& metadata_r = array_for_reads.metadata(); | |||
| if (metadata_r.loaded_metadata_uris().size() <= 1) { | |||
There was a problem hiding this comment.
I had thought about using array_for_reads.array_directory().array_meta_uris() but it's the same thing.
ihnorton
pushed a commit
that referenced
this pull request
Feb 18, 2025
When we are trying to consolidate array or group metadata but there are no array or group metadata present, we are creating an empty `.vac` file on local filesystems. In object stores, notably on S3 that I have tested, our `write` method currently doesn't create a file when size is `0` (this is most probably another issue to fix), so we don't observe the same behavior. This PR unifies the behavior by enforcing that we don't create a `.vac` file if no consolidation is possible, aka if there are 0 or 1 array/group metadata present. We already do such a check for fragment metadata, fragments and commit consolidation. Both tests in this PR were failing without the fix and are now passing. --- TYPE: BUG DESC: Do not create empty .vac files in metadata consolidation (cherry picked from commit a22fe13)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When we are trying to consolidate array or group metadata but there are no array or group metadata present, we are creating an empty
.vacfile on local filesystems. In object stores, notably on S3 that I have tested, ourwritemethod currently doesn't create a file when size is0(this is most probably another issue to fix), so we don't observe the same behavior.This PR unifies the behavior by enforcing that we don't create a
.vacfile if no consolidation is possible, aka if there are 0 or 1 array/group metadata present. We already do such a check for fragment metadata, fragments and commit consolidation.Both tests in this PR were failing without the fix and are now passing.
TYPE: BUG
DESC: Do not create empty .vac files in metadata consolidation