Skip to content

Commit

Permalink
Address reveiw comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@ajantha-bhat
ajantha-bhat committed Oct 30, 2023
1 parent 89b3a21 commit aead7d3
Show file tree
Hide file tree
Showing 7 changed files with 114 additions and 118 deletions.
54 changes: 25 additions & 29 deletions nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 =
Expand All @@ -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());
}
}
}
98 changes: 44 additions & 54 deletions nessie/src/main/java/org/apache/iceberg/nessie/NessieIcebergClient.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 extends IcebergContent> C asContent(
TableIdentifier tableIdentifier, Class<? extends C> 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;
}
Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -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) {
Expand All @@ -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"
Expand All @@ -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(
Expand Down
24 changes: 15 additions & 9 deletions nessie/src/main/java/org/apache/iceberg/nessie/NessieTableOperations.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);

Expand All @@ -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);
Expand Down
18 changes: 6 additions & 12 deletions nessie/src/main/java/org/apache/iceberg/nessie/NessieUtil.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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(
Expand All @@ -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
Expand All @@ -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) {
Expand All @@ -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();
Expand All @@ -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(
Expand Down
Loading

0 comments on commit aead7d3

Please sign in to comment.