You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "ayushtkn (via GitHub)" <gi...@apache.org> on 2026/06/30 06:37:22 UTC

[PR] Cleanup metadata files on transaction failure [polaris]

ayushtkn opened a new pull request, #4934:
URL: https://github.com/apache/polaris/pull/4934

   During `commitTransaction`, Iceberg metadata files are written before Polaris persists the corresponding entity updates. If the final metadata store update (updateEntitiesPropertiesIfNotChanged) fails (for example, due to a CAS/conflict failure), the transaction is aborted, but the newly written metadata files remain in storage. Since these metadata files are never referenced by any committed entity, they become orphaned and accumulate over time, resulting in unnecessary storage usage.
   
   ## Checklist
   - [ ] ๐Ÿ›ก๏ธ Don't disclose security issues! (contact security@apache.org)
   - [ ] ๐Ÿ”— Clearly explained why the changes are needed, or linked related issues: Fixes #
   - [ ] ๐Ÿงช Added/updated tests with good coverage, or manually tested (and explained how)
   - [ ] ๐Ÿ’ก Added comments for complex logic
   - [ ] ๐Ÿงพ Updated `CHANGELOG.md` (if needed)
   - [ ] ๐Ÿ“š Updated documentation in `site/content/in-dev/unreleased` (if needed)
   


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

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


Re: [PR] Cleanup metadata files on transaction failure [polaris]

Posted by "nandorKollar (via GitHub)" <gi...@apache.org>.
nandorKollar commented on code in PR #4934:
URL: https://github.com/apache/polaris/pull/4934#discussion_r3497467385


##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -1272,19 +1275,35 @@ public void commitTransaction(CommitTransactionRequest commitTransactionRequest)
           // Commit all accumulated changes for this table in a single atomic operation
           if (!currentMetadata.changes().isEmpty()) {
             tableOps.commit(baseMetadata, currentMetadata);
+            tableFileIOs.put(tableIdentifier, tableOps.io());
           }
 
           tableMetadataObjs.add(currentMetadata);
         });
 
-    // Commit the collected updates in a single atomic operation
+    // Extract newly written metadata locations from the buffered entity updates.
+    // We cannot use tableOps.current().metadataFileLocation() because requestRefresh()
+    // causes doRefresh() to read from the store where the entity hasn't been persisted yet.
+    // The pendingUpdates entities have the correct new location set by doCommit().
     List<EntityWithPath> pendingUpdates = transactionMetaStoreManager.getPendingUpdates();
+    List<Map.Entry<FileIO, String>> writtenMetadataFiles =
+        pendingUpdates.stream()
+            .map(ewp -> IcebergTableLikeEntity.of(ewp.entity()))
+            .filter(entity -> entity != null && entity.getMetadataLocation() != null)
+            .filter(entity -> tableFileIOs.containsKey(entity.getTableIdentifier()))
+            .map(
+                entity ->
+                    Map.entry(
+                        tableFileIOs.get(entity.getTableIdentifier()),
+                        entity.getMetadataLocation()))
+            .toList();
+
     EntitiesResult result =
         metaStoreManager()
             .updateEntitiesPropertiesIfNotChanged(
                 callContext().getPolarisCallContext(), pendingUpdates);
     if (!result.isSuccess()) {
-      // TODO: Retries and server-side cleanup on failure, review possible exceptions

Review Comment:
   Retry is not yet implemented, I think we should leave that TODO here.



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

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


Re: [PR] Cleanup metadata files on transaction failure [polaris]

Posted by "ayushtkn (via GitHub)" <gi...@apache.org>.
ayushtkn commented on PR #4934:
URL: https://github.com/apache/polaris/pull/4934#issuecomment-4879447733

   Thanx @nandorKollar & @adutra for the reviews!!! can this be merged?


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

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


Re: [PR] Cleanup metadata files on transaction failure [polaris]

Posted by "ayushtkn (via GitHub)" <gi...@apache.org>.
ayushtkn commented on code in PR #4934:
URL: https://github.com/apache/polaris/pull/4934#discussion_r3498367010


##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -1272,19 +1275,35 @@ public void commitTransaction(CommitTransactionRequest commitTransactionRequest)
           // Commit all accumulated changes for this table in a single atomic operation
           if (!currentMetadata.changes().isEmpty()) {
             tableOps.commit(baseMetadata, currentMetadata);
+            tableFileIOs.put(tableIdentifier, tableOps.io());
           }
 
           tableMetadataObjs.add(currentMetadata);
         });
 
-    // Commit the collected updates in a single atomic operation
+    // Extract newly written metadata locations from the buffered entity updates.
+    // We cannot use tableOps.current().metadataFileLocation() because requestRefresh()
+    // causes doRefresh() to read from the store where the entity hasn't been persisted yet.
+    // The pendingUpdates entities have the correct new location set by doCommit().
     List<EntityWithPath> pendingUpdates = transactionMetaStoreManager.getPendingUpdates();
+    List<Map.Entry<FileIO, String>> writtenMetadataFiles =

Review Comment:
   Sorry, I misunderstood initially, I have moved it under isSuccess, can you check if that is exactly what you asked for.



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

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


Re: [PR] Cleanup metadata files on transaction failure [polaris]

Posted by "nandorKollar (via GitHub)" <gi...@apache.org>.
nandorKollar commented on code in PR #4934:
URL: https://github.com/apache/polaris/pull/4934#discussion_r3497984240


##########
runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/CommitTransactionEventTest.java:
##########
@@ -273,4 +280,79 @@ private CommitTransactionRequest generateCommitTransactionRequest(
                 updateRequirements,
                 List.of(new MetadataUpdate.SetProperties(Map.of(propertyName, "value2"))))));
   }
+
+  @Test
+  void testCommitTransactionCleansUpMetadataOnFailure(@TempDir Path tempDir) {
+    String location = tempDir.toAbsolutePath().toUri().toString();
+    if (location.endsWith("/")) {
+      location = location.substring(0, location.length() - 1);
+    }
+
+    // Create TestServices with a spy that will fail on updateEntitiesPropertiesIfNotChanged
+    // but only AFTER initial setup (table creation) succeeds.
+    AtomicBoolean shouldFail = new AtomicBoolean(false);
+    TestServices testServices =
+        TestServices.builder()
+            .config(
+                Map.of(
+                    "ALLOW_INSECURE_STORAGE_TYPES",
+                    "true",
+                    "SUPPORTED_CATALOG_STORAGE_TYPES",
+                    List.of("FILE")))
+            .metaStoreManagerDecorator(
+                msm -> {
+                  org.apache.polaris.core.persistence.PolarisMetaStoreManager spy =
+                      Mockito.spy(msm);
+                  Mockito.doAnswer(
+                          invocation -> {
+                            if (shouldFail.get()) {
+                              return new EntitiesResult(
+                                  BaseResult.ReturnStatus.ENTITY_CANNOT_BE_RESOLVED,
+                                  "simulated CAS failure");
+                            }
+                            return invocation.callRealMethod();
+                          })
+                      .when(spy)
+                      .updateEntitiesPropertiesIfNotChanged(Mockito.any(), Mockito.any());
+                  return spy;
+                })
+            .build();
+
+    createCatalogAndNamespace(testServices, Map.of(), location);
+
+    String table1Name = "cleanup-table-1";
+    String table2Name = "cleanup-table-2";
+    createTable(testServices, table1Name, location);
+    createTable(testServices, table2Name, location);
+
+    // Count metadata files before the failing transaction
+    long metadataFilesBefore = countMetadataFiles(tempDir);
+
+    // Now enable the CAS failure and attempt a commitTransaction
+    shouldFail.set(true);
+    assertThatThrownBy(
+            () ->
+                testServices
+                    .restApi()
+                    .commitTransaction(
+                        catalog,
+                        generateCommitTransactionRequest(false, table1Name, table2Name),
+                        IDEMPOTENCY_KEY,
+                        testServices.realmContext(),
+                        testServices.securityContext()))
+        .isInstanceOf(CommitFailedException.class)
+        .hasMessageContaining("Transaction commit failed");
+
+    // After the failed transaction, no new metadata files should remain (they were cleaned up)
+    long metadataFilesAfter = countMetadataFiles(tempDir);
+    assertThat(metadataFilesAfter).isEqualTo(metadataFilesBefore);

Review Comment:
   nit: Probably matching the exact set of paths before and after is more accurate.



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

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


Re: [PR] Cleanup metadata files on transaction failure [polaris]

Posted by "ayushtkn (via GitHub)" <gi...@apache.org>.
ayushtkn commented on code in PR #4934:
URL: https://github.com/apache/polaris/pull/4934#discussion_r3498023941


##########
runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/CommitTransactionEventTest.java:
##########
@@ -273,4 +280,79 @@ private CommitTransactionRequest generateCommitTransactionRequest(
                 updateRequirements,
                 List.of(new MetadataUpdate.SetProperties(Map.of(propertyName, "value2"))))));
   }
+
+  @Test
+  void testCommitTransactionCleansUpMetadataOnFailure(@TempDir Path tempDir) {
+    String location = tempDir.toAbsolutePath().toUri().toString();
+    if (location.endsWith("/")) {
+      location = location.substring(0, location.length() - 1);
+    }
+
+    // Create TestServices with a spy that will fail on updateEntitiesPropertiesIfNotChanged
+    // but only AFTER initial setup (table creation) succeeds.
+    AtomicBoolean shouldFail = new AtomicBoolean(false);
+    TestServices testServices =
+        TestServices.builder()
+            .config(
+                Map.of(
+                    "ALLOW_INSECURE_STORAGE_TYPES",
+                    "true",
+                    "SUPPORTED_CATALOG_STORAGE_TYPES",
+                    List.of("FILE")))
+            .metaStoreManagerDecorator(
+                msm -> {
+                  org.apache.polaris.core.persistence.PolarisMetaStoreManager spy =
+                      Mockito.spy(msm);
+                  Mockito.doAnswer(
+                          invocation -> {
+                            if (shouldFail.get()) {
+                              return new EntitiesResult(
+                                  BaseResult.ReturnStatus.ENTITY_CANNOT_BE_RESOLVED,
+                                  "simulated CAS failure");
+                            }
+                            return invocation.callRealMethod();
+                          })
+                      .when(spy)
+                      .updateEntitiesPropertiesIfNotChanged(Mockito.any(), Mockito.any());
+                  return spy;
+                })
+            .build();
+
+    createCatalogAndNamespace(testServices, Map.of(), location);
+
+    String table1Name = "cleanup-table-1";
+    String table2Name = "cleanup-table-2";
+    createTable(testServices, table1Name, location);
+    createTable(testServices, table2Name, location);
+
+    // Count metadata files before the failing transaction
+    long metadataFilesBefore = countMetadataFiles(tempDir);
+
+    // Now enable the CAS failure and attempt a commitTransaction
+    shouldFail.set(true);
+    assertThatThrownBy(
+            () ->
+                testServices
+                    .restApi()
+                    .commitTransaction(
+                        catalog,
+                        generateCommitTransactionRequest(false, table1Name, table2Name),
+                        IDEMPOTENCY_KEY,
+                        testServices.realmContext(),
+                        testServices.securityContext()))
+        .isInstanceOf(CommitFailedException.class)
+        .hasMessageContaining("Transaction commit failed");
+
+    // After the failed transaction, no new metadata files should remain (they were cleaned up)
+    long metadataFilesAfter = countMetadataFiles(tempDir);
+    assertThat(metadataFilesAfter).isEqualTo(metadataFilesBefore);

Review Comment:
   Changed to validate the files



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

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


Re: [PR] Cleanup metadata files on transaction failure [polaris]

Posted by "nandorKollar (via GitHub)" <gi...@apache.org>.
nandorKollar commented on code in PR #4934:
URL: https://github.com/apache/polaris/pull/4934#discussion_r3498515044


##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -1272,19 +1275,35 @@ public void commitTransaction(CommitTransactionRequest commitTransactionRequest)
           // Commit all accumulated changes for this table in a single atomic operation
           if (!currentMetadata.changes().isEmpty()) {
             tableOps.commit(baseMetadata, currentMetadata);
+            tableFileIOs.put(tableIdentifier, tableOps.io());
           }
 
           tableMetadataObjs.add(currentMetadata);
         });
 
-    // Commit the collected updates in a single atomic operation
+    // Extract newly written metadata locations from the buffered entity updates.
+    // We cannot use tableOps.current().metadataFileLocation() because requestRefresh()
+    // causes doRefresh() to read from the store where the entity hasn't been persisted yet.
+    // The pendingUpdates entities have the correct new location set by doCommit().
     List<EntityWithPath> pendingUpdates = transactionMetaStoreManager.getPendingUpdates();
+    List<Map.Entry<FileIO, String>> writtenMetadataFiles =

Review Comment:
   Thanks, it is yes! Small optimization, but we don't need to collect list when the commit succeeds.



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

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


Re: [PR] Cleanup metadata files on transaction failure [polaris]

Posted by "nandorKollar (via GitHub)" <gi...@apache.org>.
nandorKollar merged PR #4934:
URL: https://github.com/apache/polaris/pull/4934


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

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


Re: [PR] Cleanup metadata files on transaction failure [polaris]

Posted by "ayushtkn (via GitHub)" <gi...@apache.org>.
ayushtkn commented on code in PR #4934:
URL: https://github.com/apache/polaris/pull/4934#discussion_r3497986380


##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -1272,19 +1275,35 @@ public void commitTransaction(CommitTransactionRequest commitTransactionRequest)
           // Commit all accumulated changes for this table in a single atomic operation
           if (!currentMetadata.changes().isEmpty()) {
             tableOps.commit(baseMetadata, currentMetadata);
+            tableFileIOs.put(tableIdentifier, tableOps.io());
           }
 
           tableMetadataObjs.add(currentMetadata);
         });
 
-    // Commit the collected updates in a single atomic operation
+    // Extract newly written metadata locations from the buffered entity updates.
+    // We cannot use tableOps.current().metadataFileLocation() because requestRefresh()
+    // causes doRefresh() to read from the store where the entity hasn't been persisted yet.
+    // The pendingUpdates entities have the correct new location set by doCommit().
     List<EntityWithPath> pendingUpdates = transactionMetaStoreManager.getPendingUpdates();
+    List<Map.Entry<FileIO, String>> writtenMetadataFiles =

Review Comment:
   The list needs to be populated during the transaction loop (before we know the outcome), because that's when `tableOps.commit()` writes the metadata files. We can't discover which files were written after the fact. The list is only consumed inside the failure block, but it must be built before the `commitTransactionBatch` call.  



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

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


Re: [PR] Cleanup metadata files on transaction failure [polaris]

Posted by "nandorKollar (via GitHub)" <gi...@apache.org>.
nandorKollar commented on code in PR #4934:
URL: https://github.com/apache/polaris/pull/4934#discussion_r3498307109


##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -1272,19 +1275,35 @@ public void commitTransaction(CommitTransactionRequest commitTransactionRequest)
           // Commit all accumulated changes for this table in a single atomic operation
           if (!currentMetadata.changes().isEmpty()) {
             tableOps.commit(baseMetadata, currentMetadata);
+            tableFileIOs.put(tableIdentifier, tableOps.io());
           }
 
           tableMetadataObjs.add(currentMetadata);
         });
 
-    // Commit the collected updates in a single atomic operation
+    // Extract newly written metadata locations from the buffered entity updates.
+    // We cannot use tableOps.current().metadataFileLocation() because requestRefresh()
+    // causes doRefresh() to read from the store where the entity hasn't been persisted yet.
+    // The pendingUpdates entities have the correct new location set by doCommit().
     List<EntityWithPath> pendingUpdates = transactionMetaStoreManager.getPendingUpdates();
+    List<Map.Entry<FileIO, String>> writtenMetadataFiles =

Review Comment:
   I'm not sure that I completely understood your point. `pendingUpdates` is an immutable list, and the subsequent `updateEntitiesPropertiesIfNotChanged` doesn't modify the content of tableFileIOs or pendingUpdates does it?



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

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


Re: [PR] Cleanup metadata files on transaction failure [polaris]

Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #4934:
URL: https://github.com/apache/polaris/pull/4934#discussion_r3498062295


##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -1272,19 +1275,36 @@ public void commitTransaction(CommitTransactionRequest commitTransactionRequest)
           // Commit all accumulated changes for this table in a single atomic operation
           if (!currentMetadata.changes().isEmpty()) {
             tableOps.commit(baseMetadata, currentMetadata);
+            tableFileIOs.put(tableIdentifier, tableOps.io());
           }
 
           tableMetadataObjs.add(currentMetadata);
         });
 
-    // Commit the collected updates in a single atomic operation
+    // Extract newly written metadata locations from the buffered entity updates.
+    // We cannot use tableOps.current().metadataFileLocation() because requestRefresh()
+    // causes doRefresh() to read from the store where the entity hasn't been persisted yet.
+    // The pendingUpdates entities have the correct new location set by doCommit().
     List<EntityWithPath> pendingUpdates = transactionMetaStoreManager.getPendingUpdates();
+    List<Map.Entry<FileIO, String>> writtenMetadataFiles =

Review Comment:
   `List<Map.Entry<>>` is a bit awkward, maybe introduce a record? E.g.
   
   ```java
   record FileToDelete(FileIO io, String location) {
     void cleanup() {
       try {
         io.deleteFile(location);
       } catch (Exception e) {
         LOGGER.warn("Failed to clean up metadata file {} after transaction failure", location, e);
       }
     }
   }
   ```



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

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


Re: [PR] Cleanup metadata files on transaction failure [polaris]

Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #4934:
URL: https://github.com/apache/polaris/pull/4934#discussion_r3496684314


##########
runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/CommitTransactionEventTest.java:
##########
@@ -273,4 +278,81 @@ private CommitTransactionRequest generateCommitTransactionRequest(
                 updateRequirements,
                 List.of(new MetadataUpdate.SetProperties(Map.of(propertyName, "value2"))))));
   }
+
+  @Test
+  void testCommitTransactionCleansUpMetadataOnFailure(@TempDir Path tempDir) {
+    String location = tempDir.toAbsolutePath().toUri().toString();
+    if (location.endsWith("/")) {
+      location = location.substring(0, location.length() - 1);
+    }
+
+    // Create TestServices with a spy that will fail on updateEntitiesPropertiesIfNotChanged
+    // but only AFTER initial setup (table creation) succeeds.
+    AtomicBoolean shouldFail = new AtomicBoolean(false);
+    TestServices testServices =
+        TestServices.builder()
+            .config(
+                Map.of(
+                    "ALLOW_INSECURE_STORAGE_TYPES",
+                    "true",
+                    "SUPPORTED_CATALOG_STORAGE_TYPES",
+                    List.of("FILE")))
+            .metaStoreManagerDecorator(
+                msm -> {
+                  org.apache.polaris.core.persistence.PolarisMetaStoreManager spy =
+                      Mockito.spy(msm);
+                  Mockito.doAnswer(
+                          invocation -> {
+                            if (shouldFail.get()) {
+                              return new org.apache.polaris.core.persistence.dao.entity
+                                  .EntitiesResult(
+                                  org.apache.polaris.core.persistence.dao.entity.BaseResult
+                                      .ReturnStatus.ENTITY_CANNOT_BE_RESOLVED,
+                                  "simulated CAS failure");

Review Comment:
   The `EntitiesResult` instantiation is split across tokens (`...dao.entity` on one line and `.EntitiesResult` on the next), which makes the code hard to read and is easy to mis-edit. Please reformat this into a normal constructor call (ideally also via imports, but at least keep the type name contiguous).



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

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


Re: [PR] Cleanup metadata files on transaction failure [polaris]

Posted by "nandorKollar (via GitHub)" <gi...@apache.org>.
nandorKollar commented on code in PR #4934:
URL: https://github.com/apache/polaris/pull/4934#discussion_r3497475924


##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -1272,19 +1275,35 @@ public void commitTransaction(CommitTransactionRequest commitTransactionRequest)
           // Commit all accumulated changes for this table in a single atomic operation
           if (!currentMetadata.changes().isEmpty()) {
             tableOps.commit(baseMetadata, currentMetadata);
+            tableFileIOs.put(tableIdentifier, tableOps.io());
           }
 
           tableMetadataObjs.add(currentMetadata);
         });
 
-    // Commit the collected updates in a single atomic operation
+    // Extract newly written metadata locations from the buffered entity updates.
+    // We cannot use tableOps.current().metadataFileLocation() because requestRefresh()
+    // causes doRefresh() to read from the store where the entity hasn't been persisted yet.
+    // The pendingUpdates entities have the correct new location set by doCommit().
     List<EntityWithPath> pendingUpdates = transactionMetaStoreManager.getPendingUpdates();
+    List<Map.Entry<FileIO, String>> writtenMetadataFiles =

Review Comment:
   This list of files is used only when the commit failed, can we move it inside `if (!result.isSuccess())`?



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

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