amogh-jahagirdar commented on code in PR #6335: URL: https://github.com/apache/iceberg/pull/6335#discussion_r1039115360
########## core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java: ########## @@ -948,13 +949,10 @@ private List<ManifestFile> newDeleteFilesAsManifests() { (specId, deleteFiles) -> { PartitionSpec spec = ops.current().spec(specId); try { - ManifestWriter<DeleteFile> writer = newDeleteManifestWriter(spec); - try { - writer.addAll(deleteFiles); - } finally { - writer.close(); - } - cachedNewDeleteManifests.add(writer.toManifestFile()); + List<ManifestFile> manifestFiles = Review Comment: Nit: Naming, deleteManifests? ########## core/src/main/java/org/apache/iceberg/SnapshotProducer.java: ########## @@ -499,6 +501,40 @@ protected long snapshotId() { return snapshotId; } + protected static <F extends ContentFile<F>> List<ManifestFile> writeFilesToManifests( + Iterable<F> files, + Supplier<ManifestWriter<F>> createWriter, + Long newFilesSequenceNumber, + long targetSizeBytes) + throws IOException { + List<ManifestFile> result = Lists.newArrayList(); + Iterator<F> fileIterator = files.iterator(); + ManifestWriter<F> writer = null; Review Comment: I think the writer needs to be wrapped in a try with resources to guarantee it gets closed. Also a nit which will be solved with try with resources: Could we initialize the writer upfront? ########## core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java: ########## @@ -838,9 +839,17 @@ public Object updateEvent() { } private void cleanUncommittedAppends(Set<ManifestFile> committed) { - if (cachedNewManifest != null && !committed.contains(cachedNewManifest)) { - deleteFile(cachedNewManifest.path()); - this.cachedNewManifest = null; + if (cachedNewManifests != null) { + List<ManifestFile> cleanedNewManifests = Lists.newArrayList(); + for (ManifestFile manifestFile : cachedNewManifests) { + if (!committed.contains(manifestFile)) { + deleteFile(manifestFile.path()); + } else { + cleanedNewManifests.add(manifestFile); + } + } + + this.cachedNewManifests = cleanedNewManifests; Review Comment: Sorry not sure if I follow the logic, why does cachedNewManifests need to be initialized to cleanedNewManifests? -- 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: issues-unsubscr...@iceberg.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org For additional commands, e-mail: issues-h...@iceberg.apache.org