From aead7d39a807f9dbdbff291e4c1e890f28e37bfa Mon Sep 17 00:00:00 2001 From: ajantha-bhat Date: Mon, 30 Oct 2023 17:44:09 +0530 Subject: [PATCH] Address reveiw comments --- .../apache/iceberg/nessie/NessieCatalog.java | 54 +++++----- .../iceberg/nessie/NessieIcebergClient.java | 98 +++++++++---------- .../iceberg/nessie/NessieTableOperations.java | 24 +++-- .../org/apache/iceberg/nessie/NessieUtil.java | 18 ++-- .../iceberg/nessie/NessieViewOperations.java | 26 +++-- .../iceberg/nessie/TestNessieTable.java | 6 +- .../apache/iceberg/nessie/TestNessieView.java | 6 +- 7 files changed, 114 insertions(+), 118 deletions(-) diff --git a/nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java b/nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java index 332f801dd9db..c8ca55b313d3 100644 --- a/nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java +++ b/nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java @@ -48,6 +48,7 @@ import org.projectnessie.client.api.NessieApiV2; import org.projectnessie.client.config.NessieClientConfigSource; import org.projectnessie.client.config.NessieClientConfigSources; +import org.projectnessie.model.Content; import org.projectnessie.model.ContentKey; import org.projectnessie.model.TableReference; import org.slf4j.Logger; @@ -245,28 +246,7 @@ public boolean dropTable(TableIdentifier identifier, boolean purge) { @Override public void renameTable(TableIdentifier from, TableIdentifier to) { - TableReference fromTableReference = parseTableReference(from); - TableReference toTableReference = parseTableReference(to); - String fromReference = - fromTableReference.hasReference() - ? fromTableReference.getReference() - : client.getRef().getName(); - String toReference = - toTableReference.hasReference() - ? toTableReference.getReference() - : client.getRef().getName(); - Preconditions.checkArgument( - fromReference.equalsIgnoreCase(toReference), - "from: %s and to: %s reference name must be same", - fromReference, - toReference); - - client - .withReference(fromTableReference.getReference(), fromTableReference.getHash()) - .renameTable( - identifierWithoutTableReference(from, fromTableReference), - NessieUtil.removeCatalogName( - identifierWithoutTableReference(to, toTableReference), name())); + renameContent(from, to, Content.Type.ICEBERG_TABLE); } @Override @@ -375,6 +355,10 @@ public boolean dropView(TableIdentifier identifier) { @Override public void renameView(TableIdentifier from, TableIdentifier to) { + renameContent(from, to, Content.Type.ICEBERG_VIEW); + } + + private void renameContent(TableIdentifier from, TableIdentifier to, Content.Type type) { TableReference fromTableReference = parseTableReference(from); TableReference toTableReference = parseTableReference(to); String fromReference = @@ -387,15 +371,27 @@ public void renameView(TableIdentifier from, TableIdentifier to) { : client.getRef().getName(); Preconditions.checkArgument( fromReference.equalsIgnoreCase(toReference), - "from: %s and to: %s reference name must be same", + "Reference name of identifiers in from: %s and to: %s must be same for rename operation", fromReference, toReference); - client - .withReference(fromTableReference.getReference(), fromTableReference.getHash()) - .renameView( - identifierWithoutTableReference(from, fromTableReference), - NessieUtil.removeCatalogName( - identifierWithoutTableReference(to, toTableReference), name())); + TableIdentifier fromIdentifier = + NessieUtil.removeCatalogName( + identifierWithoutTableReference(from, fromTableReference), name()); + TableIdentifier toIdentifier = + NessieUtil.removeCatalogName(identifierWithoutTableReference(to, toTableReference), name()); + + if (type == Content.Type.ICEBERG_TABLE) { + client + .withReference(fromTableReference.getReference(), fromTableReference.getHash()) + .renameTable(fromIdentifier, toIdentifier); + } else if (type == Content.Type.ICEBERG_VIEW) { + client + .withReference(fromTableReference.getReference(), fromTableReference.getHash()) + .renameView(fromIdentifier, toIdentifier); + } else { + throw new UnsupportedOperationException( + "Rename is not supported for content type:" + type.name()); + } } } diff --git a/nessie/src/main/java/org/apache/iceberg/nessie/NessieIcebergClient.java b/nessie/src/main/java/org/apache/iceberg/nessie/NessieIcebergClient.java index 4eb34630fc9d..05ab011866fc 100644 --- a/nessie/src/main/java/org/apache/iceberg/nessie/NessieIcebergClient.java +++ b/nessie/src/main/java/org/apache/iceberg/nessie/NessieIcebergClient.java @@ -46,7 +46,6 @@ import org.projectnessie.client.api.OnReferenceBuilder; import org.projectnessie.client.http.HttpClientException; import org.projectnessie.error.BaseNessieClientServerException; -import org.projectnessie.error.NessieBadRequestException; import org.projectnessie.error.NessieConflictException; import org.projectnessie.error.NessieNamespaceAlreadyExistsException; import org.projectnessie.error.NessieNamespaceNotEmptyException; @@ -185,21 +184,12 @@ private TableIdentifier toIdentifier(EntriesResponse.Entry entry) { return TableIdentifier.of(elements.toArray(new String[elements.size()])); } - public IcebergTable table(TableIdentifier tableIdentifier) { + public C asContent( + TableIdentifier tableIdentifier, Class targetClass) { try { ContentKey key = NessieUtil.toKey(tableIdentifier); - Content table = withReference(api.getContent().key(key)).get().get(key); - return table != null ? table.unwrap(IcebergTable.class).orElse(null) : null; - } catch (NessieNotFoundException e) { - return null; - } - } - - public IcebergView view(TableIdentifier tableIdentifier) { - try { - ContentKey key = NessieUtil.toKey(tableIdentifier); - Content view = withReference(api.getContent().key(key)).get().get(key); - return view != null ? view.unwrap(IcebergView.class).orElse(null) : null; + Content content = withReference(api.getContent().key(key)).get().get(key); + return content != null ? content.unwrap(targetClass).orElse(null) : null; } catch (NessieNotFoundException e) { return null; } @@ -337,27 +327,30 @@ namespace, getRef().getName()), } public void renameTable(TableIdentifier from, TableIdentifier to) { - renameContent(from, to, false); + renameContent(from, to, Content.Type.ICEBERG_TABLE); } public void renameView(TableIdentifier from, TableIdentifier to) { - renameContent(from, to, true); + renameContent(from, to, Content.Type.ICEBERG_VIEW); } - private void renameContent(TableIdentifier from, TableIdentifier to, boolean isView) { + private void renameContent(TableIdentifier from, TableIdentifier to, Content.Type type) { getRef().checkMutable(); - String contentType = isView ? "view" : "table"; + IcebergContent existingFromContent = asContent(from, IcebergContent.class); + validateFromContentForRename(from, type, existingFromContent); - IcebergContent existingFromContent = isView ? view(from) : table(from); - validateContentForRename(from, to, isView, existingFromContent); + IcebergContent existingToContent = asContent(to, IcebergContent.class); + validateToContentForRename(from, to, existingToContent); + String contentTypeString = type == Content.Type.ICEBERG_VIEW ? "view" : "table"; CommitMultipleOperationsBuilder operations = getApi() .commitMultipleOperations() .commitMeta( NessieUtil.buildCommitMetadata( - String.format("Iceberg rename %s from '%s' to '%s'", contentType, from, to), + String.format( + "Iceberg rename %s from '%s' to '%s'", contentTypeString, from, to), catalogOptions)) .operation(Operation.Delete.of(NessieUtil.toKey(from))) .operation(Operation.Put.of(NessieUtil.toKey(to), existingFromContent)); @@ -385,13 +378,13 @@ private void renameContent(TableIdentifier from, TableIdentifier to, boolean isV throw new RuntimeException( String.format( "Cannot rename %s '%s' to '%s': ref '%s' no longer exists.", - contentType, from, to, getRef().getName()), + contentTypeString, from, to, getRef().getName()), e); } catch (BaseNessieClientServerException e) { throw new CommitFailedException( e, "Cannot rename %s '%s' to '%s': the current reference is not up to date.", - contentType, + contentTypeString, from, to); } catch (HttpClientException ex) { @@ -400,18 +393,6 @@ contentType, from, to, getRef().getName()), // details and all kinds of network devices can induce unexpected behavior. So better be // safe than sorry. throw new CommitStateUnknownException(ex); - } catch (NessieBadRequestException ex) { - if (ex.getMessage().contains("already exists with content ID")) { - if (isView) { - // View might have created with the same name concurrently. - throw new AlreadyExistsException( - ex, "Cannot rename %s to %s. Table already exists", from, to); - } else { - throw new AlreadyExistsException( - ex, "Cannot rename %s to %s. View already exists", from, to); - } - } - throw ex; } // Intentionally just "throw through" Nessie's HttpClientException here and do not "special // case" @@ -420,46 +401,55 @@ contentType, from, to, getRef().getName()), // behavior. So better be safe than sorry. } - private void validateContentForRename( - TableIdentifier from, - TableIdentifier to, - boolean isView, - IcebergContent existingFromContent) { - if (existingFromContent == null) { - if (isView) { - throw new NoSuchViewException("View does not exist: %s", from); + private static void validateToContentForRename( + TableIdentifier from, TableIdentifier to, IcebergContent existingToContent) { + if (existingToContent != null) { + if (existingToContent.getType() == Content.Type.ICEBERG_VIEW) { + throw new AlreadyExistsException("Cannot rename %s to %s. View already exists", from, to); + } else if (existingToContent.getType() == Content.Type.ICEBERG_TABLE) { + throw new AlreadyExistsException("Cannot rename %s to %s. Table already exists", from, to); } else { - throw new NoSuchTableException("Table does not exist: %s", from); + throw new AlreadyExistsException( + "Cannot rename %s to %s. Another content with same name already exists", from, to); } } + } - IcebergContent existingToContent = isView ? view(to) : table(to); - if (existingToContent != null) { - if (isView) { - throw new AlreadyExistsException("Cannot rename %s to %s. View already exists", from, to); + private static void validateFromContentForRename( + TableIdentifier from, Content.Type type, IcebergContent existingFromContent) { + if (existingFromContent == null) { + if (type == Content.Type.ICEBERG_VIEW) { + throw new NoSuchViewException("View does not exist: %s", from); + } else if (type == Content.Type.ICEBERG_TABLE) { + throw new NoSuchTableException("Table does not exist: %s", from); } else { - throw new AlreadyExistsException("Cannot rename %s to %s. Table already exists", from, to); + throw new UnsupportedOperationException("Cannot rename for content type: " + type); } + } else if (existingFromContent.getType() != type) { + throw new UnsupportedOperationException( + "content type of from identifier should be of " + type); } } public boolean dropTable(TableIdentifier identifier, boolean purge) { - return dropContent(identifier, purge, false); + return dropContent(identifier, purge, Content.Type.ICEBERG_TABLE); } public boolean dropView(TableIdentifier identifier, boolean purge) { - return dropContent(identifier, purge, true); + return dropContent(identifier, purge, Content.Type.ICEBERG_VIEW); } - private boolean dropContent(TableIdentifier identifier, boolean purge, boolean isView) { + private boolean dropContent(TableIdentifier identifier, boolean purge, Content.Type type) { getRef().checkMutable(); - IcebergContent existingContent = isView ? view(identifier) : table(identifier); + IcebergContent existingContent = + asContent( + identifier, type == Content.Type.ICEBERG_VIEW ? IcebergView.class : IcebergTable.class); if (existingContent == null) { return false; } - String contentType = isView ? "view" : "table"; + String contentType = type == Content.Type.ICEBERG_VIEW ? "view" : "table"; if (purge) { LOG.info( diff --git a/nessie/src/main/java/org/apache/iceberg/nessie/NessieTableOperations.java b/nessie/src/main/java/org/apache/iceberg/nessie/NessieTableOperations.java index dbcbe1b14225..c628e2dbe586 100644 --- a/nessie/src/main/java/org/apache/iceberg/nessie/NessieTableOperations.java +++ b/nessie/src/main/java/org/apache/iceberg/nessie/NessieTableOperations.java @@ -27,7 +27,6 @@ import org.apache.iceberg.exceptions.NoSuchTableException; import org.apache.iceberg.io.FileIO; import org.projectnessie.client.http.HttpClientException; -import org.projectnessie.error.NessieBadRequestException; import org.projectnessie.error.NessieConflictException; import org.projectnessie.error.NessieNotFoundException; import org.projectnessie.model.Content; @@ -132,6 +131,19 @@ protected void doRefresh() { @Override protected void doCommit(TableMetadata base, TableMetadata metadata) { + Content content = null; + try { + content = + client.getApi().getContent().key(key).reference(client.getReference()).get().get(key); + } catch (NessieNotFoundException e) { + // Ignore the exception as the first commit may not have the content present for the key. + } + + if (content != null && content.getType() == Content.Type.ICEBERG_VIEW) { + throw new AlreadyExistsException( + "View with same name already exists: %s in %s", key, client.getReference()); + } + boolean newTable = base == null; String newMetadataLocation = writeNewMetadataIfRequired(newTable, metadata); @@ -140,14 +152,8 @@ protected void doCommit(TableMetadata base, TableMetadata metadata) { String contentId = table == null ? null : table.getId(); client.commitTable(base, metadata, newMetadataLocation, contentId, key); } catch (NessieConflictException | NessieNotFoundException | HttpClientException ex) { - NessieUtil.handleExceptionsForCommits(ex, client.refName(), failure); - } catch (NessieBadRequestException ex) { - if (ex.getMessage().contains("New value to update existing key")) { - failure.set(true); - // View might have created with the same name concurrently. - throw new AlreadyExistsException(ex, "View with same name already exists: %s", key); - } - throw ex; + NessieUtil.handleExceptionsForCommits( + ex, client.refName(), failure, Content.Type.ICEBERG_TABLE); } finally { if (failure.get()) { io().deleteFile(newMetadataLocation); diff --git a/nessie/src/main/java/org/apache/iceberg/nessie/NessieUtil.java b/nessie/src/main/java/org/apache/iceberg/nessie/NessieUtil.java index eb8aef15020a..10245af0ffbf 100644 --- a/nessie/src/main/java/org/apache/iceberg/nessie/NessieUtil.java +++ b/nessie/src/main/java/org/apache/iceberg/nessie/NessieUtil.java @@ -41,13 +41,13 @@ import org.apache.iceberg.relocated.com.google.common.collect.Lists; import org.apache.iceberg.relocated.com.google.common.collect.Maps; import org.projectnessie.client.http.HttpClientException; -import org.projectnessie.error.NessieBadRequestException; import org.projectnessie.error.NessieConflictException; import org.projectnessie.error.NessieNotFoundException; import org.projectnessie.error.NessieReferenceConflictException; import org.projectnessie.error.ReferenceConflicts; import org.projectnessie.model.CommitMeta; import org.projectnessie.model.Conflict; +import org.projectnessie.model.Content; import org.projectnessie.model.ContentKey; import org.projectnessie.model.IcebergTable; import org.projectnessie.model.ImmutableCommitMeta; @@ -182,13 +182,13 @@ public static TableMetadata updateTableMetadataWithNessieSpecificProperties( } static void handleExceptionsForCommits( - Exception exception, String refName, AtomicBoolean failure) { + Exception exception, String refName, AtomicBoolean failure, Content.Type type) { if (exception instanceof NessieConflictException) { failure.set(true); if (exception instanceof NessieReferenceConflictException) { // Throws a specialized exception, if possible NessieUtil.maybeThrowSpecializedException( - (NessieReferenceConflictException) exception, false); + (NessieReferenceConflictException) exception, type); } throw new CommitFailedException( @@ -204,12 +204,6 @@ static void handleExceptionsForCommits( String.format("Cannot commit: Reference '%s' no longer exists", refName), exception); } - if (exception instanceof NessieBadRequestException) { - failure.set(true); - throw new RuntimeException( - String.format("Cannot commit: Reference '%s' no longer exists", refName), exception); - } - if (exception instanceof HttpClientException) { // Intentionally catch all nessie-client-exceptions here and not just the "timeout" variant // to catch all kinds of network errors (e.g. connection reset). Network code implementation @@ -220,7 +214,7 @@ static void handleExceptionsForCommits( } private static void maybeThrowSpecializedException( - NessieReferenceConflictException ex, boolean isView) { + NessieReferenceConflictException ex, Content.Type type) { // Check if the server returned 'ReferenceConflicts' information ReferenceConflicts referenceConflicts = ex.getErrorDetails(); if (referenceConflicts == null) { @@ -233,7 +227,7 @@ private static void maybeThrowSpecializedException( return; } - String contentType = isView ? "View" : "Table"; + String contentType = type == Content.Type.ICEBERG_VIEW ? "View" : "Table"; Conflict conflict = conflicts.get(0); Conflict.ConflictType conflictType = conflict.conflictType(); @@ -244,7 +238,7 @@ private static void maybeThrowSpecializedException( case NAMESPACE_NOT_EMPTY: throw new NamespaceNotEmptyException(ex, "Namespace not empty: %s", conflict.key()); case KEY_DOES_NOT_EXIST: - if (isView) { + if (type == Content.Type.ICEBERG_VIEW) { throw new NoSuchViewException(ex, "%s does not exist: %s", contentType, conflict.key()); } else { throw new NoSuchTableException( diff --git a/nessie/src/main/java/org/apache/iceberg/nessie/NessieViewOperations.java b/nessie/src/main/java/org/apache/iceberg/nessie/NessieViewOperations.java index 357d8fe5b7d4..862528eee358 100644 --- a/nessie/src/main/java/org/apache/iceberg/nessie/NessieViewOperations.java +++ b/nessie/src/main/java/org/apache/iceberg/nessie/NessieViewOperations.java @@ -28,7 +28,6 @@ import org.apache.iceberg.view.ViewMetadata; import org.apache.iceberg.view.ViewMetadataParser; import org.projectnessie.client.http.HttpClientException; -import org.projectnessie.error.NessieBadRequestException; import org.projectnessie.error.NessieConflictException; import org.projectnessie.error.NessieNotFoundException; import org.projectnessie.model.Content; @@ -44,7 +43,6 @@ public class NessieViewOperations extends BaseViewOperations { private static final Logger LOG = LoggerFactory.getLogger(NessieViewOperations.class); private final NessieIcebergClient client; - private final ContentKey key; private final FileIO fileIO; private final Map catalogOptions; @@ -119,6 +117,20 @@ private ViewMetadata loadViewMetadata(String metadataLocation, Reference referen @Override public void doCommit(ViewMetadata base, ViewMetadata metadata) { + + Content content = null; + try { + content = + client.getApi().getContent().key(key).reference(client.getReference()).get().get(key); + } catch (NessieNotFoundException e) { + // Ignore the exception as the first commit may not have the content present for the key. + } + + if (content != null && content.getType() == Content.Type.ICEBERG_TABLE) { + throw new AlreadyExistsException( + "Table with same name already exists: %s in %s", key, client.getReference()); + } + String newMetadataLocation = writeNewMetadataIfRequired(metadata); AtomicBoolean failure = new AtomicBoolean(false); @@ -126,14 +138,8 @@ public void doCommit(ViewMetadata base, ViewMetadata metadata) { String contentId = icebergView == null ? null : icebergView.getId(); client.commitView(base, metadata, newMetadataLocation, contentId, key); } catch (NessieConflictException | HttpClientException | NessieNotFoundException ex) { - NessieUtil.handleExceptionsForCommits(ex, client.refName(), failure); - } catch (NessieBadRequestException ex) { - if (ex.getMessage().contains("New value to update existing key")) { - failure.set(true); - // Table might have created with the same name concurrently. - throw new AlreadyExistsException(ex, "Table with same name already exists: %s", key); - } - throw ex; + NessieUtil.handleExceptionsForCommits( + ex, client.refName(), failure, Content.Type.ICEBERG_VIEW); } finally { if (failure.get()) { io().deleteFile(newMetadataLocation); diff --git a/nessie/src/test/java/org/apache/iceberg/nessie/TestNessieTable.java b/nessie/src/test/java/org/apache/iceberg/nessie/TestNessieTable.java index f7fcbd3275aa..d9ce05286a91 100644 --- a/nessie/src/test/java/org/apache/iceberg/nessie/TestNessieTable.java +++ b/nessie/src/test/java/org/apache/iceberg/nessie/TestNessieTable.java @@ -305,7 +305,8 @@ public void testRenameWithTableReferenceInvalidCase() throws NessieNotFoundExcep Assertions.assertThatThrownBy(() -> catalog.renameTable(fromIdentifier, toIdentifier)) .isInstanceOf(IllegalArgumentException.class) - .hasMessage("from: Something and to: iceberg-table-test reference name must be same"); + .hasMessage( + "Reference name of identifiers in from: Something and to: iceberg-table-test must be same for rename operation"); fromTableReference = ImmutableTableReference.builder() @@ -324,7 +325,8 @@ public void testRenameWithTableReferenceInvalidCase() throws NessieNotFoundExcep Assertions.assertThatThrownBy(() -> catalog.renameTable(fromIdentifierNew, toIdentifierNew)) .isInstanceOf(IllegalArgumentException.class) - .hasMessage("from: iceberg-table-test and to: Something reference name must be same"); + .hasMessage( + "Reference name of identifiers in from: iceberg-table-test and to: Something must be same for rename operation"); } private void verifyCommitMetadata() throws NessieNotFoundException { diff --git a/nessie/src/test/java/org/apache/iceberg/nessie/TestNessieView.java b/nessie/src/test/java/org/apache/iceberg/nessie/TestNessieView.java index 355755580229..1199c5b0d966 100644 --- a/nessie/src/test/java/org/apache/iceberg/nessie/TestNessieView.java +++ b/nessie/src/test/java/org/apache/iceberg/nessie/TestNessieView.java @@ -268,7 +268,8 @@ public void testRenameWithTableReferenceInvalidCase() { Assertions.assertThatThrownBy(() -> catalog.renameView(fromIdentifier, toIdentifier)) .isInstanceOf(IllegalArgumentException.class) - .hasMessage("from: Something and to: iceberg-view-test reference name must be same"); + .hasMessage( + "Reference name of identifiers in from: Something and to: iceberg-view-test must be same for rename operation"); fromTableReference = ImmutableTableReference.builder() @@ -287,7 +288,8 @@ public void testRenameWithTableReferenceInvalidCase() { Assertions.assertThatThrownBy(() -> catalog.renameView(fromIdentifierNew, toIdentifierNew)) .isInstanceOf(IllegalArgumentException.class) - .hasMessage("from: iceberg-view-test and to: Something reference name must be same"); + .hasMessage( + "Reference name of identifiers in from: iceberg-view-test and to: Something must be same for rename operation"); } private void verifyCommitMetadata() throws NessieNotFoundException {