adutra commented on code in PR #8909:
URL: https://github.com/apache/iceberg/pull/8909#discussion_r1375268337
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieIcebergClient.java:
##########
@@ -378,27 +420,63 @@ public void renameTable(TableIdentifier from,
TableIdentifier to) {
// 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);
+ } else {
+ throw new NoSuchTableException("Table does not exist: %s", from);
+ }
+ }
+
+ IcebergContent existingToContent = isView ? view(to) : table(to);
Review Comment:
This is incorrect. `isView` doesn't mean that `existingToContent` is
necessarily a view. We have tests that exercise the case where we rename a view
to a table. In such cases, `existingToContent` will be a table.
Here is a correct version:
```java
IcebergContent existingToContent = asContent(to, IcebergContent.class);
if (existingToContent != null) {
Content.Type type = existingToContent.getType();
if (type == Content.Type.ICEBERG_VIEW) {
throw new AlreadyExistsException("Cannot rename %s to %s. View
already exists", from, to);
} else if (type == Content.Type.ICEBERG_TABLE) {
throw new AlreadyExistsException("Cannot rename %s to %s. Table
already exists", from, to);
} else {
throw new AlreadyExistsException("Cannot rename %s to %s. Another
content with same name already exists", from, to);
}
}
```
With the above change you can then remove the catch block catching
`NessieBadRequestException`.
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java:
##########
@@ -347,4 +348,54 @@ private TableIdentifier identifierWithoutTableReference(
protected Map<String, String> properties() {
return catalogOptions;
}
+
+ @Override
+ protected ViewOperations newViewOps(TableIdentifier identifier) {
+ TableReference tr = parseTableReference(identifier);
+ return new NessieViewOperations(
+ ContentKey.of(
+
org.projectnessie.model.Namespace.of(identifier.namespace().levels()),
tr.getName()),
+ client.withReference(tr.getReference(), tr.getHash()),
+ fileIO,
+ catalogOptions);
+ }
+
+ @Override
+ public List<TableIdentifier> listViews(Namespace namespace) {
+ return client.listViews(namespace);
+ }
+
+ @Override
+ public boolean dropView(TableIdentifier identifier) {
+ TableReference tableReference = parseTableReference(identifier);
+ return client
+ .withReference(tableReference.getReference(), tableReference.getHash())
+ .dropView(identifierWithoutTableReference(identifier, tableReference),
false);
+ }
+
+ @Override
+ public void renameView(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())
+ .renameView(
+ identifierWithoutTableReference(from, fromTableReference),
+ NessieUtil.removeCatalogName(
Review Comment:
The code here is mirroring the existing code for `renameTable`, where a call
to `NessieUtil.removeCatalogName` exists since the beginning of the Nessie
module:
https://github.com/apache/iceberg/blob/87143d53c05de308332860be12a450a8c7afb95f/nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java#L167
I don't see any test exercising this, so indeed it would be good to revisit
this logic. That said, since it's not new to this PR, I'd be in favor of doing
so in a follow-up PR.
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieIcebergClient.java:
##########
@@ -180,6 +195,16 @@ public IcebergTable table(TableIdentifier tableIdentifier)
{
}
}
+ public IcebergView view(TableIdentifier tableIdentifier) {
Review Comment:
Maybe this method could be merged with `table` since they are very similar:
```java
public <C extends IcebergContent> C asContent(
TableIdentifier tableIdentifier, Class<? extends C> targetClass) {
try {
ContentKey key = NessieUtil.toKey(tableIdentifier);
Content c = withReference(api.getContent().key(key)).get().get(key);
return c != null ? c.unwrap(targetClass).orElse(null) : null;
} catch (NessieNotFoundException e) {
return null;
}
}
```
Then you can call it like below:
```java
IcebergContent existingToContent = asContent(to, isView ? IcebergView.class
: IcebergTable.class);
```
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieIcebergClient.java:
##########
@@ -355,21 +384,34 @@ public void renameTable(TableIdentifier from,
TableIdentifier to) {
// and removed by another.
throw new RuntimeException(
String.format(
- "Cannot rename table '%s' to '%s': " + "ref '%s' no longer
exists.",
- from.name(), to.name(), getRef().getName()),
+ "Cannot rename %s '%s' to '%s': ref '%s' no longer exists.",
+ contentType, from, to, getRef().getName()),
e);
} catch (BaseNessieClientServerException e) {
throw new CommitFailedException(
e,
- "Cannot rename table '%s' to '%s': " + "the current reference is not
up to date.",
- from.name(),
- to.name());
+ "Cannot rename %s '%s' to '%s': the current reference is not up to
date.",
+ contentType,
+ from,
+ to);
} catch (HttpClientException ex) {
// 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
// details and all kinds of network devices can induce unexpected
behavior. So better be
// safe than sorry.
throw new CommitStateUnknownException(ex);
+ } catch (NessieBadRequestException ex) {
Review Comment:
This catch block is not correct. We should never catch
`NessieBadRequestException` in fact, as this denotes a misbehaving client.
What is happening here is that `validateContentForRename` is not working as
designed: see my next comment for a solution.
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieIcebergClient.java:
##########
@@ -136,15 +143,23 @@ private UpdateableReference loadReference(String
requestedRef, String hash) {
}
public List<TableIdentifier> listTables(Namespace namespace) {
+ return listContents(namespace, Content.Type.ICEBERG_TABLE);
+ }
+
+ public List<TableIdentifier> listViews(Namespace namespace) {
+ return listContents(namespace, Content.Type.ICEBERG_VIEW);
+ }
+
+ private List<TableIdentifier> listContents(Namespace namespace, Content.Type
type) {
Review Comment:
I think the plural is fine here: in English, "content" can be countable or
uncountable, depending on the context. Here, it's countable, and so can appear
in the plural.
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieTableOperations.java:
##########
@@ -135,71 +135,26 @@ protected void doCommit(TableMetadata base, TableMetadata
metadata) {
boolean newTable = base == null;
String newMetadataLocation = writeNewMetadataIfRequired(newTable,
metadata);
- String refName = client.refName();
- boolean failure = false;
+ AtomicBoolean failure = new AtomicBoolean(false);
try {
String contentId = table == null ? null : table.getId();
client.commitTable(base, metadata, newMetadataLocation, contentId, key);
- } catch (NessieConflictException ex) {
- failure = true;
- if (ex instanceof NessieReferenceConflictException) {
- // Throws a specialized exception, if possible
- maybeThrowSpecializedException((NessieReferenceConflictException) ex);
+ } catch (NessieConflictException | NessieNotFoundException |
HttpClientException ex) {
+ NessieUtil.handleExceptionsForCommits(ex, client.refName(), failure);
+ } catch (NessieBadRequestException ex) {
Review Comment:
Here too, I think this catch block is a code smell, but this one is trickier
to solve.
The issue is that in the test called
`createTableViaTransactionThatAlreadyExistsAsView`, we create a transaction
based on current hash (let's call it c0), then we commit something outside the
transaction (let's call it c1), then we attempt to commit the transaction.
As a result, the transaction/client is trying to commit using the expected
hash of c1, but it should really use c0 as the expected commit hash.
There is a property to convey the commit ID: `NESSIE_COMMIT_ID_PROPERTY`,
but it doesn't work when creating things, only when updating.
I *think* we could solve the problem by storing the current expected hash in
a field inside `NessieTableOperations`, then passing that to the client:
```java
public class NessieTableOperations extends BaseMetastoreTableOperations {
...
private Reference reference;
...
NessieTableOperations(...) {
...
reference = client.getRef().getReference();
}
...
protected void doRefresh() {
...
String metadataLocation = null;
this.reference = client.getRef().getReference();
...
}
...
protected void doCommit(TableMetadata base, TableMetadata metadata) {
...
client.commitTable(base, metadata, newMetadataLocation, contentId,
key, reference);
...
}
}
```
I did a quick experiment with the above suggestion and the test threw
`NessieConflictException` as expected, instead of `NessieBadRequestException`.
@dimas-b what would you suggest in this situation?
--
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-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
