Remove unnecessary file cleanup in Iceberg

The table builder code should not throw exceptions.
trinodb · Feb 18, 2022 · b12b793 · b12b793
1 parent 6170eec
commit b12b793
Show file tree

Hide file tree

Showing 2 changed files with 29 additions and 53 deletions.
diff --git a/...eberg/src/main/java/io/trino/plugin/iceberg/catalog/AbstractMetastoreTableOperations.java b/...eberg/src/main/java/io/trino/plugin/iceberg/catalog/AbstractMetastoreTableOperations.java
@@ -189,34 +189,22 @@ protected void commitNewTable(TableMetadata metadata)
     {
         String newMetadataLocation = writeNewMetadata(metadata, version + 1);
 
-        Table table;
-        try {
-            Table.Builder builder = Table.builder()
-                    .setDatabaseName(database)
-                    .setTableName(tableName)
-                    .setOwner(owner)
-                    .setTableType(TableType.EXTERNAL_TABLE.name())
-                    .setDataColumns(toHiveColumns(metadata.schema().columns()))
-                    .withStorage(storage -> storage.setLocation(metadata.location()))
-                    .withStorage(storage -> storage.setStorageFormat(STORAGE_FORMAT))
-                    .setParameter("EXTERNAL", "TRUE")
-                    .setParameter(TABLE_TYPE_PROP, ICEBERG_TABLE_TYPE_VALUE)
-                    .setParameter(METADATA_LOCATION_PROP, newMetadataLocation);
-            String tableComment = metadata.properties().get(TABLE_COMMENT);
-            if (tableComment != null) {
-                builder.setParameter(TABLE_COMMENT, tableComment);
-            }
-            table = builder.build();
-        }
-        catch (RuntimeException e) {
-            try {
-                io().deleteFile(newMetadataLocation);
-            }
-            catch (RuntimeException ex) {
-                e.addSuppressed(ex);
-            }
-            throw e;
+        Table.Builder builder = Table.builder()
+                .setDatabaseName(database)
+                .setTableName(tableName)
+                .setOwner(owner)
+                .setTableType(TableType.EXTERNAL_TABLE.name())
+                .setDataColumns(toHiveColumns(metadata.schema().columns()))
+                .withStorage(storage -> storage.setLocation(metadata.location()))
+                .withStorage(storage -> storage.setStorageFormat(STORAGE_FORMAT))
+                .setParameter("EXTERNAL", "TRUE")
+                .setParameter(TABLE_TYPE_PROP, ICEBERG_TABLE_TYPE_VALUE)
+                .setParameter(METADATA_LOCATION_PROP, newMetadataLocation);
+        String tableComment = metadata.properties().get(TABLE_COMMENT);
+        if (tableComment != null) {
+            builder.setParameter(TABLE_COMMENT, tableComment);
         }
+        Table table = builder.build();
 
         PrincipalPrivileges privileges = owner.map(MetastoreUtil::buildInitialPrivilegeSet).orElse(NO_PRIVILEGES);
         metastore.createTable(table, privileges);

diff --git a/...berg/src/main/java/io/trino/plugin/iceberg/catalog/file/FileMetastoreTableOperations.java b/...berg/src/main/java/io/trino/plugin/iceberg/catalog/file/FileMetastoreTableOperations.java
@@ -51,35 +51,23 @@ public FileMetastoreTableOperations(
     @Override
     protected void commitToExistingTable(TableMetadata base, TableMetadata metadata)
     {
-        String newMetadataLocation = writeNewMetadata(metadata, version + 1);
+        Table currentTable = getTable();
 
-        Table table;
-        try {
-            Table currentTable = getTable();
+        checkState(currentMetadataLocation != null, "No current metadata location for existing table");
+        String metadataLocation = currentTable.getParameters().get(METADATA_LOCATION_PROP);
+        if (!currentMetadataLocation.equals(metadataLocation)) {
+            throw new CommitFailedException("Metadata location [%s] is not same as table metadata location [%s] for %s",
+                    currentMetadataLocation, metadataLocation, getSchemaTableName());
+        }
 
-            checkState(currentMetadataLocation != null, "No current metadata location for existing table");
-            String metadataLocation = currentTable.getParameters().get(METADATA_LOCATION_PROP);
-            if (!currentMetadataLocation.equals(metadataLocation)) {
-                throw new CommitFailedException("Metadata location [%s] is not same as table metadata location [%s] for %s",
-                        currentMetadataLocation, metadataLocation, getSchemaTableName());
-            }
+        String newMetadataLocation = writeNewMetadata(metadata, version + 1);
 
-            table = Table.builder(currentTable)
-                    .setDataColumns(toHiveColumns(metadata.schema().columns()))
-                    .withStorage(storage -> storage.setLocation(metadata.location()))
-                    .setParameter(METADATA_LOCATION_PROP, newMetadataLocation)
-                    .setParameter(PREVIOUS_METADATA_LOCATION_PROP, currentMetadataLocation)
-                    .build();
-        }
-        catch (RuntimeException e) {
-            try {
-                io().deleteFile(newMetadataLocation);
-            }
-            catch (RuntimeException ex) {
-                e.addSuppressed(ex);
-            }
-            throw e;
-        }
+        Table table = Table.builder(currentTable)
+                .setDataColumns(toHiveColumns(metadata.schema().columns()))
+                .withStorage(storage -> storage.setLocation(metadata.location()))
+                .setParameter(METADATA_LOCATION_PROP, newMetadataLocation)
+                .setParameter(PREVIOUS_METADATA_LOCATION_PROP, currentMetadataLocation)
+                .build();
 
         // todo privileges should not be replaced for an alter
         PrincipalPrivileges privileges = owner.isEmpty() && table.getOwner().isPresent() ? NO_PRIVILEGES : buildInitialPrivilegeSet(table.getOwner().get());