You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "ajantha-bhat (via GitHub)" <gi...@apache.org> on 2023/01/31 17:32:22 UTC
[GitHub] [iceberg] ajantha-bhat opened a new pull request, #6712: [WIP] Nessie: Support ApiV2 for Nessie client
ajantha-bhat opened a new pull request, #6712:
URL: https://github.com/apache/iceberg/pull/6712
- The default is still kept as v1 as the v2 API cannot list implicit namespaces and need to provide a tool to convert implicit namespaces to explicit namespaces for an existing table if we make v2 as default.
- "client-api-version" catalog property is added to specify the API version
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ajantha-bhat commented on pull request #6712: Nessie: Support ApiV2 for Nessie client
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on PR #6712:
URL: https://github.com/apache/iceberg/pull/6712#issuecomment-1413808807
Thanks @dimas-b for the review.
@snazy : Do you have any suggestions for this PR?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ajantha-bhat commented on pull request #6712: Nessie: Support ApiV2 for Nessie client
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on PR #6712:
URL: https://github.com/apache/iceberg/pull/6712#issuecomment-1416063538
> Let’s wait with this one until Nessie‘s v2 API is out of beta and included in a Nessie release.
Rushing this likely causes more trouble Than necessary
ACK.
Moved to Draft considering V2 APIs cannot change much once we expose them to the users via iceberg release.
PR can wait till v2 to move out of "beta" status.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ajantha-bhat commented on a diff in pull request #6712: Nessie: Support ApiV2 for Nessie client
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #6712:
URL: https://github.com/apache/iceberg/pull/6712#discussion_r1199012120
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java:
##########
@@ -88,11 +89,24 @@ public void initialize(String name, Map<String, String> options) {
options.get(removePrefix.apply(NessieConfigConstants.CONF_NESSIE_REF));
String requestedHash =
options.get(removePrefix.apply(NessieConfigConstants.CONF_NESSIE_REF_HASH));
- NessieApiV1 api =
+
+ NessieClientBuilder<?> nessieClientBuilder =
createNessieClientBuilder(
options.get(NessieConfigConstants.CONF_NESSIE_CLIENT_BUILDER_IMPL))
- .fromConfig(x -> options.get(removePrefix.apply(x)))
- .build(NessieApiV1.class);
+ .fromConfig(x -> options.get(removePrefix.apply(x)));
+ final String apiVersion = options.get(NessieUtil.CLIENT_API_VERSION);
+ NessieApiV1 api;
+ if (apiVersion == null || apiVersion.equalsIgnoreCase("v2")) {
+ // default version is set to v2.
+ api = nessieClientBuilder.build(NessieApiV2.class);
Review Comment:
I assumed the newer Nessie servers will have to V2 URI by default. I will change it to V1
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ajantha-bhat commented on a diff in pull request #6712: Nessie: Support ApiV2 for Nessie client
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #6712:
URL: https://github.com/apache/iceberg/pull/6712#discussion_r1230843375
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNessieCatalog.java:
##########
@@ -115,17 +117,17 @@ private void resetData() throws NessieConflictException, NessieNotFoundException
private NessieCatalog initNessieCatalog(String ref) {
NessieCatalog newCatalog = new NessieCatalog();
newCatalog.setConf(hadoopConfig);
- newCatalog.initialize(
- "nessie",
+ ImmutableMap<String, String> options =
ImmutableMap.of(
"ref",
ref,
CatalogProperties.URI,
uri,
- "auth-type",
Review Comment:
the default value `NONE` itself. Hence, I just removed it to make less verbose.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ajantha-bhat commented on a diff in pull request #6712: [WIP] Nessie: Support ApiV2 for Nessie client
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #6712:
URL: https://github.com/apache/iceberg/pull/6712#discussion_r1092693947
##########
nessie/src/test/java/org/apache/iceberg/nessie/BaseTestIceberg.java:
##########
@@ -144,6 +144,9 @@ NessieCatalog initCatalog(String ref, String hash) {
.put(CatalogProperties.URI, uri)
.put("auth-type", "NONE")
.put(CatalogProperties.WAREHOUSE_LOCATION, temp.toUri().toString());
+ if (api instanceof NessieApiV2) {
Review Comment:
WFM. Since nothing was there. I used this way.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ajantha-bhat commented on a diff in pull request #6712: Nessie: Support ApiV2 for Nessie client
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #6712:
URL: https://github.com/apache/iceberg/pull/6712#discussion_r1094732867
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNamespace.java:
##########
@@ -73,6 +77,48 @@ public void testListNamespaces() {
Assertions.assertThat(namespaces).isNotNull().hasSize(2);
}
+ @Test
+ @NessieApiVersions(versions = NessieApiVersion.V1)
+ public void testNamespacesWithV1Api() {
+ createNamespacesAndTables();
+
+ testListTables();
+
+ List<Namespace> namespaces;
+ namespaces = catalog.listNamespaces(Namespace.of("foo"));
+ // NessieApiV1 should include both implicit and explicit namespaces
+ Assertions.assertThat(namespaces)
+ .containsExactlyInAnyOrder(Namespace.of("foo"), Namespace.of("foo", "bar"));
+ }
+
+ @Test
+ @NessieApiVersions(versions = NessieApiVersion.V2)
+ public void testNamespacesWithV2Api() {
Review Comment:
Originally had a single testcase with an ugly `instance of` a check. So, got [this](https://github.com/apache/iceberg/pull/6712#discussion_r1092350427) comment to separate it into a new testcase.
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNamespace.java:
##########
@@ -73,6 +77,48 @@ public void testListNamespaces() {
Assertions.assertThat(namespaces).isNotNull().hasSize(2);
}
+ @Test
+ @NessieApiVersions(versions = NessieApiVersion.V1)
+ public void testNamespacesWithV1Api() {
+ createNamespacesAndTables();
+
+ testListTables();
+
+ List<Namespace> namespaces;
+ namespaces = catalog.listNamespaces(Namespace.of("foo"));
+ // NessieApiV1 should include both implicit and explicit namespaces
+ Assertions.assertThat(namespaces)
+ .containsExactlyInAnyOrder(Namespace.of("foo"), Namespace.of("foo", "bar"));
+ }
+
+ @Test
+ @NessieApiVersions(versions = NessieApiVersion.V2)
+ public void testNamespacesWithV2Api() {
Review Comment:
Originally had a single testcase with an ugly `instance of` check. So, got [this](https://github.com/apache/iceberg/pull/6712#discussion_r1092350427) comment to separate it into a new testcase.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] dimas-b commented on a diff in pull request #6712: Nessie: Support ApiV2 for Nessie client
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #6712:
URL: https://github.com/apache/iceberg/pull/6712#discussion_r1093528350
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNamespace.java:
##########
@@ -73,6 +77,53 @@ public void testListNamespaces() {
Assertions.assertThat(namespaces).isNotNull().hasSize(2);
}
+ @Test
+ @NessieApiVersions(versions = NessieApiVersion.V1)
+ public void testNamespacesWithV1Api() {
+ createNamespacesAndTables();
+
+ testListTables();
+
+ List<Namespace> namespaces;
+ namespaces = catalog.listNamespaces(Namespace.of("foo"));
+ // NessieApiV1 should include both implicit and explicit namespaces
+ Assertions.assertThat(namespaces)
+ .isNotNull()
+ .hasSize(2)
+ .isEqualTo(Arrays.asList(Namespace.of("foo"), Namespace.of("foo", "bar")));
Review Comment:
Is the list order guaranteed? Why not `.containsExactlyInAnyOrder`?
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieUtil.java:
##########
@@ -37,6 +37,8 @@ public final class NessieUtil {
public static final String NESSIE_CONFIG_PREFIX = "nessie.";
static final String APPLICATION_TYPE = "application-type";
+ public static final String CLIENT_API_VERSION = "client-api-version";
Review Comment:
Not sure about that. Whether API v1 or v2 is used depends on what the client (e.g. `NessieCatalog`) expects and how its code is written. I think it is proper to have this config option defined on the catalog side.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] dimas-b commented on a diff in pull request #6712: Nessie: Support ApiV2 for Nessie client
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #6712:
URL: https://github.com/apache/iceberg/pull/6712#discussion_r1093525346
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieUtil.java:
##########
@@ -37,6 +37,8 @@ public final class NessieUtil {
public static final String NESSIE_CONFIG_PREFIX = "nessie.";
static final String APPLICATION_TYPE = "application-type";
+ public static final String CLIENT_API_VERSION = "client-api-version";
Review Comment:
> better to define in NessieConfigConstants on Nessie side [...]
Not sure about that. Whether API v1 or v2 is used depends on what the client (e.g. `NessieCatalog`) expects and how its code is written. I think it is proper to have this config option defined on the catalog side.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ajantha-bhat commented on a diff in pull request #6712: Nessie: Support ApiV2 for Nessie client
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #6712:
URL: https://github.com/apache/iceberg/pull/6712#discussion_r1094018996
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieUtil.java:
##########
@@ -37,6 +37,8 @@ public final class NessieUtil {
public static final String NESSIE_CONFIG_PREFIX = "nessie.";
static final String APPLICATION_TYPE = "application-type";
+ public static final String CLIENT_API_VERSION = "client-api-version";
Review Comment:
`nessie.client-builder-impl` is also a client's configuration. But we declare that constant variable in Nessie repo and use it here. So, I thought we can have it similarly.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] snazy commented on a diff in pull request #6712: Nessie: Support ApiV2 for Nessie client
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy commented on code in PR #6712:
URL: https://github.com/apache/iceberg/pull/6712#discussion_r1206314966
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java:
##########
@@ -88,11 +89,23 @@ public void initialize(String name, Map<String, String> options) {
options.get(removePrefix.apply(NessieConfigConstants.CONF_NESSIE_REF));
String requestedHash =
options.get(removePrefix.apply(NessieConfigConstants.CONF_NESSIE_REF_HASH));
- NessieApiV1 api =
+
+ NessieClientBuilder<?> nessieClientBuilder =
createNessieClientBuilder(
options.get(NessieConfigConstants.CONF_NESSIE_CLIENT_BUILDER_IMPL))
- .fromConfig(x -> options.get(removePrefix.apply(x)))
- .build(NessieApiV1.class);
+ .fromConfig(x -> options.get(removePrefix.apply(x)));
+ final String apiVersion = options.get(NessieUtil.CLIENT_API_VERSION);
Review Comment:
This should be prefixed like all other options IMO
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java:
##########
@@ -88,11 +89,23 @@ public void initialize(String name, Map<String, String> options) {
options.get(removePrefix.apply(NessieConfigConstants.CONF_NESSIE_REF));
String requestedHash =
options.get(removePrefix.apply(NessieConfigConstants.CONF_NESSIE_REF_HASH));
- NessieApiV1 api =
+
+ NessieClientBuilder<?> nessieClientBuilder =
createNessieClientBuilder(
options.get(NessieConfigConstants.CONF_NESSIE_CLIENT_BUILDER_IMPL))
- .fromConfig(x -> options.get(removePrefix.apply(x)))
- .build(NessieApiV1.class);
+ .fromConfig(x -> options.get(removePrefix.apply(x)));
+ final String apiVersion = options.get(NessieUtil.CLIENT_API_VERSION);
Review Comment:
Why not `getOrDefault(..., "1")?
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java:
##########
@@ -88,11 +89,23 @@ public void initialize(String name, Map<String, String> options) {
options.get(removePrefix.apply(NessieConfigConstants.CONF_NESSIE_REF));
String requestedHash =
options.get(removePrefix.apply(NessieConfigConstants.CONF_NESSIE_REF_HASH));
- NessieApiV1 api =
+
+ NessieClientBuilder<?> nessieClientBuilder =
createNessieClientBuilder(
options.get(NessieConfigConstants.CONF_NESSIE_CLIENT_BUILDER_IMPL))
- .fromConfig(x -> options.get(removePrefix.apply(x)))
- .build(NessieApiV1.class);
+ .fromConfig(x -> options.get(removePrefix.apply(x)));
+ final String apiVersion = options.get(NessieUtil.CLIENT_API_VERSION);
+ NessieApiV1 api;
+ if (apiVersion == null || apiVersion.equalsIgnoreCase("1")) {
Review Comment:
The `if`-cascade can be replaced with `switch`
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #6712: Nessie: Support ApiV2 for Nessie client
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #6712:
URL: https://github.com/apache/iceberg/pull/6712#discussion_r1230607887
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNessieCatalog.java:
##########
@@ -115,17 +117,16 @@ private void resetData() throws NessieConflictException, NessieNotFoundException
private NessieCatalog initNessieCatalog(String ref) {
NessieCatalog newCatalog = new NessieCatalog();
newCatalog.setConf(hadoopConfig);
- newCatalog.initialize(
- "nessie",
- ImmutableMap.of(
- "ref",
- ref,
- CatalogProperties.URI,
- uri,
- "auth-type",
- "NONE",
- CatalogProperties.WAREHOUSE_LOCATION,
- temp.toUri().toString()));
+ ImmutableMap.Builder<String, String> options =
+ ImmutableMap.<String, String>builder()
+ .put("ref", ref)
+ .put(CatalogProperties.URI, uri)
+ .put("auth-type", "NONE")
+ .put(CatalogProperties.WAREHOUSE_LOCATION, temp.toUri().toString());
+ if (apiVersion == NessieApiVersion.V2) {
Review Comment:
seems inconsistent with what's being done in other files where `client-api-version` is always added. Any particular reason to not just do `options.put("client-api-version", apiVersion == NessieApiVersion.V2 ? "2" : "1")`?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] dimas-b commented on a diff in pull request #6712: Nessie: Support ApiV2 for Nessie client
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #6712:
URL: https://github.com/apache/iceberg/pull/6712#discussion_r1198970876
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java:
##########
@@ -88,11 +89,24 @@ public void initialize(String name, Map<String, String> options) {
options.get(removePrefix.apply(NessieConfigConstants.CONF_NESSIE_REF));
String requestedHash =
options.get(removePrefix.apply(NessieConfigConstants.CONF_NESSIE_REF_HASH));
- NessieApiV1 api =
+
+ NessieClientBuilder<?> nessieClientBuilder =
createNessieClientBuilder(
options.get(NessieConfigConstants.CONF_NESSIE_CLIENT_BUILDER_IMPL))
- .fromConfig(x -> options.get(removePrefix.apply(x)))
- .build(NessieApiV1.class);
+ .fromConfig(x -> options.get(removePrefix.apply(x)));
+ final String apiVersion = options.get(NessieUtil.CLIENT_API_VERSION);
+ NessieApiV1 api;
+ if (apiVersion == null || apiVersion.equalsIgnoreCase("v2")) {
+ // default version is set to v2.
+ api = nessieClientBuilder.build(NessieApiV2.class);
Review Comment:
I'd still default to v1. Otherwise, users with existing jobs will get failures because of URI / API version mismatch and will have to debug failures and adjust their configs with urgency.
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieUtil.java:
##########
@@ -37,6 +37,8 @@ public final class NessieUtil {
public static final String NESSIE_CONFIG_PREFIX = "nessie.";
static final String APPLICATION_TYPE = "application-type";
+ public static final String CLIENT_API_VERSION = "client-api-version";
Review Comment:
Actually, it might be possible to have a constant for the API version on the Nessie Client side, but then the client should be choosing the class object for the `build(...)` call. This might be an option for a smooth migration path from API v1 to v2... but certainly it requires some Nessie-side changes.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ajantha-bhat commented on a diff in pull request #6712: [WIP] Nessie: Support ApiV2 for Nessie client
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #6712:
URL: https://github.com/apache/iceberg/pull/6712#discussion_r1092270108
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieUtil.java:
##########
@@ -37,6 +37,8 @@ public final class NessieUtil {
public static final String NESSIE_CONFIG_PREFIX = "nessie.";
static final String APPLICATION_TYPE = "application-type";
+ public static final String CLIENT_API_VERSION = "client-api-version";
Review Comment:
better to define in `NessieConfigConstants` on Nessie side. But that needs a Nessie release.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] dimas-b commented on a diff in pull request #6712: [WIP] Nessie: Support ApiV2 for Nessie client
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #6712:
URL: https://github.com/apache/iceberg/pull/6712#discussion_r1092350427
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNamespace.java:
##########
@@ -63,14 +65,56 @@ public void testListNamespaces() {
tables = catalog.listTables(null);
Assertions.assertThat(tables).isNotNull().hasSize(6);
- List<Namespace> namespaces = catalog.listNamespaces();
- Assertions.assertThat(namespaces).isNotNull().hasSize(5);
- namespaces = catalog.listNamespaces(Namespace.of("a"));
- Assertions.assertThat(namespaces).isNotNull().hasSize(3);
- namespaces = catalog.listNamespaces(Namespace.of("a", "b"));
- Assertions.assertThat(namespaces).isNotNull().hasSize(2);
- namespaces = catalog.listNamespaces(Namespace.of("b"));
- Assertions.assertThat(namespaces).isNotNull().hasSize(2);
+ List<Namespace> namespaces;
+ if (api instanceof NessieApiV2) {
Review Comment:
Would it be possible to use `@NessieApiVersions` test annotations to avoid this kind of `if` is test code?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] dimas-b commented on a diff in pull request #6712: [WIP] Nessie: Support ApiV2 for Nessie client
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #6712:
URL: https://github.com/apache/iceberg/pull/6712#discussion_r1092348487
##########
nessie/src/test/java/org/apache/iceberg/nessie/BaseTestIceberg.java:
##########
@@ -144,6 +144,9 @@ NessieCatalog initCatalog(String ref, String hash) {
.put(CatalogProperties.URI, uri)
.put("auth-type", "NONE")
.put(CatalogProperties.WAREHOUSE_LOCATION, temp.toUri().toString());
+ if (api instanceof NessieApiV2) {
Review Comment:
We should probably make the API version available via injected test parameters (needs a Nessie change), for example in `NessieClientFactory`, or as a separate injected parameter of type `NessieApiVersion`.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] dimas-b commented on a diff in pull request #6712: Nessie: Support ApiV2 for Nessie client
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #6712:
URL: https://github.com/apache/iceberg/pull/6712#discussion_r1199137305
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieUtil.java:
##########
@@ -37,6 +37,8 @@ public final class NessieUtil {
public static final String NESSIE_CONFIG_PREFIX = "nessie.";
static final String APPLICATION_TYPE = "application-type";
+ public static final String CLIENT_API_VERSION = "client-api-version";
Review Comment:
I think it is fine to keep this constant in this class for now.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ajantha-bhat commented on a diff in pull request #6712: Nessie: Support ApiV2 for Nessie client
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #6712:
URL: https://github.com/apache/iceberg/pull/6712#discussion_r1206608261
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java:
##########
@@ -88,11 +89,23 @@ public void initialize(String name, Map<String, String> options) {
options.get(removePrefix.apply(NessieConfigConstants.CONF_NESSIE_REF));
String requestedHash =
options.get(removePrefix.apply(NessieConfigConstants.CONF_NESSIE_REF_HASH));
- NessieApiV1 api =
+
+ NessieClientBuilder<?> nessieClientBuilder =
createNessieClientBuilder(
options.get(NessieConfigConstants.CONF_NESSIE_CLIENT_BUILDER_IMPL))
- .fromConfig(x -> options.get(removePrefix.apply(x)))
- .build(NessieApiV1.class);
+ .fromConfig(x -> options.get(removePrefix.apply(x)));
+ final String apiVersion = options.get(NessieUtil.CLIENT_API_VERSION);
Review Comment:
I have added the prefix for const declaration.
But Do you know why we don't remove the prefix for `CONF_NESSIE_CLIENT_BUILDER_IMPL`?
For `CLIENT_API_VERSION` I have removed the prefix like other properties.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ajantha-bhat commented on a diff in pull request #6712: Nessie: Support ApiV2 for Nessie client
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #6712:
URL: https://github.com/apache/iceberg/pull/6712#discussion_r1094732867
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNamespace.java:
##########
@@ -73,6 +77,48 @@ public void testListNamespaces() {
Assertions.assertThat(namespaces).isNotNull().hasSize(2);
}
+ @Test
+ @NessieApiVersions(versions = NessieApiVersion.V1)
+ public void testNamespacesWithV1Api() {
+ createNamespacesAndTables();
+
+ testListTables();
+
+ List<Namespace> namespaces;
+ namespaces = catalog.listNamespaces(Namespace.of("foo"));
+ // NessieApiV1 should include both implicit and explicit namespaces
+ Assertions.assertThat(namespaces)
+ .containsExactlyInAnyOrder(Namespace.of("foo"), Namespace.of("foo", "bar"));
+ }
+
+ @Test
+ @NessieApiVersions(versions = NessieApiVersion.V2)
+ public void testNamespacesWithV2Api() {
Review Comment:
Originally had a single testcase with an ugly instance of a check. So, got [this](https://github.com/apache/iceberg/pull/6712#discussion_r1092350427) comment to separate it into a new testcase.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] dimas-b commented on a diff in pull request #6712: Nessie: Support ApiV2 for Nessie client
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #6712:
URL: https://github.com/apache/iceberg/pull/6712#discussion_r1093346037
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNamespace.java:
##########
@@ -63,14 +65,56 @@ public void testListNamespaces() {
tables = catalog.listTables(null);
Assertions.assertThat(tables).isNotNull().hasSize(6);
- List<Namespace> namespaces = catalog.listNamespaces();
- Assertions.assertThat(namespaces).isNotNull().hasSize(5);
- namespaces = catalog.listNamespaces(Namespace.of("a"));
- Assertions.assertThat(namespaces).isNotNull().hasSize(3);
- namespaces = catalog.listNamespaces(Namespace.of("a", "b"));
- Assertions.assertThat(namespaces).isNotNull().hasSize(2);
- namespaces = catalog.listNamespaces(Namespace.of("b"));
- Assertions.assertThat(namespaces).isNotNull().hasSize(2);
+ List<Namespace> namespaces;
+ if (api instanceof NessieApiV2) {
Review Comment:
If there are many differences perhaps we could have a dedicated sub-class for each API version.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #6712: Nessie: Support ApiV2 for Nessie client
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #6712:
URL: https://github.com/apache/iceberg/pull/6712#discussion_r1229744332
##########
nessie/src/test/java/org/apache/iceberg/nessie/BaseTestIceberg.java:
##########
@@ -145,7 +147,8 @@ NessieCatalog initCatalog(String ref, String hash) {
.put("ref", ref)
.put(CatalogProperties.URI, uri)
.put("auth-type", "NONE")
- .put(CatalogProperties.WAREHOUSE_LOCATION, temp.toUri().toString());
+ .put(CatalogProperties.WAREHOUSE_LOCATION, temp.toUri().toString())
+ .put("client-api-version", apiVersion);
Review Comment:
should this be using the property from `NessieUtil`?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] dimas-b commented on a diff in pull request #6712: Nessie: Support ApiV2 for Nessie client
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #6712:
URL: https://github.com/apache/iceberg/pull/6712#discussion_r1199139215
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java:
##########
@@ -88,11 +89,24 @@ public void initialize(String name, Map<String, String> options) {
options.get(removePrefix.apply(NessieConfigConstants.CONF_NESSIE_REF));
String requestedHash =
options.get(removePrefix.apply(NessieConfigConstants.CONF_NESSIE_REF_HASH));
- NessieApiV1 api =
+
+ NessieClientBuilder<?> nessieClientBuilder =
createNessieClientBuilder(
options.get(NessieConfigConstants.CONF_NESSIE_CLIENT_BUILDER_IMPL))
- .fromConfig(x -> options.get(removePrefix.apply(x)))
- .build(NessieApiV1.class);
+ .fromConfig(x -> options.get(removePrefix.apply(x)));
+ final String apiVersion = options.get(NessieUtil.CLIENT_API_VERSION);
+ NessieApiV1 api;
+ if (apiVersion == null || apiVersion.equalsIgnoreCase("v1")) {
Review Comment:
I wonder if a simple `int` value is preferable. Nessie Client API version checks use simple ints, e.g. https://github.com/projectnessie/nessie/pull/6818
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ajantha-bhat commented on a diff in pull request #6712: Nessie: Support ApiV2 for Nessie client
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #6712:
URL: https://github.com/apache/iceberg/pull/6712#discussion_r1230740842
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNessieCatalog.java:
##########
@@ -115,17 +117,16 @@ private void resetData() throws NessieConflictException, NessieNotFoundException
private NessieCatalog initNessieCatalog(String ref) {
NessieCatalog newCatalog = new NessieCatalog();
newCatalog.setConf(hadoopConfig);
- newCatalog.initialize(
- "nessie",
- ImmutableMap.of(
- "ref",
- ref,
- CatalogProperties.URI,
- uri,
- "auth-type",
- "NONE",
- CatalogProperties.WAREHOUSE_LOCATION,
- temp.toUri().toString()));
+ ImmutableMap.Builder<String, String> options =
+ ImmutableMap.<String, String>builder()
+ .put("ref", ref)
+ .put(CatalogProperties.URI, uri)
+ .put("auth-type", "NONE")
+ .put(CatalogProperties.WAREHOUSE_LOCATION, temp.toUri().toString());
+ if (apiVersion == NessieApiVersion.V2) {
Review Comment:
updated it.
Else if the default value is changed in the future, I have to update the test case, If I won't add it for both versions.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #6712: Nessie: Support ApiV2 for Nessie client
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #6712:
URL: https://github.com/apache/iceberg/pull/6712#discussion_r1094775416
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java:
##########
@@ -103,7 +103,9 @@ public void initialize(String name, Map<String, String> options) {
api = nessieClientBuilder.build(NessieApiV2.class);
} else {
throw new IllegalArgumentException(
- "Unsupported client-api-version: " + apiVersion + ". Can only be v1 or v2");
+ String.format(
+ "Unsupported %s: " + apiVersion + ". Can only be v1 or v2",
Review Comment:
I think it would be good to add a test for this
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #6712: Nessie: Support ApiV2 for Nessie client
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #6712:
URL: https://github.com/apache/iceberg/pull/6712#discussion_r1094703306
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java:
##########
@@ -88,11 +89,22 @@ public void initialize(String name, Map<String, String> options) {
options.get(removePrefix.apply(NessieConfigConstants.CONF_NESSIE_REF));
String requestedHash =
options.get(removePrefix.apply(NessieConfigConstants.CONF_NESSIE_REF_HASH));
- NessieApiV1 api =
+
+ NessieClientBuilder<?> nessieClientBuilder =
createNessieClientBuilder(
options.get(NessieConfigConstants.CONF_NESSIE_CLIENT_BUILDER_IMPL))
- .fromConfig(x -> options.get(removePrefix.apply(x)))
- .build(NessieApiV1.class);
+ .fromConfig(x -> options.get(removePrefix.apply(x)));
+ final String apiVersion = options.get(NessieUtil.CLIENT_API_VERSION);
+ NessieApiV1 api;
+ if (apiVersion == null || apiVersion.equalsIgnoreCase("v1")) {
+ // default version is set to v1.
+ api = nessieClientBuilder.build(NessieApiV1.class);
+ } else if (apiVersion.equalsIgnoreCase("v2")) {
+ api = nessieClientBuilder.build(NessieApiV2.class);
+ } else {
+ throw new IllegalArgumentException(
+ "Unsupported client-api-version: " + apiVersion + ". Can only be v1 or v2");
Review Comment:
should this be using whatever `NessieUtil.CLIENT_API_VERSION` is set to?
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNamespace.java:
##########
@@ -73,6 +77,48 @@ public void testListNamespaces() {
Assertions.assertThat(namespaces).isNotNull().hasSize(2);
}
+ @Test
+ @NessieApiVersions(versions = NessieApiVersion.V1)
+ public void testNamespacesWithV1Api() {
+ createNamespacesAndTables();
+
+ testListTables();
+
+ List<Namespace> namespaces;
+ namespaces = catalog.listNamespaces(Namespace.of("foo"));
+ // NessieApiV1 should include both implicit and explicit namespaces
+ Assertions.assertThat(namespaces)
+ .containsExactlyInAnyOrder(Namespace.of("foo"), Namespace.of("foo", "bar"));
+ }
+
+ @Test
+ @NessieApiVersions(versions = NessieApiVersion.V2)
+ public void testNamespacesWithV2Api() {
Review Comment:
could we have a single test and dynamically pick a different assertion based on the given API version? I think that would be better than having two separate tests
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNamespace.java:
##########
@@ -73,6 +77,48 @@ public void testListNamespaces() {
Assertions.assertThat(namespaces).isNotNull().hasSize(2);
}
+ @Test
+ @NessieApiVersions(versions = NessieApiVersion.V1)
+ public void testNamespacesWithV1Api() {
+ createNamespacesAndTables();
+
+ testListTables();
+
+ List<Namespace> namespaces;
+ namespaces = catalog.listNamespaces(Namespace.of("foo"));
+ // NessieApiV1 should include both implicit and explicit namespaces
+ Assertions.assertThat(namespaces)
+ .containsExactlyInAnyOrder(Namespace.of("foo"), Namespace.of("foo", "bar"));
+ }
+
+ @Test
+ @NessieApiVersions(versions = NessieApiVersion.V2)
+ public void testNamespacesWithV2Api() {
+ createNamespacesAndTables();
+
+ testListTables();
+
+ List<Namespace> namespaces;
+ namespaces = catalog.listNamespaces(Namespace.of("foo"));
+ // NessieApiV2 will not list the implicit namespaces
+ Assertions.assertThat(namespaces).isEqualTo(Collections.singletonList(Namespace.of("foo")));
+ }
+
+ private void testListTables() {
Review Comment:
maybe rather name it `verifyListingTables` or something along those lines
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] dimas-b commented on a diff in pull request #6712: [WIP] Nessie: Support ApiV2 for Nessie client
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #6712:
URL: https://github.com/apache/iceberg/pull/6712#discussion_r1092348487
##########
nessie/src/test/java/org/apache/iceberg/nessie/BaseTestIceberg.java:
##########
@@ -144,6 +144,9 @@ NessieCatalog initCatalog(String ref, String hash) {
.put(CatalogProperties.URI, uri)
.put("auth-type", "NONE")
.put(CatalogProperties.WAREHOUSE_LOCATION, temp.toUri().toString());
+ if (api instanceof NessieApiV2) {
Review Comment:
We should probably make the API version available via injected test parameters (needs a Nessie change), for example in `NessieClientFactory`
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ajantha-bhat commented on pull request #6712: [WIP] Nessie: Support ApiV2 for Nessie client
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on PR #6712:
URL: https://github.com/apache/iceberg/pull/6712#issuecomment-1411455250
cc: @snazy, @dimas-b
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] dimas-b commented on a diff in pull request #6712: Nessie: Support ApiV2 for Nessie client
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #6712:
URL: https://github.com/apache/iceberg/pull/6712#discussion_r1094020382
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNamespace.java:
##########
@@ -73,6 +77,53 @@ public void testListNamespaces() {
Assertions.assertThat(namespaces).isNotNull().hasSize(2);
}
+ @Test
+ @NessieApiVersions(versions = NessieApiVersion.V1)
+ public void testNamespacesWithV1Api() {
+ createNamespacesAndTables();
+
+ testListTables();
+
+ List<Namespace> namespaces;
+ namespaces = catalog.listNamespaces(Namespace.of("foo"));
+ // NessieApiV1 should include both implicit and explicit namespaces
+ Assertions.assertThat(namespaces)
+ .isNotNull()
+ .hasSize(2)
+ .containsExactlyInAnyOrder(Namespace.of("foo"), Namespace.of("foo", "bar"));
+ }
+
+ @Test
+ @NessieApiVersions(versions = NessieApiVersion.V2)
+ public void testNamespacesWithV2Api() {
+ createNamespacesAndTables();
+
+ testListTables();
+
+ List<Namespace> namespaces;
+ namespaces = catalog.listNamespaces(Namespace.of("foo"));
+ // NessieApiV2 will not list the implicit namespaces
+ Assertions.assertThat(namespaces)
+ .isNotNull()
+ .hasSize(1)
Review Comment:
nit: `hasSize` is redundant with `isEqualTo` (below)
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNamespace.java:
##########
@@ -73,6 +77,53 @@ public void testListNamespaces() {
Assertions.assertThat(namespaces).isNotNull().hasSize(2);
}
+ @Test
+ @NessieApiVersions(versions = NessieApiVersion.V1)
+ public void testNamespacesWithV1Api() {
+ createNamespacesAndTables();
+
+ testListTables();
+
+ List<Namespace> namespaces;
+ namespaces = catalog.listNamespaces(Namespace.of("foo"));
+ // NessieApiV1 should include both implicit and explicit namespaces
+ Assertions.assertThat(namespaces)
+ .isNotNull()
+ .hasSize(2)
Review Comment:
nit: `hasSize` is redundant with `containsExactlyInAnyOrder` (below)
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ajantha-bhat commented on a diff in pull request #6712: Nessie: Support ApiV2 for Nessie client
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #6712:
URL: https://github.com/apache/iceberg/pull/6712#discussion_r1198999468
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieUtil.java:
##########
@@ -37,6 +37,8 @@ public final class NessieUtil {
public static final String NESSIE_CONFIG_PREFIX = "nessie.";
static final String APPLICATION_TYPE = "application-type";
+ public static final String CLIENT_API_VERSION = "client-api-version";
Review Comment:
Yes, maybe in the later versions?
Since nothing was there. I used this way.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ajantha-bhat commented on a diff in pull request #6712: Nessie: Support ApiV2 for Nessie client
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #6712:
URL: https://github.com/apache/iceberg/pull/6712#discussion_r1199029436
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java:
##########
@@ -88,11 +89,24 @@ public void initialize(String name, Map<String, String> options) {
options.get(removePrefix.apply(NessieConfigConstants.CONF_NESSIE_REF));
String requestedHash =
options.get(removePrefix.apply(NessieConfigConstants.CONF_NESSIE_REF_HASH));
- NessieApiV1 api =
+
+ NessieClientBuilder<?> nessieClientBuilder =
createNessieClientBuilder(
options.get(NessieConfigConstants.CONF_NESSIE_CLIENT_BUILDER_IMPL))
- .fromConfig(x -> options.get(removePrefix.apply(x)))
- .build(NessieApiV1.class);
+ .fromConfig(x -> options.get(removePrefix.apply(x)));
+ final String apiVersion = options.get(NessieUtil.CLIENT_API_VERSION);
+ NessieApiV1 api;
+ if (apiVersion == null || apiVersion.equalsIgnoreCase("v2")) {
+ // default version is set to v2.
+ api = nessieClientBuilder.build(NessieApiV2.class);
Review Comment:
Reverted changing the default.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ajantha-bhat commented on a diff in pull request #6712: [WIP] Nessie: Support ApiV2 for Nessie client
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #6712:
URL: https://github.com/apache/iceberg/pull/6712#discussion_r1092693475
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNamespace.java:
##########
@@ -63,14 +65,56 @@ public void testListNamespaces() {
tables = catalog.listTables(null);
Assertions.assertThat(tables).isNotNull().hasSize(6);
- List<Namespace> namespaces = catalog.listNamespaces();
- Assertions.assertThat(namespaces).isNotNull().hasSize(5);
- namespaces = catalog.listNamespaces(Namespace.of("a"));
- Assertions.assertThat(namespaces).isNotNull().hasSize(3);
- namespaces = catalog.listNamespaces(Namespace.of("a", "b"));
- Assertions.assertThat(namespaces).isNotNull().hasSize(2);
- namespaces = catalog.listNamespaces(Namespace.of("b"));
- Assertions.assertThat(namespaces).isNotNull().hasSize(2);
+ List<Namespace> namespaces;
+ if (api instanceof NessieApiV2) {
Review Comment:
This class extends `BaseTestIceberg` which uses `@NessieApiVersions`. Here, If the test case run is for v2 I want to do these checks. Do you mean there is some other way to find out whether this testcase is running for v2? Can you please point me to it in Nessie's side testcases?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ajantha-bhat commented on pull request #6712: Nessie: Support ApiV2 for Nessie client
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on PR #6712:
URL: https://github.com/apache/iceberg/pull/6712#issuecomment-1591140971
I just rebased the PR (as it was stale form a few weeks)
@snazy: The previous comments are addressed. Can you please take a look at this again? Thanks.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ajantha-bhat commented on a diff in pull request #6712: Nessie: Support ApiV2 for Nessie client
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #6712:
URL: https://github.com/apache/iceberg/pull/6712#discussion_r1093457847
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNamespace.java:
##########
@@ -63,14 +65,56 @@ public void testListNamespaces() {
tables = catalog.listTables(null);
Assertions.assertThat(tables).isNotNull().hasSize(6);
- List<Namespace> namespaces = catalog.listNamespaces();
- Assertions.assertThat(namespaces).isNotNull().hasSize(5);
- namespaces = catalog.listNamespaces(Namespace.of("a"));
- Assertions.assertThat(namespaces).isNotNull().hasSize(3);
- namespaces = catalog.listNamespaces(Namespace.of("a", "b"));
- Assertions.assertThat(namespaces).isNotNull().hasSize(2);
- namespaces = catalog.listNamespaces(Namespace.of("b"));
- Assertions.assertThat(namespaces).isNotNull().hasSize(2);
+ List<Namespace> namespaces;
+ if (api instanceof NessieApiV2) {
Review Comment:
Done. Used `@NessieApiVersions(...)` now with separate test cases.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] dimas-b commented on a diff in pull request #6712: Nessie: Support ApiV2 for Nessie client
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #6712:
URL: https://github.com/apache/iceberg/pull/6712#discussion_r1093347940
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNamespace.java:
##########
@@ -63,14 +65,56 @@ public void testListNamespaces() {
tables = catalog.listTables(null);
Assertions.assertThat(tables).isNotNull().hasSize(6);
- List<Namespace> namespaces = catalog.listNamespaces();
- Assertions.assertThat(namespaces).isNotNull().hasSize(5);
- namespaces = catalog.listNamespaces(Namespace.of("a"));
- Assertions.assertThat(namespaces).isNotNull().hasSize(3);
- namespaces = catalog.listNamespaces(Namespace.of("a", "b"));
- Assertions.assertThat(namespaces).isNotNull().hasSize(2);
- namespaces = catalog.listNamespaces(Namespace.of("b"));
- Assertions.assertThat(namespaces).isNotNull().hasSize(2);
+ List<Namespace> namespaces;
+ if (api instanceof NessieApiV2) {
Review Comment:
IMHO, `if`s in the test code complicate understanding of what the test actually covers.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ajantha-bhat commented on a diff in pull request #6712: Nessie: Support ApiV2 for Nessie client
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #6712:
URL: https://github.com/apache/iceberg/pull/6712#discussion_r1094019269
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNamespace.java:
##########
@@ -73,6 +77,53 @@ public void testListNamespaces() {
Assertions.assertThat(namespaces).isNotNull().hasSize(2);
}
+ @Test
+ @NessieApiVersions(versions = NessieApiVersion.V1)
+ public void testNamespacesWithV1Api() {
+ createNamespacesAndTables();
+
+ testListTables();
+
+ List<Namespace> namespaces;
+ namespaces = catalog.listNamespaces(Namespace.of("foo"));
+ // NessieApiV1 should include both implicit and explicit namespaces
+ Assertions.assertThat(namespaces)
+ .isNotNull()
+ .hasSize(2)
+ .isEqualTo(Arrays.asList(Namespace.of("foo"), Namespace.of("foo", "bar")));
Review Comment:
done.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] dimas-b commented on a diff in pull request #6712: Nessie: Support ApiV2 for Nessie client
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #6712:
URL: https://github.com/apache/iceberg/pull/6712#discussion_r1094033556
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNamespace.java:
##########
@@ -73,6 +77,51 @@ public void testListNamespaces() {
Assertions.assertThat(namespaces).isNotNull().hasSize(2);
}
+ @Test
+ @NessieApiVersions(versions = NessieApiVersion.V1)
+ public void testNamespacesWithV1Api() {
+ createNamespacesAndTables();
+
+ testListTables();
+
+ List<Namespace> namespaces;
+ namespaces = catalog.listNamespaces(Namespace.of("foo"));
+ // NessieApiV1 should include both implicit and explicit namespaces
+ Assertions.assertThat(namespaces)
+ .isNotNull()
Review Comment:
nit: `isNotNull` is also redundant :) Sorry, I missed this in my previous comment.
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNamespace.java:
##########
@@ -73,6 +77,51 @@ public void testListNamespaces() {
Assertions.assertThat(namespaces).isNotNull().hasSize(2);
}
+ @Test
+ @NessieApiVersions(versions = NessieApiVersion.V1)
+ public void testNamespacesWithV1Api() {
+ createNamespacesAndTables();
+
+ testListTables();
+
+ List<Namespace> namespaces;
+ namespaces = catalog.listNamespaces(Namespace.of("foo"));
+ // NessieApiV1 should include both implicit and explicit namespaces
+ Assertions.assertThat(namespaces)
+ .isNotNull()
+ .containsExactlyInAnyOrder(Namespace.of("foo"), Namespace.of("foo", "bar"));
+ }
+
+ @Test
+ @NessieApiVersions(versions = NessieApiVersion.V2)
+ public void testNamespacesWithV2Api() {
+ createNamespacesAndTables();
+
+ testListTables();
+
+ List<Namespace> namespaces;
+ namespaces = catalog.listNamespaces(Namespace.of("foo"));
+ // NessieApiV2 will not list the implicit namespaces
+ Assertions.assertThat(namespaces)
+ .isNotNull()
Review Comment:
nit: `isNotNull` is redunadant
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] dimas-b commented on a diff in pull request #6712: Nessie: Support ApiV2 for Nessie client
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #6712:
URL: https://github.com/apache/iceberg/pull/6712#discussion_r1094829567
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNamespace.java:
##########
@@ -73,6 +77,48 @@ public void testListNamespaces() {
Assertions.assertThat(namespaces).isNotNull().hasSize(2);
}
+ @Test
+ @NessieApiVersions(versions = NessieApiVersion.V1)
+ public void testNamespacesWithV1Api() {
+ createNamespacesAndTables();
+
+ testListTables();
+
+ List<Namespace> namespaces;
+ namespaces = catalog.listNamespaces(Namespace.of("foo"));
+ // NessieApiV1 should include both implicit and explicit namespaces
+ Assertions.assertThat(namespaces)
+ .containsExactlyInAnyOrder(Namespace.of("foo"), Namespace.of("foo", "bar"));
+ }
+
+ @Test
+ @NessieApiVersions(versions = NessieApiVersion.V2)
+ public void testNamespacesWithV2Api() {
Review Comment:
Would it be more readable if we had a common test that works for both API versions and validates the main namespace functionality and separate tests for v1/v2 for validating the handling of "implicit" namespaces?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #6712: Nessie: Support ApiV2 for Nessie client
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #6712:
URL: https://github.com/apache/iceberg/pull/6712#discussion_r1094773774
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java:
##########
@@ -103,7 +103,9 @@ public void initialize(String name, Map<String, String> options) {
api = nessieClientBuilder.build(NessieApiV2.class);
} else {
throw new IllegalArgumentException(
- "Unsupported client-api-version: " + apiVersion + ". Can only be v1 or v2");
+ String.format(
+ "Unsupported %s: " + apiVersion + ". Can only be v1 or v2",
Review Comment:
this uses now a mix of String.format and string concatenation. Would be good to only use String.format
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ajantha-bhat commented on a diff in pull request #6712: Nessie: Support ApiV2 for Nessie client
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #6712:
URL: https://github.com/apache/iceberg/pull/6712#discussion_r1199572850
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java:
##########
@@ -88,11 +89,24 @@ public void initialize(String name, Map<String, String> options) {
options.get(removePrefix.apply(NessieConfigConstants.CONF_NESSIE_REF));
String requestedHash =
options.get(removePrefix.apply(NessieConfigConstants.CONF_NESSIE_REF_HASH));
- NessieApiV1 api =
+
+ NessieClientBuilder<?> nessieClientBuilder =
createNessieClientBuilder(
options.get(NessieConfigConstants.CONF_NESSIE_CLIENT_BUILDER_IMPL))
- .fromConfig(x -> options.get(removePrefix.apply(x)))
- .build(NessieApiV1.class);
+ .fromConfig(x -> options.get(removePrefix.apply(x)));
+ final String apiVersion = options.get(NessieUtil.CLIENT_API_VERSION);
+ NessieApiV1 api;
+ if (apiVersion == null || apiVersion.equalsIgnoreCase("v1")) {
Review Comment:
ok changed to just 1 or 2.
Even Iceberg format version is 1 or 2. So, the syntax is inline.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ajantha-bhat commented on a diff in pull request #6712: Nessie: Support ApiV2 for Nessie client
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #6712:
URL: https://github.com/apache/iceberg/pull/6712#discussion_r1229834658
##########
nessie/src/test/java/org/apache/iceberg/nessie/BaseTestIceberg.java:
##########
@@ -145,7 +147,8 @@ NessieCatalog initCatalog(String ref, String hash) {
.put("ref", ref)
.put(CatalogProperties.URI, uri)
.put("auth-type", "NONE")
- .put(CatalogProperties.WAREHOUSE_LOCATION, temp.toUri().toString());
+ .put(CatalogProperties.WAREHOUSE_LOCATION, temp.toUri().toString())
+ .put("client-api-version", apiVersion);
Review Comment:
Nope. The other configurations like `uri`, `ref` is not using properties from `NessieConfigConstant` because catalog properties are passed without prefix.
`CONF_NESSIE_CLIENT_BUILDER_IMPL` is an exception which does not follow this style. We discussed this in Nessie public zulip channel and planning to deprecate it in future.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ajantha-bhat commented on pull request #6712: Nessie: Support ApiV2 for Nessie client
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on PR #6712:
URL: https://github.com/apache/iceberg/pull/6712#issuecomment-1563833059
@snazy: Can you please take a look at this PR again? Thanks
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #6712: Nessie: Support ApiV2 for Nessie client
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #6712:
URL: https://github.com/apache/iceberg/pull/6712#discussion_r1230770002
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNessieCatalog.java:
##########
@@ -115,17 +117,17 @@ private void resetData() throws NessieConflictException, NessieNotFoundException
private NessieCatalog initNessieCatalog(String ref) {
NessieCatalog newCatalog = new NessieCatalog();
newCatalog.setConf(hadoopConfig);
- newCatalog.initialize(
- "nessie",
+ ImmutableMap<String, String> options =
ImmutableMap.of(
"ref",
ref,
CatalogProperties.URI,
uri,
- "auth-type",
Review Comment:
is that not needed anymore?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra merged pull request #6712: Nessie: Support ApiV2 for Nessie client
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra merged PR #6712:
URL: https://github.com/apache/iceberg/pull/6712
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ajantha-bhat commented on a diff in pull request #6712: Nessie: Support ApiV2 for Nessie client
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #6712:
URL: https://github.com/apache/iceberg/pull/6712#discussion_r1199572873
##########
nessie/src/test/java/org/apache/iceberg/nessie/BaseTestIceberg.java:
##########
@@ -144,6 +144,9 @@ NessieCatalog initCatalog(String ref, String hash) {
.put(CatalogProperties.URI, uri)
.put("auth-type", "NONE")
.put(CatalogProperties.WAREHOUSE_LOCATION, temp.toUri().toString());
+ if (api instanceof NessieApiV2) {
Review Comment:
oh nice. Updated it.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] dimas-b commented on a diff in pull request #6712: Nessie: Support ApiV2 for Nessie client
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #6712:
URL: https://github.com/apache/iceberg/pull/6712#discussion_r1199136390
##########
nessie/src/test/java/org/apache/iceberg/nessie/BaseTestIceberg.java:
##########
@@ -144,6 +144,9 @@ NessieCatalog initCatalog(String ref, String hash) {
.put(CatalogProperties.URI, uri)
.put("auth-type", "NONE")
.put(CatalogProperties.WAREHOUSE_LOCATION, temp.toUri().toString());
+ if (api instanceof NessieApiV2) {
Review Comment:
API version is explicitly accessible now, cf. https://github.com/projectnessie/nessie/blob/9558d8e105a1e33763918c0336d8020118e4f7e3/servers/jax-rs-tests/src/main/java/org/projectnessie/jaxrs/tests/BaseTestNessieApi.java#L131
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] dimas-b commented on a diff in pull request #6712: Nessie: Support ApiV2 for Nessie client
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #6712:
URL: https://github.com/apache/iceberg/pull/6712#discussion_r1093344545
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNamespace.java:
##########
@@ -63,14 +65,56 @@ public void testListNamespaces() {
tables = catalog.listTables(null);
Assertions.assertThat(tables).isNotNull().hasSize(6);
- List<Namespace> namespaces = catalog.listNamespaces();
- Assertions.assertThat(namespaces).isNotNull().hasSize(5);
- namespaces = catalog.listNamespaces(Namespace.of("a"));
- Assertions.assertThat(namespaces).isNotNull().hasSize(3);
- namespaces = catalog.listNamespaces(Namespace.of("a", "b"));
- Assertions.assertThat(namespaces).isNotNull().hasSize(2);
- namespaces = catalog.listNamespaces(Namespace.of("b"));
- Assertions.assertThat(namespaces).isNotNull().hasSize(2);
+ List<Namespace> namespaces;
+ if (api instanceof NessieApiV2) {
Review Comment:
Whenever test logic different, I think it would be nice to have one method for v1 and another for v2, each annotated with `@NessieApiVersions(...)` for its specific target version. Common code could be refactored into utility methods. WDYT?
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNamespace.java:
##########
@@ -63,14 +65,56 @@ public void testListNamespaces() {
tables = catalog.listTables(null);
Assertions.assertThat(tables).isNotNull().hasSize(6);
- List<Namespace> namespaces = catalog.listNamespaces();
- Assertions.assertThat(namespaces).isNotNull().hasSize(5);
- namespaces = catalog.listNamespaces(Namespace.of("a"));
- Assertions.assertThat(namespaces).isNotNull().hasSize(3);
- namespaces = catalog.listNamespaces(Namespace.of("a", "b"));
- Assertions.assertThat(namespaces).isNotNull().hasSize(2);
- namespaces = catalog.listNamespaces(Namespace.of("b"));
- Assertions.assertThat(namespaces).isNotNull().hasSize(2);
+ List<Namespace> namespaces;
+ if (api instanceof NessieApiV2) {
Review Comment:
Whenever test logic differs, I think it would be nice to have one method for v1 and another for v2, each annotated with `@NessieApiVersions(...)` for its specific target version. Common code could be refactored into utility methods. WDYT?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org