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

["szehon-ho (via GitHub)" <gi...@apache.org>]

szehon-ho commented on code in PR #6335:
URL: https://github.com/apache/iceberg/pull/6335#discussion_r1248320316


##########
core/src/main/java/org/apache/iceberg/FastAppend.java:
##########
@@ -178,8 +184,25 @@ public Object updateEvent() {
 
   @Override
   protected void cleanUncommitted(Set<ManifestFile> committed) {
-    if (newManifest != null && !committed.contains(newManifest)) {
-      deleteFile(newManifest.path());
+    if (newManifests != null) {
+      // Delete those manifests are not contained in the committed set and keep others as here.

Review Comment:
   'keep others as here' is a bit ambigious, how about?
   'Delete newManifests that have not been committed and clear them from the list'



##########
core/src/main/java/org/apache/iceberg/FastAppend.java:
##########
@@ -178,8 +184,25 @@ public Object updateEvent() {
 
   @Override
   protected void cleanUncommitted(Set<ManifestFile> committed) {
-    if (newManifest != null && !committed.contains(newManifest)) {
-      deleteFile(newManifest.path());
+    if (newManifests != null) {
+      // Delete those manifests are not contained in the committed set and keep others as here.
+      // This is needed for manifests cleanup especially in transaction mode, for example:
+      //   Transaction txn = beginTransaction(...)
+      //   // success operation and call this cleanUncommittedAppends

Review Comment:
    'operation succeeds and calls cleanUncommitted'?



##########
core/src/main/java/org/apache/iceberg/FastAppend.java:
##########
@@ -178,8 +184,25 @@ public Object updateEvent() {
 
   @Override
   protected void cleanUncommitted(Set<ManifestFile> committed) {
-    if (newManifest != null && !committed.contains(newManifest)) {
-      deleteFile(newManifest.path());
+    if (newManifests != null) {
+      // Delete those manifests are not contained in the committed set and keep others as here.
+      // This is needed for manifests cleanup especially in transaction mode, for example:
+      //   Transaction txn = beginTransaction(...)
+      //   // success operation and call this cleanUncommittedAppends
+      //   txn.newFastAppend().appendFile(...).commit();
+      //   some other operations ...

Review Comment:
   some other operations need an additional // before for consistency?



##########
core/src/main/java/org/apache/iceberg/FastAppend.java:
##########
@@ -178,8 +184,25 @@ public Object updateEvent() {
 
   @Override
   protected void cleanUncommitted(Set<ManifestFile> committed) {
-    if (newManifest != null && !committed.contains(newManifest)) {
-      deleteFile(newManifest.path());
+    if (newManifests != null) {
+      // Delete those manifests are not contained in the committed set and keep others as here.
+      // This is needed for manifests cleanup especially in transaction mode, for example:
+      //   Transaction txn = beginTransaction(...)
+      //   // success operation and call this cleanUncommittedAppends
+      //   txn.newFastAppend().appendFile(...).commit();
+      //   some other operations ...
+      //   // commit failed need to clean up those successes committed manifests

Review Comment:
   'Commit fails and needs to clean up newManifests' ?  (they are not committed , right?)



##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -513,6 +515,38 @@ protected long snapshotId() {
     return snapshotId;
   }
 
+  protected static <F extends ContentFile<F>> List<ManifestFile> writeFilesToManifests(

Review Comment:
   I feel like , its easier to read and fits more Iceberg pattern if the logic is inside ManifestWriter (similar to RollingFileWriter), is that possible?  Can also eliminate the double method for data/delete file.



-- 
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