omarsmak commented on a change in pull request #3257:
URL: https://github.com/apache/iceberg/pull/3257#discussion_r737313828
##########
File path:
nessie/src/main/java/org/apache/iceberg/nessie/UpdateableReference.java
##########
@@ -20,32 +20,38 @@
package org.apache.iceberg.nessie;
import java.util.Objects;
-import org.projectnessie.api.TreeApi;
+import org.projectnessie.client.api.NessieApiV1;
import org.projectnessie.error.NessieNotFoundException;
import org.projectnessie.model.Branch;
-import org.projectnessie.model.Hash;
import org.projectnessie.model.Reference;
class UpdateableReference {
private Reference reference;
- private final TreeApi client;
+ private final boolean mutable;
- UpdateableReference(Reference reference, TreeApi client) {
+ /**
+ * Construct a new {@link UpdateableReference} using a Nessie reference
object and a flag
+ * whether an explicit hash was used to create the reference object.
+ */
+ UpdateableReference(Reference reference, boolean hashReference) {
this.reference = reference;
- this.client = client;
+ this.mutable = reference instanceof Branch && !hashReference;
}
- public boolean refresh() throws NessieNotFoundException {
- if (reference instanceof Hash) {
+ public boolean refresh(NessieApiV1 api) throws NessieNotFoundException {
+ if (!mutable) {
return false;
}
Reference oldReference = reference;
- reference = client.getReferenceByName(reference.getName());
+ reference = api.getReference().refName(reference.getName()).get();
return !oldReference.equals(reference);
}
public void updateReference(Reference ref) {
+ if (!mutable) {
Review comment:
I think `Preconditions.checkState(mutable, "Hash references cannot be
updated.")` would be nicer.
##########
File path: nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java
##########
@@ -214,22 +224,21 @@ public void renameTable(TableIdentifier from,
TableIdentifier toOriginal) {
throw new AlreadyExistsException("table %s already exists", to.name());
}
- Operations contents = ImmutableOperations.builder()
- .addOperations(
-
ImmutablePut.builder().key(NessieUtil.toKey(to)).contents(existingFromTable).build(),
- ImmutableDelete.builder().key(NessieUtil.toKey(from)).build())
+ CommitMultipleOperationsBuilder op = api.commitMultipleOperations()
.commitMeta(NessieUtil.buildCommitMetadata("iceberg rename table",
catalogOptions))
- .build();
+ .operation(Operation.Put.of(NessieUtil.toKey(to), existingFromTable,
existingFromTable))
+ .operation(Operation.Delete.of(NessieUtil.toKey(from)));
try {
- Tasks.foreach(contents)
+ Tasks.foreach(op)
.retry(5)
.stopRetryOn(NessieNotFoundException.class)
.throwFailureWhenFinished()
- .onFailure((c, exception) -> refresh())
- .run(c -> {
- Branch branch =
client.getTreeApi().commitMultipleOperations(reference.getAsBranch().getName(),
- reference.getHash(), c);
+ .onFailure((o, exception) -> refresh())
+ .run(o -> {
Review comment:
nit: perhaps singular `operation` would make sense instead of `o` here?
##########
File path: nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java
##########
@@ -214,22 +224,21 @@ public void renameTable(TableIdentifier from,
TableIdentifier toOriginal) {
throw new AlreadyExistsException("table %s already exists", to.name());
}
- Operations contents = ImmutableOperations.builder()
- .addOperations(
-
ImmutablePut.builder().key(NessieUtil.toKey(to)).contents(existingFromTable).build(),
- ImmutableDelete.builder().key(NessieUtil.toKey(from)).build())
+ CommitMultipleOperationsBuilder op = api.commitMultipleOperations()
Review comment:
nit: Can we change `op` to something plural like `operations` to have
more readable.
##########
File path:
nessie/src/main/java/org/apache/iceberg/nessie/UpdateableReference.java
##########
@@ -69,9 +71,14 @@ public Branch getAsBranch() {
return (Branch) reference;
}
+ public Reference getReference() {
+ return reference;
+ }
+
public void checkMutable() {
- if (!isBranch()) {
- throw new IllegalArgumentException("You can only mutate tables when
using a branch.");
+ if (!mutable) {
Review comment:
Here is as well, perhaps `Preconditions.checkArgument` would be nicer
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
