stevenzwu commented on code in PR #15003:
URL: https://github.com/apache/iceberg/pull/15003#discussion_r2755712455
##########
core/src/main/java/org/apache/iceberg/ManifestMergeManager.java:
##########
@@ -96,8 +111,13 @@ void cleanUncommitted(Set<ManifestFile> committed) {
ManifestFile merged = entry.getValue();
if (!committed.contains(merged)) {
deleteFile(merged.path());
- // remove the deleted file from the cache
- mergedManifests.remove(entry.getKey());
+ List<ManifestFile> bin = entry.getKey();
+ mergedManifests.remove(bin);
+ for (ManifestFile m : bin) {
+ if (snapshotId() != m.snapshotId()) {
Review Comment:
Is this correct in the case of multi-snapshot transaction commit?
##########
core/src/test/java/org/apache/iceberg/TestDeleteFiles.java:
##########
@@ -351,6 +351,62 @@ public void testDeleteFilesOnIndependentBranches() {
statuses(Status.EXISTING, Status.DELETED, Status.DELETED));
}
+ @TestTemplate
+ public void testDeleteFilesWithManifestSnapshotSummary() {
Review Comment:
do we still need this new test method? is any scenarios here not covered by
the existing tests?
##########
core/src/main/java/org/apache/iceberg/ManifestMergeManager.java:
##########
@@ -200,8 +220,14 @@ private ManifestFile createManifest(int specId,
List<ManifestFile> bin) {
ManifestFile manifest = writer.toManifestFile();
- // update the cache
+ // cache the merged manifest to reuse when retrying and track replaced
manifests
mergedManifests.put(bin, manifest);
+ for (ManifestFile m : bin) {
+ // only count manifests from previous snapshots; in-memory manifests are
not replaced
+ if (snapshotId() != m.snapshotId()) {
Review Comment:
same comment for multi-snapshot transaction commit
##########
core/src/test/java/org/apache/iceberg/TestCommitReporting.java:
##########
@@ -195,4 +213,68 @@ public void addAndDeleteManifests() {
assertThat(metrics.manifestsReplaced().value()).isEqualTo(2L);
assertThat(metrics.manifestEntriesProcessed().value()).isEqualTo(2L);
}
+
+ @TestTemplate
+ public void manifestMetricsForVariousOperations() {
Review Comment:
do we still need this method after adding assertions to existing test
methods?
--
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]
