adutra commented on code in PR #8857:
URL: https://github.com/apache/iceberg/pull/8857#discussion_r1364490041
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieIcebergClient.java:
##########
@@ -181,21 +182,76 @@ public IcebergTable table(TableIdentifier
tableIdentifier) {
}
public void createNamespace(Namespace namespace, Map<String, String>
metadata) {
+ getRef().checkMutable();
+
+ if (namespace.isEmpty()) {
+ throw new IllegalArgumentException("Creating empty namespaces is not
supported");
+ }
+
+ ContentKey key = ContentKey.of(namespace.levels());
+ org.projectnessie.model.Namespace content =
+ org.projectnessie.model.Namespace.of(key.getElements(), metadata);
+
try {
- getRef().checkMutable();
- withReference(
- getApi()
- .createNamespace()
-
.namespace(org.projectnessie.model.Namespace.of(namespace.levels()))
- .properties(metadata))
- .create();
- refresh();
- } catch (NessieNamespaceAlreadyExistsException e) {
- throw new AlreadyExistsException(e, "Namespace already exists: %s",
namespace);
+
+ // First, check that the namespace doesn't exist at the current hash;
Review Comment:
I added this check after talking with other Nessie devs on Zulip.
The idea is to prevent `NessieBadRequestException`s when committing. For
that, it is important to check whether the content already exists, _on the
exact same hash that is going to be used as expected hash for the commit_.
For that, I am deliberately avoiding a call to `withReference`, because that
method would fetch the contents on the current HEAD of the branch, which may be
a more recent one than the one we know.
If we did that, we would certainly reduce the likelihood of conflicts when
committing, _but we wouldn't eliminate it completely_ as someone else could
commit a conflicting key between the content-check and the commit steps.
But that, on the flip side, would make the conlfict handling code almost
untestable.
All in all, I think it is saner to test whether the content exists on the
last HEAD we know about, and let Nessie return a conflict, if something was
committed in the interim.
I added tests for all cases (content added at our HEAD, and content added by
someone else on an ulterior commit).
--
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
