laskoviymishka commented on code in PR #16796:
URL: https://github.com/apache/iceberg/pull/16796#discussion_r3598767644
##########
core/src/main/java/org/apache/iceberg/CatalogUtil.java:
##########
@@ -597,12 +598,19 @@ 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));
+ // An empty metadata log (e.g. METADATA_PREVIOUS_VERSIONS_MAX=0) no
longer retains base's
+ // own metadata file, so delete it too.
+ if (metadata.previousFiles().isEmpty()
Review Comment:
Still open from last round — this branch fires on
`previousFiles().isEmpty()` rather than on `previous-versions-max` actually
being 0. That's an implicit contract between two distant methods: nothing here
documents why an empty log is a safe proxy for max=0, and any future path that
legitimately empties `previousFiles()` would start deleting
`base.metadataFileLocation()` for every such table with delete-after-commit on
— which is permanent.
I'd read the property directly here (`maxPrev == 0`) so the branch is
self-evidently tied to this feature.
##########
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:
Also still open — the `Math.max(0, ...)` change means a fat-fingered `-1`
now clamps to 0, the most destructive setting, so it starts deleting the
superseded file every commit instead of keeping one.
I'd reject a negative value at write time (a
`Preconditions.checkArgument(maxSize >= 0, ...)`, or validation in
`UpdateProperties.commit()`) rather than silently clamp it to zero-retention. A
negative value should fail loudly, not quietly become the most aggressive
setting.
##########
core/src/main/java/org/apache/iceberg/CatalogUtil.java:
##########
@@ -597,12 +598,19 @@ 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));
+ // An empty metadata log (e.g. METADATA_PREVIOUS_VERSIONS_MAX=0) no
longer retains base's
+ // own metadata file, so delete it too.
+ if (metadata.previousFiles().isEmpty()
+ && !Objects.equals(base.metadataFileLocation(),
metadata.metadataFileLocation())) {
+ metadataFilesToDelete.add(base.metadataFileLocation());
Review Comment:
New one I hit while re-reading this block: `base.metadataFileLocation()` can
be null for uncommitted in-memory metadata, and since this is a public static
method the guard above doesn't rule that out. If base's location is null and
`metadata`'s is non-null, `Objects.equals(null, nonNull)` is false, the branch
fires, and we add `null` to the delete set → `io.deleteFile(null)`.
It's safe in the current commit path since base is always loaded from a
location, but I'd add `base.metadataFileLocation() != null` to the condition to
keep it robust for other callers.
--
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]
