Add an API compatibility check to Nessie clients #6818

adutra · 2023-05-12T16:08:36Z

... and change default URI of GC tool to a v2 URI.

snazy · 2023-05-12T16:36:45Z

Can you move the fix for the GC tool from this PR?

dimas-b

The PR LGTM overall. Did you have a chance to test how the new error manifests on the Iceberg side (e.g. in Spark) when the Nessie Catalog URI is misconfigured? We may need a corresponding change in the Iceberg NessieCatalog.

dimas-b · 2023-05-12T16:36:05Z

api/client/src/main/java/org/projectnessie/client/http/NessieApiCompatibility.java

+    // FIXME we need a better way to detect API vs URI prefix mismatches.
+    int serverApiVersion = specVersion != null && !specVersion.isEmpty() ? 2 : 1;
+    if (clientApiVersion != serverApiVersion) {
+      throw new IllegalArgumentException(


WDYT about using NessieServiceException instead? NessieServiceException is often reported when the URL is misconfigured in such as way that Nessie end points are not even reachable... Would it make sense to reuse it for this failure mode too?

That's an idea, although here the error is raised on the client side. What would you say if we create a dedicated NessieApiCompatibilityException instead?

NessieServiceException is also raised on the client side.

+1 to NessieApiCompatibilityException

adutra · 2023-05-12T16:41:59Z

Switching to draft, sorry, because unsurprisingly, these changes are breaking a few compatibility tests, especially ITOlderServers. Obviously I could fix them by simply disabling the compatibility check, but I would like to first understand why and where the tests are failing.

adutra · 2023-05-12T16:44:51Z

Did you have a chance to test how the new error manifests on the Iceberg side (e.g. in Spark) when the Nessie Catalog URI is misconfigured? We may need a corresponding change in the Iceberg NessieCatalog.

Very good point, I will check.

adutra · 2023-05-12T17:19:04Z

So, the compatibility tests were failing because of a combination of factors:

The NessieApi instances are eagerly injected into the test classes, and so when e.g. the server doesn't support V2 at all, the apiV2 field cannot be populated and the whole test class fails.
A more interesting failure is e.g. when the server does support V2, but returns null for specVersion. This actually happens for many server versions, I believe because specVersion is a rather recent addition.

I temporarily fixed the issues by disabling the client compat check.

But this unfortunately uncovers a deeper problem: specVersion is not a good indicator of whether the server is replying with v1 or v2. I will think about this more and see if there is a better alternative.

adutra · 2023-05-12T21:54:24Z

OK, I am proposing something different: test what the /trees/tree/{defaultBranch} endpoint returns. If it returns 200, then the server used v1 unambiguously, since this path cannot exist in v2. I could have used /trees/tree, but in the unlikely case where the default branch is named tree, this path is ambiguous.

dimas-b · 2023-05-12T23:19:56Z

tree/main is also a valid branch name 🤔

dimas-b · 2023-05-12T23:22:03Z

How about returning the "effective" API version from the /config endpoint (existing NessieConfiguration object or something new)?

snazy · 2023-05-13T10:55:18Z

api/client/src/main/java/org/projectnessie/client/http/NessieApiCompatibility.java

+    return httpClient.newRequest().path("config").get().readEntity(NessieConfiguration.class);
+  }
+
+  private static int fetchActualServerApiVersion(


This check requires that authz works for you on that branch.
Also not sure whether it's really worth to run some arbitrary request as a guess implying an API version.

Better to return the "effective" version (the one that was used on the server side) via NessieConfiguration

snazy · 2023-05-13T10:56:04Z

api/client/src/main/java/org/projectnessie/client/http/NessieApiCompatibility.java

+  }
+
+  private static NessieConfiguration fetchConfig(HttpClient httpClient) {
+    return httpClient.newRequest().path("config").get().readEntity(NessieConfiguration.class);


You'd actually have to read it into a Map<String, Object>, because newer servers may return more elements, which would breal this check.

snazy · 2023-05-13T10:57:52Z

api/client/src/main/java/org/projectnessie/client/http/NessieApiCompatibility.java

+  public static void check(int clientApiVersion, HttpClient httpClient)
+      throws NessieApiCompatibilityException {
+    NessieConfiguration config = fetchConfig(httpClient);
+    int minServerApiVersion = config.getMinSupportedApiVersion();


If min/max are not present (see below), then you can safely assume V1.
If those are present, you can safely assume that V2.
No need for any other checks.

This is not true, unfortunately.

Min and max were introduced in Nessie 0.55.0, way after the introduction of v2 in 0.46.0. So all servers between [0.46.0,0.55.0[ reply with the same payload for both versions, e.g. 0.47.0:

v1: {"defaultBranch":"main","maxSupportedApiVersion":2}

v2: {"defaultBranch":"main","maxSupportedApiVersion":2}

Starting with 0.55.0, indeed the response is different:

v1: {"defaultBranch":"main","maxSupportedApiVersion":2}

v2: {"defaultBranch":"main","minSupportedApiVersion":1,"maxSupportedApiVersion":2,"specVersion":"2.0-beta.1"}

So this is not a good indicator of the actual server version.

v2 is still in beta, we can somewhat ignore older versions w/ v2.

snazy · 2023-05-13T11:02:53Z

api/client/src/main/java/org/projectnessie/client/http/HttpClientBuilder.java

@@ -323,12 +337,18 @@ public <API extends NessieApi> API build(Class<API> apiVersion) {
    if (apiVersion.isAssignableFrom(HttpApiV1.class)) {
      builder.setJsonView(Views.V1.class);
      HttpClient httpClient = builder.build();
+      if (enableApiCompatibilityCheck) {
+        NessieApiCompatibility.check(1, httpClient);
+      }
      return (API) new HttpApiV1(new NessieHttpClient(httpClient));
    }

    if (apiVersion.isAssignableFrom(HttpApiV2.class)) {


Can you move this if up (minor nit)

Actually this would break things: HttpApiV2 is a subclass of NessieApiV1.

snazy · 2023-05-13T11:05:47Z

api/client/src/test/java/org/projectnessie/client/http/TestNessieHttpClient.java

+  @Test
+  void testApiCompatibility() {
+    // Good cases
+    assertThatCode(() -> testConfig(NessieApiV1.class, 1, 1, 1, true)).doesNotThrowAnyException();


Candidate for softly-assertions (soft.assertThatCode...)

compatibility/compatibility-tests/build.gradle.kts

adutra · 2023-05-14T13:00:15Z

tree/main is also a valid branch name 🤔

Hmm right. There will always be a risk of ambiguity it seems.

This check requires that authz works for you on that branch.

That is true as well, I didn't think of it so far. This may be a blocker.

How about returning the "effective" API version from the /config endpoint

That is imho the proper way to solve this, but keep in mind that this won't solve the problem for existing Nessie versions, only for future ones.

I think this PR is getting stuck. As it stands, I see 2 options going forward:

Give up on this work and just acknowledge that there is no good way to detect URI vs API version mismatches.
Implement "returning the effective API version from the /config endpoint" and only run the compatibility check for versions that implemented this endpoint.

Wdyt?

snazy · 2023-05-15T07:07:34Z

Implement "returning the effective API version from the /config endpoint" and only run the compatibility check for versions that implemented this endpoint.

That could work.

...src/intTest/java/org/projectnessie/tools/compatibility/tests/AbstractCompatibilityTests.java

dimas-b

The latest approach (actual API version in NessieConfiguration) looks good to me.

dimas-b · 2023-05-16T16:18:13Z

...ility-tests/src/intTest/java/org/projectnessie/tools/compatibility/tests/ITOlderServers.java

  @Override
+  void getConfig() {


nit: maybe rename to getConfigV1() for clarity?

dimas-b · 2023-05-16T16:26:02Z

api/client/src/test/java/org/projectnessie/client/http/TestNessieApiCompatibility.java

+            .hasMessageContaining(
+                expectation == Expectation.MISMATCH
+                    ? "mismatch"
+                    : expectation == Expectation.TOO_OLD ? "too old" : "too new")


Suggestion: put too old, etc. into enum object fields in Expectation and use smth like expectation.getExpectedText()

adutra added 3 commits May 12, 2023 18:04

Add an API compatibility check to Nessie clients

fea6656

Minor nit

52b474c

spotlessApply

ee1c26e

dimas-b reviewed May 12, 2023

View reviewed changes

adutra marked this pull request as draft May 12, 2023 16:40

adutra added 3 commits May 12, 2023 19:12

Fix compatibility tests

71c7c7f

Minor nit

1bc9959

Revert NessieOptions

e0dab4b

adutra added 2 commits May 12, 2023 19:50

Fix TestNessieApiHolder

24058ae

Doing things differently

f6cbeaa

adutra marked this pull request as ready for review May 12, 2023 22:14

snazy reviewed May 13, 2023

View reviewed changes

wip

9d12b31

adutra added 6 commits May 15, 2023 14:41

New field in NessieConfiguration

28bbaf1

Revert unnecessary change

a7fbcc6

Move if up

6e18959

Revert unnecessary change

435d5b2

Revert move if up

5ae7cef

Improving compat tests

d60d2a0

adutra commented May 15, 2023

View reviewed changes

...src/intTest/java/org/projectnessie/tools/compatibility/tests/AbstractCompatibilityTests.java Show resolved Hide resolved

Fix failing tests

df8d35e

Soft assertions

692b748

dimas-b previously approved these changes May 16, 2023

View reviewed changes

snazy previously approved these changes May 17, 2023

View reviewed changes

Review comments

52b90ec

adutra dismissed stale reviews from snazy and dimas-b via 52b90ec May 17, 2023 16:58

snazy approved these changes May 17, 2023

View reviewed changes

adutra merged commit cff1788 into projectnessie:main May 17, 2023

adutra deleted the gc-tool branch May 17, 2023 18:07

dimas-b mentioned this pull request May 19, 2023

Nessie: Support ApiV2 for Nessie client apache/iceberg#6712

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an API compatibility check to Nessie clients #6818

Add an API compatibility check to Nessie clients #6818

adutra commented May 12, 2023

snazy commented May 12, 2023

dimas-b left a comment

dimas-b May 12, 2023

adutra May 12, 2023

dimas-b May 12, 2023

adutra commented May 12, 2023

adutra commented May 12, 2023

adutra commented May 12, 2023

adutra commented May 12, 2023

dimas-b commented May 12, 2023

dimas-b commented May 12, 2023

snazy May 13, 2023

snazy May 13, 2023

snazy May 13, 2023

snazy May 13, 2023

adutra May 14, 2023

snazy May 15, 2023

snazy May 13, 2023

adutra May 15, 2023

snazy May 13, 2023

adutra commented May 14, 2023

snazy commented May 15, 2023

dimas-b left a comment

dimas-b May 16, 2023

dimas-b May 16, 2023

Add an API compatibility check to Nessie clients #6818

Add an API compatibility check to Nessie clients #6818

Conversation

adutra commented May 12, 2023

snazy commented May 12, 2023

dimas-b left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adutra commented May 12, 2023

adutra commented May 12, 2023

adutra commented May 12, 2023

adutra commented May 12, 2023

dimas-b commented May 12, 2023

dimas-b commented May 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adutra commented May 14, 2023

snazy commented May 15, 2023

dimas-b left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment