laskoviymishka commented on code in PR #16796:
URL: https://github.com/apache/iceberg/pull/16796#discussion_r3567167760
##########
core/src/main/java/org/apache/iceberg/TableMetadata.java:
##########
@@ -1804,12 +1804,16 @@ private static List<MetadataLogEntry> addPreviousFile(
int maxSize =
Math.max(
- 1,
+ 0,
Review Comment:
`Math.max(0, ...)` now clamps a negative value straight to 0 — which is the
most destructive setting, since it means "keep nothing and start deleting the
superseded file." Before this change, `-1` would have landed on 1.
I'd add a `Preconditions.checkArgument(maxSize >= 0, ...)` (or at least a
`LOG.warn`) so a fat-fingered `-1` doesn't silently turn into aggressive
deletion. Other write props already validate non-negative via
`PropertyUtil.validateCommitProperties`.
##########
core/src/test/java/org/apache/iceberg/TestCatalogUtil.java:
##########
@@ -320,6 +325,76 @@ public void noFailureWhenBulkDeletingMetadataFiles() {
.doesNotThrowAnyException();
}
+ @Test
+ public void deletesSupersededMetadataFileWhenPreviousVersionsMaxIsZero() {
+ FileIO io = mock(FileIO.class);
+
+ TableMetadata.MetadataLogEntry entry1 =
+ new TableMetadata.MetadataLogEntry(
+ System.currentTimeMillis(), "s3://bucket/metadata/v1.json");
+
+ TableMetadata base = mock(TableMetadata.class);
+ TableMetadata metadata = mock(TableMetadata.class);
+
+ when(metadata.propertyAsBoolean(
+ eq(TableProperties.METADATA_DELETE_AFTER_COMMIT_ENABLED),
anyBoolean()))
+ .thenReturn(true);
+ when(base.previousFiles()).thenReturn(ImmutableList.of(entry1));
+
when(base.metadataFileLocation()).thenReturn("s3://bucket/metadata/v2.json");
+ when(metadata.previousFiles()).thenReturn(ImmutableList.of());
+
+ CatalogUtil.deleteRemovedMetadataFiles(io, base, metadata);
+
+ ArgumentCaptor<String> deleted = ArgumentCaptor.forClass(String.class);
+ verify(io, times(2)).deleteFile(deleted.capture());
+ assertThat(deleted.getAllValues())
+ .containsExactlyInAnyOrder("s3://bucket/metadata/v1.json",
"s3://bucket/metadata/v2.json");
+ }
+
+ @Test
+ public void keepsSupersededMetadataFileWhenStillTracked() {
+ FileIO io = mock(FileIO.class);
+
+ TableMetadata.MetadataLogEntry entry1 =
+ new TableMetadata.MetadataLogEntry(
+ System.currentTimeMillis(), "s3://bucket/metadata/v1.json");
+ TableMetadata.MetadataLogEntry entry2 =
+ new TableMetadata.MetadataLogEntry(
+ System.currentTimeMillis(), "s3://bucket/metadata/v2.json");
+
+ TableMetadata base = mock(TableMetadata.class);
+ TableMetadata metadata = mock(TableMetadata.class);
+
+ when(metadata.propertyAsBoolean(
+ eq(TableProperties.METADATA_DELETE_AFTER_COMMIT_ENABLED),
anyBoolean()))
+ .thenReturn(true);
+ when(base.previousFiles()).thenReturn(ImmutableList.of(entry1));
+
when(base.metadataFileLocation()).thenReturn("s3://bucket/metadata/v2.json");
Review Comment:
`base.metadataFileLocation()` is never read in this scenario —
`metadata.previousFiles()` is non-empty so the new branch doesn't fire. This
stub is dead and would trip STRICT_STUBS. I'd drop it.
##########
core/src/test/java/org/apache/iceberg/TestCatalogUtil.java:
##########
@@ -320,6 +325,76 @@ public void noFailureWhenBulkDeletingMetadataFiles() {
.doesNotThrowAnyException();
}
+ @Test
+ public void deletesSupersededMetadataFileWhenPreviousVersionsMaxIsZero() {
+ FileIO io = mock(FileIO.class);
+
+ TableMetadata.MetadataLogEntry entry1 =
+ new TableMetadata.MetadataLogEntry(
+ System.currentTimeMillis(), "s3://bucket/metadata/v1.json");
+
+ TableMetadata base = mock(TableMetadata.class);
+ TableMetadata metadata = mock(TableMetadata.class);
+
+ when(metadata.propertyAsBoolean(
+ eq(TableProperties.METADATA_DELETE_AFTER_COMMIT_ENABLED),
anyBoolean()))
+ .thenReturn(true);
+ when(base.previousFiles()).thenReturn(ImmutableList.of(entry1));
+
when(base.metadataFileLocation()).thenReturn("s3://bucket/metadata/v2.json");
+ when(metadata.previousFiles()).thenReturn(ImmutableList.of());
Review Comment:
This one passes a bit by accident — we never stub
`metadata.metadataFileLocation()`, so it returns null, and
`!Objects.equals("s3://bucket/metadata/v2.json", null)` is trivially true. The
guard that's actually load-bearing here (don't delete the current metadata
file) never gets exercised.
I'd stub `metadata.metadataFileLocation()` to a distinct location like
`v3.json` and assert it's *not* in the deleted set. And since the test drives
the behaviour by stubbing `previousFiles()` empty directly rather than setting
`previous-versions-max=0`, it's checking the code path, not the =0 contract —
setting the property (or building real metadata with it) would tie it to the
feature. wdyt?
##########
core/src/main/java/org/apache/iceberg/CatalogUtil.java:
##########
@@ -597,12 +598,18 @@ public static void deleteRemovedMetadataFiles(
// the log, thus we don't include metadata.previousFiles() for deletion
- everything else can
// be removed
removedPreviousMetadataFiles.removeAll(metadata.previousFiles());
- deleteFiles(
- io,
+
+ Set<String> metadataFilesToDelete =
removedPreviousMetadataFiles.stream()
.map(TableMetadata.MetadataLogEntry::file)
- .collect(Collectors.toSet()),
- "metadata");
+ .collect(Collectors.toCollection(Sets::newHashSet));
+ // delete base's metadata file too if log is empty
+ if (metadata.previousFiles().isEmpty()
Review Comment:
This keys the base-file deletion off `previousFiles().isEmpty()`, but what
we actually mean is "previous-versions-max is 0." Reading this branch in
isolation it's hard to tell it even belongs to the =0 feature — it's an
implicit contract with `addPreviousFile` sitting in another class.
I'd gate on the property explicitly, something like
`PropertyUtil.propertyAsInt(metadata.properties(),
METADATA_PREVIOUS_VERSIONS_MAX, DEFAULT) == 0`, so the intent is clear and
we're not relying on an empty log as a proxy that could arise some other way.
wdyt?
##########
docs/docs/maintenance.md:
##########
@@ -78,6 +78,8 @@ Untracked metadata files are also deleted as part of [orphan
file deletion](#del
| write.metadata.delete-after-commit.enabled | false | Controls
whether to delete the oldest **tracked** version metadata files after each
table commit |
| write.metadata.previous-versions-max | 100 | The max
number of previous version metadata files to track
|
+Setting `write.metadata.previous-versions-max=0` keeps no previous metadata
files, so the `metadata-log` is always empty. With
`write.metadata.delete-after-commit.enabled=true`, the superseded metadata file
is deleted on every commit, leaving only the current metadata file; lowering
the property to `0` also deletes any files still tracked in the existing
`metadata-log` on the next commit.
Review Comment:
Worth spelling out the `delete-after-commit.enabled=false` case too: with
`=0` and deletion disabled, nothing gets deleted and every superseded file just
becomes an orphan, so the log stays empty but the files pile up until an
orphan-file cleanup runs. The examples right below cover a similar shape for
`=10` but not this one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
