From e23dfe87dc03c12089a7b58f7682d1b19e4d716c Mon Sep 17 00:00:00 2001 From: ajantha-bhat Date: Thu, 9 Feb 2023 19:53:11 +0530 Subject: [PATCH 1/7] Nessie: Handle refresh for catalog APIs that doesn't use table operations --- .../apache/iceberg/nessie/NessieCatalog.java | 39 +++-- .../iceberg/nessie/TestMultipleClients.java | 143 ++++++++++++++++++ 2 files changed, 168 insertions(+), 14 deletions(-) create mode 100644 nessie/src/test/java/org/apache/iceberg/nessie/TestMultipleClients.java 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 37755f5123b2..c0594c8b1967 100644 --- a/nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java +++ b/nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java @@ -47,6 +47,7 @@ import org.projectnessie.client.NessieConfigConstants; import org.projectnessie.client.api.NessieApiV1; import org.projectnessie.client.http.HttpClientBuilder; +import org.projectnessie.error.NessieNotFoundException; import org.projectnessie.model.ContentKey; import org.projectnessie.model.TableReference; import org.slf4j.Logger; @@ -194,7 +195,7 @@ protected TableOperations newTableOps(TableIdentifier tableIdentifier) { ContentKey.of( org.projectnessie.model.Namespace.of(tableIdentifier.namespace().levels()), tr.getName()), - client.withReference(tr.getReference(), tr.getHash()), + refreshedClient().withReference(tr.getReference(), tr.getHash()), fileIO, catalogOptions); } @@ -223,13 +224,13 @@ protected String defaultWarehouseLocation(TableIdentifier table) { @Override public List listTables(Namespace namespace) { - return client.listTables(namespace); + return refreshedClient().listTables(namespace); } @Override public boolean dropTable(TableIdentifier identifier, boolean purge) { TableReference tableReference = parseTableReference(identifier); - return client + return refreshedClient() .withReference(tableReference.getReference(), tableReference.getHash()) .dropTable(identifierWithoutTableReference(identifier, tableReference), purge); } @@ -238,21 +239,22 @@ public boolean dropTable(TableIdentifier identifier, boolean purge) { public void renameTable(TableIdentifier from, TableIdentifier to) { TableReference fromTableReference = parseTableReference(from); TableReference toTableReference = parseTableReference(to); + NessieIcebergClient nessieIcebergClient = refreshedClient(); String fromReference = fromTableReference.hasReference() ? fromTableReference.getReference() - : client.getRef().getName(); + : nessieIcebergClient.getRef().getName(); String toReference = toTableReference.hasReference() ? toTableReference.getReference() - : client.getRef().getName(); + : nessieIcebergClient.getRef().getName(); Preconditions.checkArgument( fromReference.equalsIgnoreCase(toReference), "from: %s and to: %s reference name must be same", fromReference, toReference); - client + nessieIcebergClient .withReference(fromTableReference.getReference(), fromTableReference.getHash()) .renameTable( identifierWithoutTableReference(from, fromTableReference), @@ -262,12 +264,12 @@ public void renameTable(TableIdentifier from, TableIdentifier to) { @Override public void createNamespace(Namespace namespace, Map metadata) { - client.createNamespace(namespace, metadata); + refreshedClient().createNamespace(namespace, metadata); } @Override public List listNamespaces(Namespace namespace) throws NoSuchNamespaceException { - return client.listNamespaces(namespace); + return refreshedClient().listNamespaces(namespace); } /** @@ -280,22 +282,22 @@ public List listNamespaces(Namespace namespace) throws NoSuchNamespac @Override public Map loadNamespaceMetadata(Namespace namespace) throws NoSuchNamespaceException { - return client.loadNamespaceMetadata(namespace); + return refreshedClient().loadNamespaceMetadata(namespace); } @Override public boolean dropNamespace(Namespace namespace) throws NamespaceNotEmptyException { - return client.dropNamespace(namespace); + return refreshedClient().dropNamespace(namespace); } @Override public boolean setProperties(Namespace namespace, Map properties) { - return client.setProperties(namespace, properties); + return refreshedClient().setProperties(namespace, properties); } @Override public boolean removeProperties(Namespace namespace, Set properties) { - return client.removeProperties(namespace, properties); + return refreshedClient().removeProperties(namespace, properties); } @Override @@ -310,12 +312,12 @@ public Configuration getConf() { @VisibleForTesting String currentHash() { - return client.getRef().getHash(); + return refreshedClient().getRef().getHash(); } @VisibleForTesting String currentRefName() { - return client.getRef().getName(); + return refreshedClient().getRef().getName(); } @VisibleForTesting @@ -323,6 +325,15 @@ FileIO fileIO() { return fileIO; } + private NessieIcebergClient refreshedClient() { + try { + client.refresh(); + } catch (NessieNotFoundException e) { + throw new RuntimeException(e); + } + return client; + } + private TableReference parseTableReference(TableIdentifier tableIdentifier) { TableReference tr = TableReference.parse(tableIdentifier.name()); Preconditions.checkArgument( diff --git a/nessie/src/test/java/org/apache/iceberg/nessie/TestMultipleClients.java b/nessie/src/test/java/org/apache/iceberg/nessie/TestMultipleClients.java new file mode 100644 index 000000000000..571c57ddc62e --- /dev/null +++ b/nessie/src/test/java/org/apache/iceberg/nessie/TestMultipleClients.java @@ -0,0 +1,143 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.iceberg.nessie; + +import static org.apache.iceberg.types.Types.NestedField.required; + +import java.io.File; +import java.io.IOException; +import java.net.URI; +import java.util.AbstractMap; +import java.util.Collections; +import org.apache.commons.io.FileUtils; +import org.apache.iceberg.Schema; +import org.apache.iceberg.Table; +import org.apache.iceberg.catalog.Namespace; +import org.apache.iceberg.catalog.TableIdentifier; +import org.apache.iceberg.types.Types; +import org.assertj.core.api.Assertions; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.projectnessie.client.ext.NessieClientFactory; +import org.projectnessie.client.ext.NessieClientUri; + +public class TestMultipleClients extends BaseTestIceberg { + + private static final String BRANCH = "multiple-clients-test"; + private static final Schema schema = + new Schema(Types.StructType.of(required(1, "id", Types.LongType.get())).fields()); + + public TestMultipleClients() { + super(BRANCH); + } + + // another client that connects to the same nessie server. + NessieCatalog anotherCatalog; + + @Override + @BeforeEach + public void beforeEach(NessieClientFactory clientFactory, @NessieClientUri URI nessieUri) + throws IOException { + super.beforeEach(clientFactory, nessieUri); + anotherCatalog = initCatalog(branch); + } + + @Override + @AfterEach + public void afterEach() throws Exception { + dropTables(catalog); + dropTables(anotherCatalog); + + super.afterEach(); + anotherCatalog.close(); + } + + @Test + public void testListNamespaces() { + catalog.createNamespace(Namespace.of("db1"), Collections.emptyMap()); + Assertions.assertThat(catalog.listNamespaces()).containsExactlyInAnyOrder(Namespace.of("db1")); + + // another client creates a namespace with the same nessie server + anotherCatalog.createNamespace(Namespace.of("db2"), Collections.emptyMap()); + Assertions.assertThat(anotherCatalog.listNamespaces()) + .containsExactlyInAnyOrder(Namespace.of("db1"), Namespace.of("db2")); + + Assertions.assertThat(catalog.listNamespaces()) + .containsExactlyInAnyOrder(Namespace.of("db1"), Namespace.of("db2")); + } + + @Test + public void testLoadNamespaceMetadata() { + catalog.createNamespace(Namespace.of("namespace1"), Collections.emptyMap()); + Assertions.assertThat(catalog.listNamespaces()) + .containsExactlyInAnyOrder(Namespace.of("namespace1")); + + // another client adds a metadata to the same namespace + anotherCatalog.setProperties(Namespace.of("namespace1"), Collections.singletonMap("k1", "v1")); + AbstractMap.SimpleEntry entry = new AbstractMap.SimpleEntry<>("k1", "v1"); + Assertions.assertThat(anotherCatalog.loadNamespaceMetadata(Namespace.of("namespace1"))) + .containsExactly(entry); + + Assertions.assertThat(catalog.loadNamespaceMetadata(Namespace.of("namespace1"))) + .containsExactly(entry); + } + + @Test + public void testListTables() { + catalog.createTable(TableIdentifier.parse("foo.tbl1"), schema); + Assertions.assertThat(catalog.listTables(Namespace.of("foo"))) + .containsExactlyInAnyOrder(TableIdentifier.parse("foo.tbl1")); + + // another client creates a table with the same nessie server + anotherCatalog.createTable(TableIdentifier.parse("foo.tbl2"), schema); + Assertions.assertThat(anotherCatalog.listTables(Namespace.of("foo"))) + .containsExactlyInAnyOrder( + TableIdentifier.parse("foo.tbl1"), TableIdentifier.parse("foo.tbl2")); + + Assertions.assertThat(catalog.listTables(Namespace.of("foo"))) + .containsExactlyInAnyOrder( + TableIdentifier.parse("foo.tbl1"), TableIdentifier.parse("foo.tbl2")); + } + + private static void dropTables(NessieCatalog nessieCatalog) { + nessieCatalog + .listNamespaces() + .forEach( + namespace -> + nessieCatalog + .listTables(namespace) + .forEach(identifier -> dropTable(nessieCatalog, identifier))); + } + + private static void dropTable(NessieCatalog nessieCatalog, TableIdentifier identifier) { + Table table = nessieCatalog.loadTable(identifier); + File tableLocation = tableLocation(table); + nessieCatalog.dropTable(identifier, false); + try { + FileUtils.deleteDirectory(tableLocation); + } catch (IOException e) { + throw new RuntimeException(e); + } + } + + private static File tableLocation(Table table) { + return new File(table.location().replaceFirst("file:", "")); + } +} From 6f632fdec218789ba5a0b8396553e1d324b61293 Mon Sep 17 00:00:00 2001 From: ajantha-bhat Date: Fri, 17 Feb 2023 19:30:34 +0530 Subject: [PATCH 2/7] Add commit testcase --- .../iceberg/nessie/TestMultipleClients.java | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/nessie/src/test/java/org/apache/iceberg/nessie/TestMultipleClients.java b/nessie/src/test/java/org/apache/iceberg/nessie/TestMultipleClients.java index 571c57ddc62e..870f3034817f 100644 --- a/nessie/src/test/java/org/apache/iceberg/nessie/TestMultipleClients.java +++ b/nessie/src/test/java/org/apache/iceberg/nessie/TestMultipleClients.java @@ -116,6 +116,23 @@ public void testListTables() { TableIdentifier.parse("foo.tbl1"), TableIdentifier.parse("foo.tbl2")); } + @Test + public void testCommits() { + TableIdentifier identifier = TableIdentifier.parse("foo.tbl1"); + catalog.createTable(identifier, schema); + Table tableFromCatalog = catalog.loadTable(identifier); + tableFromCatalog.updateSchema().addColumn("x1", Types.LongType.get()).commit(); + + Table tableFromAnotherCatalog = anotherCatalog.loadTable(identifier); + tableFromAnotherCatalog.updateSchema().addColumn("x2", Types.LongType.get()).commit(); + + tableFromCatalog.updateSchema().addColumn("x3", Types.LongType.get()).commit(); + tableFromAnotherCatalog.updateSchema().addColumn("x4", Types.LongType.get()).commit(); + + Assertions.assertThat(catalog.loadTable(identifier).schema().columns()).hasSize(5); + Assertions.assertThat(anotherCatalog.loadTable(identifier).schema().columns()).hasSize(5); + } + private static void dropTables(NessieCatalog nessieCatalog) { nessieCatalog .listNamespaces() From b84b5164e36e186ad87fce9d27b9a43f5569ca11 Mon Sep 17 00:00:00 2001 From: ajantha-bhat Date: Mon, 27 Feb 2023 21:35:05 +0530 Subject: [PATCH 3/7] Another test case --- .../iceberg/nessie/TestMultipleClients.java | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/nessie/src/test/java/org/apache/iceberg/nessie/TestMultipleClients.java b/nessie/src/test/java/org/apache/iceberg/nessie/TestMultipleClients.java index 870f3034817f..c3f3673ecd2d 100644 --- a/nessie/src/test/java/org/apache/iceberg/nessie/TestMultipleClients.java +++ b/nessie/src/test/java/org/apache/iceberg/nessie/TestMultipleClients.java @@ -28,8 +28,12 @@ import org.apache.commons.io.FileUtils; import org.apache.iceberg.Schema; import org.apache.iceberg.Table; +import org.apache.iceberg.TableMetadata; +import org.apache.iceberg.TableOperations; import org.apache.iceberg.catalog.Namespace; import org.apache.iceberg.catalog.TableIdentifier; +import org.apache.iceberg.exceptions.CommitFailedException; +import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap; import org.apache.iceberg.types.Types; import org.assertj.core.api.Assertions; import org.junit.jupiter.api.AfterEach; @@ -133,6 +137,34 @@ public void testCommits() { Assertions.assertThat(anotherCatalog.loadTable(identifier).schema().columns()).hasSize(5); } + @Test + public void testConcurrentCommitsWithRefresh() { + TableIdentifier identifier = TableIdentifier.parse("foo.tbl1"); + catalog.createTable(identifier, schema); + + String hashBefore = catalog.currentHash(); + + TableOperations ops1 = catalog.newTableOps(identifier); + TableMetadata current1 = + TableMetadata.buildFrom(ops1.current()).setProperties(ImmutableMap.of("k1", "v1")).build(); + + // commit should succeed + TableOperations ops2 = catalog.newTableOps(identifier); + TableMetadata current2 = + TableMetadata.buildFrom(ops2.current()).setProperties(ImmutableMap.of("k2", "v2")).build(); + ops2.commit(ops2.current(), current2); + + // refresh the catalog's client. + String hashAfter = catalog.currentHash(); + Assertions.assertThat(hashBefore).isNotEqualTo(hashAfter); + + // client refresh should not affect the ongoing commits (commit should still fail due staleness) + Assertions.assertThatThrownBy(() -> ops1.commit(ops1.current(), current1)) + .isInstanceOf(CommitFailedException.class) + .hasMessageContaining( + "Cannot commit: Reference hash is out of date. Update the reference 'multiple-clients-test' and try again"); + } + private static void dropTables(NessieCatalog nessieCatalog) { nessieCatalog .listNamespaces() From c6ee39e53a733f6b97df7a22bf937ca52aa9bf3b Mon Sep 17 00:00:00 2001 From: ajantha-bhat Date: Thu, 2 Mar 2023 08:12:48 +0530 Subject: [PATCH 4/7] Address comments --- .../apache/iceberg/nessie/NessieCatalog.java | 26 ++++++------ .../iceberg/nessie/TestMultipleClients.java | 40 ++----------------- 2 files changed, 17 insertions(+), 49 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 c0594c8b1967..59fcebbb2fe1 100644 --- a/nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java +++ b/nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java @@ -195,7 +195,7 @@ protected TableOperations newTableOps(TableIdentifier tableIdentifier) { ContentKey.of( org.projectnessie.model.Namespace.of(tableIdentifier.namespace().levels()), tr.getName()), - refreshedClient().withReference(tr.getReference(), tr.getHash()), + client().withReference(tr.getReference(), tr.getHash()), fileIO, catalogOptions); } @@ -224,13 +224,13 @@ protected String defaultWarehouseLocation(TableIdentifier table) { @Override public List listTables(Namespace namespace) { - return refreshedClient().listTables(namespace); + return client().listTables(namespace); } @Override public boolean dropTable(TableIdentifier identifier, boolean purge) { TableReference tableReference = parseTableReference(identifier); - return refreshedClient() + return client() .withReference(tableReference.getReference(), tableReference.getHash()) .dropTable(identifierWithoutTableReference(identifier, tableReference), purge); } @@ -239,7 +239,7 @@ public boolean dropTable(TableIdentifier identifier, boolean purge) { public void renameTable(TableIdentifier from, TableIdentifier to) { TableReference fromTableReference = parseTableReference(from); TableReference toTableReference = parseTableReference(to); - NessieIcebergClient nessieIcebergClient = refreshedClient(); + NessieIcebergClient nessieIcebergClient = client(); String fromReference = fromTableReference.hasReference() ? fromTableReference.getReference() @@ -264,12 +264,12 @@ public void renameTable(TableIdentifier from, TableIdentifier to) { @Override public void createNamespace(Namespace namespace, Map metadata) { - refreshedClient().createNamespace(namespace, metadata); + client().createNamespace(namespace, metadata); } @Override public List listNamespaces(Namespace namespace) throws NoSuchNamespaceException { - return refreshedClient().listNamespaces(namespace); + return client().listNamespaces(namespace); } /** @@ -282,22 +282,22 @@ public List listNamespaces(Namespace namespace) throws NoSuchNamespac @Override public Map loadNamespaceMetadata(Namespace namespace) throws NoSuchNamespaceException { - return refreshedClient().loadNamespaceMetadata(namespace); + return client().loadNamespaceMetadata(namespace); } @Override public boolean dropNamespace(Namespace namespace) throws NamespaceNotEmptyException { - return refreshedClient().dropNamespace(namespace); + return client().dropNamespace(namespace); } @Override public boolean setProperties(Namespace namespace, Map properties) { - return refreshedClient().setProperties(namespace, properties); + return client().setProperties(namespace, properties); } @Override public boolean removeProperties(Namespace namespace, Set properties) { - return refreshedClient().removeProperties(namespace, properties); + return client().removeProperties(namespace, properties); } @Override @@ -312,12 +312,12 @@ public Configuration getConf() { @VisibleForTesting String currentHash() { - return refreshedClient().getRef().getHash(); + return client().getRef().getHash(); } @VisibleForTesting String currentRefName() { - return refreshedClient().getRef().getName(); + return client().getRef().getName(); } @VisibleForTesting @@ -325,7 +325,7 @@ FileIO fileIO() { return fileIO; } - private NessieIcebergClient refreshedClient() { + private NessieIcebergClient client() { try { client.refresh(); } catch (NessieNotFoundException e) { diff --git a/nessie/src/test/java/org/apache/iceberg/nessie/TestMultipleClients.java b/nessie/src/test/java/org/apache/iceberg/nessie/TestMultipleClients.java index c3f3673ecd2d..8da96c224d51 100644 --- a/nessie/src/test/java/org/apache/iceberg/nessie/TestMultipleClients.java +++ b/nessie/src/test/java/org/apache/iceberg/nessie/TestMultipleClients.java @@ -20,12 +20,10 @@ import static org.apache.iceberg.types.Types.NestedField.required; -import java.io.File; import java.io.IOException; import java.net.URI; import java.util.AbstractMap; import java.util.Collections; -import org.apache.commons.io.FileUtils; import org.apache.iceberg.Schema; import org.apache.iceberg.Table; import org.apache.iceberg.TableMetadata; @@ -63,13 +61,8 @@ public void beforeEach(NessieClientFactory clientFactory, @NessieClientUri URI n anotherCatalog = initCatalog(branch); } - @Override @AfterEach public void afterEach() throws Exception { - dropTables(catalog); - dropTables(anotherCatalog); - - super.afterEach(); anotherCatalog.close(); } @@ -145,48 +138,23 @@ public void testConcurrentCommitsWithRefresh() { String hashBefore = catalog.currentHash(); TableOperations ops1 = catalog.newTableOps(identifier); - TableMetadata current1 = + TableMetadata metadata1 = TableMetadata.buildFrom(ops1.current()).setProperties(ImmutableMap.of("k1", "v1")).build(); // commit should succeed TableOperations ops2 = catalog.newTableOps(identifier); - TableMetadata current2 = + TableMetadata metadata2 = TableMetadata.buildFrom(ops2.current()).setProperties(ImmutableMap.of("k2", "v2")).build(); - ops2.commit(ops2.current(), current2); + ops2.commit(ops2.current(), metadata2); // refresh the catalog's client. String hashAfter = catalog.currentHash(); Assertions.assertThat(hashBefore).isNotEqualTo(hashAfter); // client refresh should not affect the ongoing commits (commit should still fail due staleness) - Assertions.assertThatThrownBy(() -> ops1.commit(ops1.current(), current1)) + Assertions.assertThatThrownBy(() -> ops1.commit(ops1.current(), metadata1)) .isInstanceOf(CommitFailedException.class) .hasMessageContaining( "Cannot commit: Reference hash is out of date. Update the reference 'multiple-clients-test' and try again"); } - - private static void dropTables(NessieCatalog nessieCatalog) { - nessieCatalog - .listNamespaces() - .forEach( - namespace -> - nessieCatalog - .listTables(namespace) - .forEach(identifier -> dropTable(nessieCatalog, identifier))); - } - - private static void dropTable(NessieCatalog nessieCatalog, TableIdentifier identifier) { - Table table = nessieCatalog.loadTable(identifier); - File tableLocation = tableLocation(table); - nessieCatalog.dropTable(identifier, false); - try { - FileUtils.deleteDirectory(tableLocation); - } catch (IOException e) { - throw new RuntimeException(e); - } - } - - private static File tableLocation(Table table) { - return new File(table.location().replaceFirst("file:", "")); - } } From e62c214b22d713dcc7ed6b234ee0c883a767982e Mon Sep 17 00:00:00 2001 From: ajantha-bhat Date: Mon, 13 Mar 2023 18:56:06 +0530 Subject: [PATCH 5/7] Avoid hash roundtrip --- .../apache/iceberg/nessie/NessieCatalog.java | 39 +++++-------- .../iceberg/nessie/NessieIcebergClient.java | 56 ++++++++++++------- .../iceberg/nessie/UpdateableReference.java | 11 ++++ 3 files changed, 61 insertions(+), 45 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 59fcebbb2fe1..37755f5123b2 100644 --- a/nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java +++ b/nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java @@ -47,7 +47,6 @@ import org.projectnessie.client.NessieConfigConstants; import org.projectnessie.client.api.NessieApiV1; import org.projectnessie.client.http.HttpClientBuilder; -import org.projectnessie.error.NessieNotFoundException; import org.projectnessie.model.ContentKey; import org.projectnessie.model.TableReference; import org.slf4j.Logger; @@ -195,7 +194,7 @@ protected TableOperations newTableOps(TableIdentifier tableIdentifier) { ContentKey.of( org.projectnessie.model.Namespace.of(tableIdentifier.namespace().levels()), tr.getName()), - client().withReference(tr.getReference(), tr.getHash()), + client.withReference(tr.getReference(), tr.getHash()), fileIO, catalogOptions); } @@ -224,13 +223,13 @@ protected String defaultWarehouseLocation(TableIdentifier table) { @Override public List listTables(Namespace namespace) { - return client().listTables(namespace); + return client.listTables(namespace); } @Override public boolean dropTable(TableIdentifier identifier, boolean purge) { TableReference tableReference = parseTableReference(identifier); - return client() + return client .withReference(tableReference.getReference(), tableReference.getHash()) .dropTable(identifierWithoutTableReference(identifier, tableReference), purge); } @@ -239,22 +238,21 @@ public boolean dropTable(TableIdentifier identifier, boolean purge) { public void renameTable(TableIdentifier from, TableIdentifier to) { TableReference fromTableReference = parseTableReference(from); TableReference toTableReference = parseTableReference(to); - NessieIcebergClient nessieIcebergClient = client(); String fromReference = fromTableReference.hasReference() ? fromTableReference.getReference() - : nessieIcebergClient.getRef().getName(); + : client.getRef().getName(); String toReference = toTableReference.hasReference() ? toTableReference.getReference() - : nessieIcebergClient.getRef().getName(); + : client.getRef().getName(); Preconditions.checkArgument( fromReference.equalsIgnoreCase(toReference), "from: %s and to: %s reference name must be same", fromReference, toReference); - nessieIcebergClient + client .withReference(fromTableReference.getReference(), fromTableReference.getHash()) .renameTable( identifierWithoutTableReference(from, fromTableReference), @@ -264,12 +262,12 @@ public void renameTable(TableIdentifier from, TableIdentifier to) { @Override public void createNamespace(Namespace namespace, Map metadata) { - client().createNamespace(namespace, metadata); + client.createNamespace(namespace, metadata); } @Override public List listNamespaces(Namespace namespace) throws NoSuchNamespaceException { - return client().listNamespaces(namespace); + return client.listNamespaces(namespace); } /** @@ -282,22 +280,22 @@ public List listNamespaces(Namespace namespace) throws NoSuchNamespac @Override public Map loadNamespaceMetadata(Namespace namespace) throws NoSuchNamespaceException { - return client().loadNamespaceMetadata(namespace); + return client.loadNamespaceMetadata(namespace); } @Override public boolean dropNamespace(Namespace namespace) throws NamespaceNotEmptyException { - return client().dropNamespace(namespace); + return client.dropNamespace(namespace); } @Override public boolean setProperties(Namespace namespace, Map properties) { - return client().setProperties(namespace, properties); + return client.setProperties(namespace, properties); } @Override public boolean removeProperties(Namespace namespace, Set properties) { - return client().removeProperties(namespace, properties); + return client.removeProperties(namespace, properties); } @Override @@ -312,12 +310,12 @@ public Configuration getConf() { @VisibleForTesting String currentHash() { - return client().getRef().getHash(); + return client.getRef().getHash(); } @VisibleForTesting String currentRefName() { - return client().getRef().getName(); + return client.getRef().getName(); } @VisibleForTesting @@ -325,15 +323,6 @@ FileIO fileIO() { return fileIO; } - private NessieIcebergClient client() { - try { - client.refresh(); - } catch (NessieNotFoundException e) { - throw new RuntimeException(e); - } - return client; - } - private TableReference parseTableReference(TableIdentifier tableIdentifier) { TableReference tr = TableReference.parse(tableIdentifier.name()); Preconditions.checkArgument( 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 596f72bb592d..0df3a16a6661 100644 --- a/nessie/src/main/java/org/apache/iceberg/nessie/NessieIcebergClient.java +++ b/nessie/src/main/java/org/apache/iceberg/nessie/NessieIcebergClient.java @@ -132,7 +132,8 @@ private UpdateableReference loadReference(String requestedRef, String hash) { public List listTables(Namespace namespace) { try { - return api.getEntries().reference(getRef().getReference()).get().getEntries().stream() + return api.getEntries().reference(getRef().getReferenceForApiRequest()).get().getEntries() + .stream() .filter(namespacePredicate(namespace)) .filter(e -> Content.Type.ICEBERG_TABLE == e.getType()) .map(this::toIdentifier) @@ -165,10 +166,10 @@ private TableIdentifier toIdentifier(EntriesResponse.Entry entry) { return TableIdentifier.of(elements.toArray(new String[elements.size()])); } - public IcebergTable table(TableIdentifier tableIdentifier) { + public IcebergTable table(TableIdentifier tableIdentifier, Reference ref) { try { ContentKey key = NessieUtil.toKey(tableIdentifier); - Content table = api.getContent().key(key).reference(getRef().getReference()).get().get(key); + Content table = api.getContent().key(key).reference(ref).get().get(key); return table != null ? table.unwrap(IcebergTable.class).orElse(null) : null; } catch (NessieNotFoundException e) { return null; @@ -180,7 +181,7 @@ public void createNamespace(Namespace namespace, Map metadata) { getRef().checkMutable(); getApi() .createNamespace() - .reference(getRef().getReference()) + .reference(getRef().getReferenceForApiRequest()) .namespace(org.projectnessie.model.Namespace.of(namespace.levels())) .properties(metadata) .create(); @@ -201,7 +202,7 @@ public List listNamespaces(Namespace namespace) throws NoSuchNamespac GetNamespacesResponse response = getApi() .getMultipleNamespaces() - .reference(getRef().getReference()) + .reference(getRef().getReferenceForApiRequest()) .namespace(org.projectnessie.model.Namespace.of(namespace.levels())) .get(); return response.getNamespaces().stream() @@ -221,7 +222,7 @@ public boolean dropNamespace(Namespace namespace) throws NamespaceNotEmptyExcept getRef().checkMutable(); getApi() .deleteNamespace() - .reference(getRef().getReference()) + .reference(getRef().getReferenceForApiRequest()) .namespace(org.projectnessie.model.Namespace.of(namespace.levels())) .delete(); refresh(); @@ -246,7 +247,7 @@ public Map loadNamespaceMetadata(Namespace namespace) try { return getApi() .getNamespace() - .reference(getRef().getReference()) + .reference(getRef().getReferenceForApiRequest()) .namespace(org.projectnessie.model.Namespace.of(namespace.levels())) .get() .getProperties(); @@ -265,7 +266,7 @@ public boolean setProperties(Namespace namespace, Map properties try { getApi() .updateProperties() - .reference(getRef().getReference()) + .reference(getRef().getReferenceForApiRequest()) .namespace(org.projectnessie.model.Namespace.of(namespace.levels())) .updateProperties(properties) .update(); @@ -287,7 +288,7 @@ public boolean removeProperties(Namespace namespace, Set properties) { try { getApi() .updateProperties() - .reference(getRef().getReference()) + .reference(getRef().getReferenceForApiRequest()) .namespace(org.projectnessie.model.Namespace.of(namespace.levels())) .removeProperties(properties) .update(); @@ -307,12 +308,18 @@ namespace, getRef().getName()), public void renameTable(TableIdentifier from, TableIdentifier to) { getRef().checkMutable(); + try { + getRef().refresh(api); + } catch (NessieNotFoundException e) { + throw new RuntimeException(e); + } - IcebergTable existingFromTable = table(from); + UpdateableReference updateableReference = getRef(); + IcebergTable existingFromTable = table(from, updateableReference.getReference()); if (existingFromTable == null) { throw new NoSuchTableException("Table does not exist: %s", from.name()); } - IcebergTable existingToTable = table(to); + IcebergTable existingToTable = table(to, updateableReference.getReference()); if (existingToTable != null) { throw new AlreadyExistsException("Table already exists: %s", to.name()); } @@ -335,8 +342,8 @@ public void renameTable(TableIdentifier from, TableIdentifier to) { .onFailure((o, exception) -> refresh()) .run( ops -> { - Branch branch = ops.branch(getRef().getAsBranch()).commit(); - getRef().updateReference(branch); + Branch branch = ops.branch((Branch) updateableReference.getReference()).commit(); + updateableReference.updateReference(branch); }, BaseNessieClientServerException.class); } catch (NessieNotFoundException e) { @@ -350,7 +357,7 @@ public void renameTable(TableIdentifier from, TableIdentifier to) { throw new RuntimeException( String.format( "Cannot rename table '%s' to '%s': " + "ref '%s' no longer exists.", - from.name(), to.name(), getRef().getName()), + from.name(), to.name(), updateableReference.getName()), e); } catch (BaseNessieClientServerException e) { throw new CommitFailedException( @@ -374,8 +381,15 @@ public void renameTable(TableIdentifier from, TableIdentifier to) { public boolean dropTable(TableIdentifier identifier, boolean purge) { getRef().checkMutable(); + try { + getRef().refresh(api); + } catch (NessieNotFoundException e) { + throw new RuntimeException(e); + } + + UpdateableReference updateableReference = getRef(); - IcebergTable existingTable = table(identifier); + IcebergTable existingTable = table(identifier, updateableReference.getReference()); if (existingTable == null) { return false; } @@ -402,18 +416,20 @@ public boolean dropTable(TableIdentifier identifier, boolean purge) { .onFailure((o, exception) -> refresh()) .run( commitBuilder -> { - Branch branch = commitBuilder.branch(getRef().getAsBranch()).commit(); - getRef().updateReference(branch); + Branch branch = + commitBuilder.branch((Branch) updateableReference.getReference()).commit(); + updateableReference.updateReference(branch); }, BaseNessieClientServerException.class); threw = false; } catch (NessieConflictException e) { LOG.error( "Cannot drop table: failed after retry (update ref '{}' and retry)", - getRef().getName(), + updateableReference.getName(), e); } catch (NessieNotFoundException e) { - LOG.error("Cannot drop table: ref '{}' is no longer valid.", getRef().getName(), e); + LOG.error( + "Cannot drop table: ref '{}' is no longer valid.", updateableReference.getName(), e); } catch (BaseNessieClientServerException e) { LOG.error("Cannot drop table: unknown error", e); } @@ -431,7 +447,7 @@ public void commitTable( updateableReference.checkMutable(); - Branch current = updateableReference.getAsBranch(); + Branch current = (Branch) updateableReference.getReference(); Branch expectedHead = current; if (base != null) { String metadataCommitId = diff --git a/nessie/src/main/java/org/apache/iceberg/nessie/UpdateableReference.java b/nessie/src/main/java/org/apache/iceberg/nessie/UpdateableReference.java index 7e49457981bf..9a68b69a0ae5 100644 --- a/nessie/src/main/java/org/apache/iceberg/nessie/UpdateableReference.java +++ b/nessie/src/main/java/org/apache/iceberg/nessie/UpdateableReference.java @@ -60,6 +60,8 @@ public String getHash() { return reference.getHash(); } + /** @deprecated will be removed in 1.3.0 */ + @Deprecated public Branch getAsBranch() { if (!isBranch()) { throw new IllegalArgumentException("Reference is not a branch"); @@ -71,6 +73,15 @@ public Reference getReference() { return reference; } + public Reference getReferenceForApiRequest() { + if (!mutable) { + return reference; + } + // setting hash to null in client's request, + // So that server uses the latest commit hash for that branch. + return Branch.of(reference.getName(), null); + } + public void checkMutable() { Preconditions.checkArgument( mutable, "You can only mutate tables when using a branch without a hash or timestamp."); From 804bc28b94b31700e0c62cc5670f242d3986f90d Mon Sep 17 00:00:00 2001 From: ajantha-bhat Date: Thu, 16 Mar 2023 11:48:38 +0530 Subject: [PATCH 6/7] Address new comments --- .../iceberg/nessie/NessieIcebergClient.java | 91 ++++++++++++------- .../iceberg/nessie/UpdateableReference.java | 22 +---- 2 files changed, 60 insertions(+), 53 deletions(-) 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 0df3a16a6661..09077ab3ac87 100644 --- a/nessie/src/main/java/org/apache/iceberg/nessie/NessieIcebergClient.java +++ b/nessie/src/main/java/org/apache/iceberg/nessie/NessieIcebergClient.java @@ -39,7 +39,14 @@ import org.apache.iceberg.util.Tasks; import org.projectnessie.client.NessieConfigConstants; import org.projectnessie.client.api.CommitMultipleOperationsBuilder; +import org.projectnessie.client.api.CreateNamespaceBuilder; +import org.projectnessie.client.api.DeleteNamespaceBuilder; +import org.projectnessie.client.api.GetEntriesBuilder; +import org.projectnessie.client.api.GetMultipleNamespacesBuilder; +import org.projectnessie.client.api.GetNamespaceBuilder; import org.projectnessie.client.api.NessieApiV1; +import org.projectnessie.client.api.OnReferenceBuilder; +import org.projectnessie.client.api.UpdateNamespaceBuilder; import org.projectnessie.client.http.HttpClientException; import org.projectnessie.error.BaseNessieClientServerException; import org.projectnessie.error.NessieConflictException; @@ -132,8 +139,9 @@ private UpdateableReference loadReference(String requestedRef, String hash) { public List listTables(Namespace namespace) { try { - return api.getEntries().reference(getRef().getReferenceForApiRequest()).get().getEntries() - .stream() + GetEntriesBuilder getEntriesBuilder = api.getEntries(); + addReference(getEntriesBuilder); + return getEntriesBuilder.get().getEntries().stream() .filter(namespacePredicate(namespace)) .filter(e -> Content.Type.ICEBERG_TABLE == e.getType()) .map(this::toIdentifier) @@ -179,12 +187,13 @@ public IcebergTable table(TableIdentifier tableIdentifier, Reference ref) { public void createNamespace(Namespace namespace, Map metadata) { try { getRef().checkMutable(); - getApi() - .createNamespace() - .reference(getRef().getReferenceForApiRequest()) - .namespace(org.projectnessie.model.Namespace.of(namespace.levels())) - .properties(metadata) - .create(); + CreateNamespaceBuilder createNamespaceBuilder = + getApi() + .createNamespace() + .namespace(org.projectnessie.model.Namespace.of(namespace.levels())) + .properties(metadata); + addReference(createNamespaceBuilder); + createNamespaceBuilder.create(); refresh(); } catch (NessieNamespaceAlreadyExistsException e) { throw new AlreadyExistsException(e, "Namespace already exists: %s", namespace); @@ -199,12 +208,12 @@ namespace, getRef().getName()), public List listNamespaces(Namespace namespace) throws NoSuchNamespaceException { try { - GetNamespacesResponse response = + GetMultipleNamespacesBuilder getMultipleNamespacesBuilder = getApi() .getMultipleNamespaces() - .reference(getRef().getReferenceForApiRequest()) - .namespace(org.projectnessie.model.Namespace.of(namespace.levels())) - .get(); + .namespace(org.projectnessie.model.Namespace.of(namespace.levels())); + addReference(getMultipleNamespacesBuilder); + GetNamespacesResponse response = getMultipleNamespacesBuilder.get(); return response.getNamespaces().stream() .map(ns -> Namespace.of(ns.getElements().toArray(new String[0]))) .collect(Collectors.toList()); @@ -220,11 +229,12 @@ namespace, getRef().getName()), public boolean dropNamespace(Namespace namespace) throws NamespaceNotEmptyException { try { getRef().checkMutable(); - getApi() - .deleteNamespace() - .reference(getRef().getReferenceForApiRequest()) - .namespace(org.projectnessie.model.Namespace.of(namespace.levels())) - .delete(); + DeleteNamespaceBuilder deleteNamespaceBuilder = + getApi() + .deleteNamespace() + .namespace(org.projectnessie.model.Namespace.of(namespace.levels())); + addReference(deleteNamespaceBuilder); + deleteNamespaceBuilder.delete(); refresh(); return true; } catch (NessieNamespaceNotFoundException e) { @@ -245,12 +255,12 @@ public boolean dropNamespace(Namespace namespace) throws NamespaceNotEmptyExcept public Map loadNamespaceMetadata(Namespace namespace) throws NoSuchNamespaceException { try { - return getApi() - .getNamespace() - .reference(getRef().getReferenceForApiRequest()) - .namespace(org.projectnessie.model.Namespace.of(namespace.levels())) - .get() - .getProperties(); + GetNamespaceBuilder getNamespaceBuilder = + getApi() + .getNamespace() + .namespace(org.projectnessie.model.Namespace.of(namespace.levels())); + addReference(getNamespaceBuilder); + return getNamespaceBuilder.get().getProperties(); } catch (NessieNamespaceNotFoundException e) { throw new NoSuchNamespaceException(e, "Namespace does not exist: %s", namespace); } catch (NessieReferenceNotFoundException e) { @@ -264,12 +274,13 @@ namespace, getRef().getName()), public boolean setProperties(Namespace namespace, Map properties) { try { - getApi() - .updateProperties() - .reference(getRef().getReferenceForApiRequest()) - .namespace(org.projectnessie.model.Namespace.of(namespace.levels())) - .updateProperties(properties) - .update(); + UpdateNamespaceBuilder updateNamespaceBuilder = + getApi() + .updateProperties() + .namespace(org.projectnessie.model.Namespace.of(namespace.levels())) + .updateProperties(properties); + addReference(updateNamespaceBuilder); + updateNamespaceBuilder.update(); refresh(); // always successful, otherwise an exception is thrown return true; @@ -286,12 +297,13 @@ namespace, getRef().getName()), public boolean removeProperties(Namespace namespace, Set properties) { try { - getApi() - .updateProperties() - .reference(getRef().getReferenceForApiRequest()) - .namespace(org.projectnessie.model.Namespace.of(namespace.levels())) - .removeProperties(properties) - .update(); + UpdateNamespaceBuilder updateNamespaceBuilder = + getApi() + .updateProperties() + .namespace(org.projectnessie.model.Namespace.of(namespace.levels())) + .removeProperties(properties); + addReference(updateNamespaceBuilder); + updateNamespaceBuilder.update(); refresh(); // always successful, otherwise an exception is thrown return true; @@ -507,6 +519,15 @@ private boolean isSnapshotOperation(TableMetadata base, TableMetadata metadata) || snapshot.snapshotId() != base.currentSnapshot().snapshotId()); } + private void addReference(OnReferenceBuilder builder) { + UpdateableReference ref = getRef(); + if (!ref.isMutable()) { + builder.reference(ref.getReference()); + } else { + builder.refName(ref.getName()); + } + } + private String buildCommitMsg(TableMetadata base, TableMetadata metadata, String tableName) { if (isSnapshotOperation(base, metadata)) { return String.format( diff --git a/nessie/src/main/java/org/apache/iceberg/nessie/UpdateableReference.java b/nessie/src/main/java/org/apache/iceberg/nessie/UpdateableReference.java index 9a68b69a0ae5..c133bea00bc7 100644 --- a/nessie/src/main/java/org/apache/iceberg/nessie/UpdateableReference.java +++ b/nessie/src/main/java/org/apache/iceberg/nessie/UpdateableReference.java @@ -60,28 +60,10 @@ public String getHash() { return reference.getHash(); } - /** @deprecated will be removed in 1.3.0 */ - @Deprecated - public Branch getAsBranch() { - if (!isBranch()) { - throw new IllegalArgumentException("Reference is not a branch"); - } - return (Branch) reference; - } - public Reference getReference() { return reference; } - public Reference getReferenceForApiRequest() { - if (!mutable) { - return reference; - } - // setting hash to null in client's request, - // So that server uses the latest commit hash for that branch. - return Branch.of(reference.getName(), null); - } - public void checkMutable() { Preconditions.checkArgument( mutable, "You can only mutate tables when using a branch without a hash or timestamp."); @@ -90,4 +72,8 @@ public void checkMutable() { public String getName() { return reference.getName(); } + + public boolean isMutable() { + return mutable; + } } From cc1c61cb3b7a54d2e43892a9d14d43923eb4025c Mon Sep 17 00:00:00 2001 From: ajantha-bhat Date: Thu, 16 Mar 2023 14:40:05 +0530 Subject: [PATCH 7/7] refactor --- .../iceberg/nessie/NessieIcebergClient.java | 126 +++++++----------- .../iceberg/nessie/UpdateableReference.java | 4 - 2 files changed, 50 insertions(+), 80 deletions(-) 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 09077ab3ac87..b006846566ba 100644 --- a/nessie/src/main/java/org/apache/iceberg/nessie/NessieIcebergClient.java +++ b/nessie/src/main/java/org/apache/iceberg/nessie/NessieIcebergClient.java @@ -39,14 +39,8 @@ import org.apache.iceberg.util.Tasks; import org.projectnessie.client.NessieConfigConstants; import org.projectnessie.client.api.CommitMultipleOperationsBuilder; -import org.projectnessie.client.api.CreateNamespaceBuilder; -import org.projectnessie.client.api.DeleteNamespaceBuilder; -import org.projectnessie.client.api.GetEntriesBuilder; -import org.projectnessie.client.api.GetMultipleNamespacesBuilder; -import org.projectnessie.client.api.GetNamespaceBuilder; import org.projectnessie.client.api.NessieApiV1; import org.projectnessie.client.api.OnReferenceBuilder; -import org.projectnessie.client.api.UpdateNamespaceBuilder; import org.projectnessie.client.http.HttpClientException; import org.projectnessie.error.BaseNessieClientServerException; import org.projectnessie.error.NessieConflictException; @@ -139,9 +133,7 @@ private UpdateableReference loadReference(String requestedRef, String hash) { public List listTables(Namespace namespace) { try { - GetEntriesBuilder getEntriesBuilder = api.getEntries(); - addReference(getEntriesBuilder); - return getEntriesBuilder.get().getEntries().stream() + return withReference(api.getEntries()).get().getEntries().stream() .filter(namespacePredicate(namespace)) .filter(e -> Content.Type.ICEBERG_TABLE == e.getType()) .map(this::toIdentifier) @@ -174,10 +166,10 @@ private TableIdentifier toIdentifier(EntriesResponse.Entry entry) { return TableIdentifier.of(elements.toArray(new String[elements.size()])); } - public IcebergTable table(TableIdentifier tableIdentifier, Reference ref) { + public IcebergTable table(TableIdentifier tableIdentifier) { try { ContentKey key = NessieUtil.toKey(tableIdentifier); - Content table = api.getContent().key(key).reference(ref).get().get(key); + 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; @@ -187,13 +179,12 @@ public IcebergTable table(TableIdentifier tableIdentifier, Reference ref) { public void createNamespace(Namespace namespace, Map metadata) { try { getRef().checkMutable(); - CreateNamespaceBuilder createNamespaceBuilder = - getApi() - .createNamespace() - .namespace(org.projectnessie.model.Namespace.of(namespace.levels())) - .properties(metadata); - addReference(createNamespaceBuilder); - createNamespaceBuilder.create(); + 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); @@ -208,12 +199,12 @@ namespace, getRef().getName()), public List listNamespaces(Namespace namespace) throws NoSuchNamespaceException { try { - GetMultipleNamespacesBuilder getMultipleNamespacesBuilder = - getApi() - .getMultipleNamespaces() - .namespace(org.projectnessie.model.Namespace.of(namespace.levels())); - addReference(getMultipleNamespacesBuilder); - GetNamespacesResponse response = getMultipleNamespacesBuilder.get(); + GetNamespacesResponse response = + withReference( + getApi() + .getMultipleNamespaces() + .namespace(org.projectnessie.model.Namespace.of(namespace.levels()))) + .get(); return response.getNamespaces().stream() .map(ns -> Namespace.of(ns.getElements().toArray(new String[0]))) .collect(Collectors.toList()); @@ -229,12 +220,11 @@ namespace, getRef().getName()), public boolean dropNamespace(Namespace namespace) throws NamespaceNotEmptyException { try { getRef().checkMutable(); - DeleteNamespaceBuilder deleteNamespaceBuilder = - getApi() - .deleteNamespace() - .namespace(org.projectnessie.model.Namespace.of(namespace.levels())); - addReference(deleteNamespaceBuilder); - deleteNamespaceBuilder.delete(); + withReference( + getApi() + .deleteNamespace() + .namespace(org.projectnessie.model.Namespace.of(namespace.levels()))) + .delete(); refresh(); return true; } catch (NessieNamespaceNotFoundException e) { @@ -255,12 +245,12 @@ public boolean dropNamespace(Namespace namespace) throws NamespaceNotEmptyExcept public Map loadNamespaceMetadata(Namespace namespace) throws NoSuchNamespaceException { try { - GetNamespaceBuilder getNamespaceBuilder = - getApi() - .getNamespace() - .namespace(org.projectnessie.model.Namespace.of(namespace.levels())); - addReference(getNamespaceBuilder); - return getNamespaceBuilder.get().getProperties(); + return withReference( + getApi() + .getNamespace() + .namespace(org.projectnessie.model.Namespace.of(namespace.levels()))) + .get() + .getProperties(); } catch (NessieNamespaceNotFoundException e) { throw new NoSuchNamespaceException(e, "Namespace does not exist: %s", namespace); } catch (NessieReferenceNotFoundException e) { @@ -274,13 +264,12 @@ namespace, getRef().getName()), public boolean setProperties(Namespace namespace, Map properties) { try { - UpdateNamespaceBuilder updateNamespaceBuilder = - getApi() - .updateProperties() - .namespace(org.projectnessie.model.Namespace.of(namespace.levels())) - .updateProperties(properties); - addReference(updateNamespaceBuilder); - updateNamespaceBuilder.update(); + withReference( + getApi() + .updateProperties() + .namespace(org.projectnessie.model.Namespace.of(namespace.levels())) + .updateProperties(properties)) + .update(); refresh(); // always successful, otherwise an exception is thrown return true; @@ -297,13 +286,12 @@ namespace, getRef().getName()), public boolean removeProperties(Namespace namespace, Set properties) { try { - UpdateNamespaceBuilder updateNamespaceBuilder = - getApi() - .updateProperties() - .namespace(org.projectnessie.model.Namespace.of(namespace.levels())) - .removeProperties(properties); - addReference(updateNamespaceBuilder); - updateNamespaceBuilder.update(); + withReference( + getApi() + .updateProperties() + .namespace(org.projectnessie.model.Namespace.of(namespace.levels())) + .removeProperties(properties)) + .update(); refresh(); // always successful, otherwise an exception is thrown return true; @@ -320,18 +308,12 @@ namespace, getRef().getName()), public void renameTable(TableIdentifier from, TableIdentifier to) { getRef().checkMutable(); - try { - getRef().refresh(api); - } catch (NessieNotFoundException e) { - throw new RuntimeException(e); - } - UpdateableReference updateableReference = getRef(); - IcebergTable existingFromTable = table(from, updateableReference.getReference()); + IcebergTable existingFromTable = table(from); if (existingFromTable == null) { throw new NoSuchTableException("Table does not exist: %s", from.name()); } - IcebergTable existingToTable = table(to, updateableReference.getReference()); + IcebergTable existingToTable = table(to); if (existingToTable != null) { throw new AlreadyExistsException("Table already exists: %s", to.name()); } @@ -354,8 +336,8 @@ public void renameTable(TableIdentifier from, TableIdentifier to) { .onFailure((o, exception) -> refresh()) .run( ops -> { - Branch branch = ops.branch((Branch) updateableReference.getReference()).commit(); - updateableReference.updateReference(branch); + Branch branch = ops.branch((Branch) getRef().getReference()).commit(); + getRef().updateReference(branch); }, BaseNessieClientServerException.class); } catch (NessieNotFoundException e) { @@ -369,7 +351,7 @@ public void renameTable(TableIdentifier from, TableIdentifier to) { throw new RuntimeException( String.format( "Cannot rename table '%s' to '%s': " + "ref '%s' no longer exists.", - from.name(), to.name(), updateableReference.getName()), + from.name(), to.name(), getRef().getName()), e); } catch (BaseNessieClientServerException e) { throw new CommitFailedException( @@ -393,15 +375,8 @@ public void renameTable(TableIdentifier from, TableIdentifier to) { public boolean dropTable(TableIdentifier identifier, boolean purge) { getRef().checkMutable(); - try { - getRef().refresh(api); - } catch (NessieNotFoundException e) { - throw new RuntimeException(e); - } - - UpdateableReference updateableReference = getRef(); - IcebergTable existingTable = table(identifier, updateableReference.getReference()); + IcebergTable existingTable = table(identifier); if (existingTable == null) { return false; } @@ -428,20 +403,18 @@ public boolean dropTable(TableIdentifier identifier, boolean purge) { .onFailure((o, exception) -> refresh()) .run( commitBuilder -> { - Branch branch = - commitBuilder.branch((Branch) updateableReference.getReference()).commit(); - updateableReference.updateReference(branch); + Branch branch = commitBuilder.branch((Branch) getRef().getReference()).commit(); + getRef().updateReference(branch); }, BaseNessieClientServerException.class); threw = false; } catch (NessieConflictException e) { LOG.error( "Cannot drop table: failed after retry (update ref '{}' and retry)", - updateableReference.getName(), + getRef().getName(), e); } catch (NessieNotFoundException e) { - LOG.error( - "Cannot drop table: ref '{}' is no longer valid.", updateableReference.getName(), e); + LOG.error("Cannot drop table: ref '{}' is no longer valid.", getRef().getName(), e); } catch (BaseNessieClientServerException e) { LOG.error("Cannot drop table: unknown error", e); } @@ -519,13 +492,14 @@ private boolean isSnapshotOperation(TableMetadata base, TableMetadata metadata) || snapshot.snapshotId() != base.currentSnapshot().snapshotId()); } - private void addReference(OnReferenceBuilder builder) { + private > T withReference(T builder) { UpdateableReference ref = getRef(); if (!ref.isMutable()) { builder.reference(ref.getReference()); } else { builder.refName(ref.getName()); } + return builder; } private String buildCommitMsg(TableMetadata base, TableMetadata metadata, String tableName) { diff --git a/nessie/src/main/java/org/apache/iceberg/nessie/UpdateableReference.java b/nessie/src/main/java/org/apache/iceberg/nessie/UpdateableReference.java index c133bea00bc7..28ef7fe7c22b 100644 --- a/nessie/src/main/java/org/apache/iceberg/nessie/UpdateableReference.java +++ b/nessie/src/main/java/org/apache/iceberg/nessie/UpdateableReference.java @@ -52,10 +52,6 @@ public void updateReference(Reference ref) { this.reference = Preconditions.checkNotNull(ref, "ref is null"); } - public boolean isBranch() { - return reference instanceof Branch; - } - public String getHash() { return reference.getHash(); }