[GitHub] [iceberg] amogh-jahagirdar commented on a diff in pull request #6335: Core: Avoid generating a large ManifestFile when committing

[GitBox <gi...@apache.org>]

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-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org