Nessie: Use latest hash for catalog APIs #6789

ajantha-bhat · 2023-02-09T16:43:25Z

Consider the scenario,
a) client1 - java Nessie catalog client creates a table1
b) client2 - spark + iceberg+ Nessie creates a table2
c) client3 - another java Nessie catalog client creates a table3

where all the 3 clients are connected to the same Nessie server.

Now for the first client (client1), list tables show only one table even though all 3 clients are connected to the same Nessie server.
The reason is only the catalog APIs that involves table operations (like loadTable(), commit(), etc) refresh the NessieIcebergClient. The APIs like listTables(), listNamespaces() use the old state as it doesn't use table operations.

Changes:

Pass null for the reference hash in api request for immutable references. So that server will use the latest hash

Other scenarios can be two concurrent spark sessions.

ajantha-bhat · 2023-02-09T16:44:46Z

nessie/src/test/java/org/apache/iceberg/nessie/TestMultipleClients.java

+            TableIdentifier.parse("foo.tbl1"), TableIdentifier.parse("foo.tbl2"));
+
+    Assertions.assertThat(catalog.listTables(Namespace.of("foo")))
+        .containsExactlyInAnyOrder(


This particular check fails without this PR. As, catalog refers to the old state for listTables().

ajantha-bhat · 2023-02-17T09:10:38Z

nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java

@@ -223,13 +224,13 @@ protected String defaultWarehouseLocation(TableIdentifier table) {

  @Override
  public List<TableIdentifier> listTables(Namespace namespace) {
-    return client.listTables(namespace);
+    return refreshedClient().listTables(namespace);


refreshing the client is enough for a few APIs that doesn't use TableOperations. But the code looks odd. So, I am refreshing all the APIs as it is a lightweight operation.

ajantha-bhat · 2023-02-17T10:55:23Z

Restarting the build.

Execution failed for task ':iceberg-spark:iceberg-spark-3.3_2.12:checkstyleMain'.
See https://docs.gradle.org/7.6/userguide/command_line_interface.html#sec:command_line_warnings
> A failure occurred while executing org.gradle.api.plugins.quality.internal.CheckstyleAction
499 actionable tasks: 499 executed
   > An unexpected error occurred configuring and executing Checkstyle.
      > java.lang.Error: Error was thrown while processing /home/runner/work/iceberg/iceberg/spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/SparkTableUtil.java

ajantha-bhat · 2023-02-17T12:06:49Z

cc: @snazy, @dimas-b, @nastra

snazy · 2023-02-17T13:05:45Z

Sure this does not break the expected-hash calculations for commits?

ajantha-bhat · 2023-02-17T14:06:21Z

Sure this does not break the expected-hash calculations for commits?

I just added a testcase for commit from multiple catalogs/clients on same branch + same table. It passes.
So, I think there is no impact.
Also, expected hash is passed after the refresh operation from NessieCatalog. So, the client won't be modified after passing expected hash in the same flow.

@snazy: Do you see any problem with the current changes? Do you feel we should only modify listTables, listNamespaces, listNamespaceMetadata, removeProperties, setProperties instead of refreshing all the APIs?

dimas-b

I agree with @snazy 's concern about "expected hash". I think the question here is not about multiple clients, but about correctness within the same JVM. If a transaction starts with Nessie reference being in a certain state, the commit of that Tx should use "expected" reference as it was at the start of the Tx... but it looks like this PR increases the risk of inadvertently updating the reference while transactions are in progress.

ajantha-bhat · 2023-02-17T17:08:32Z

Isn't now also (without this PR) if two concurrent commits happen for a same table, the client can get refreshed between the
caller of this

iceberg/nessie/src/main/java/org/apache/iceberg/nessie/NessieIcebergClient.java

Line 423 in bb95e8b

public void commitTable(

to

iceberg/nessie/src/main/java/org/apache/iceberg/nessie/NessieIcebergClient.java

Line 430 in bb95e8b

UpdateableReference updateableReference = getRef();

because of existing refresh in this code by concurrent operations?

iceberg/core/src/main/java/org/apache/iceberg/BaseMetastoreTableOperations.java

Line 119 in bb95e8b

if (base != current()) {

dimas-b · 2023-02-17T18:00:06Z

Isn't now also (without this PR) if two concurrent commits happen for a same table, the client can get refreshed [...]

I think so. This issue is not specific to this PR. I just think this PR makes it more likely to manifest.

Would it be possible to keep the "expected" hash at the Tx level?

ajantha-bhat · 2023-02-20T07:50:27Z

I think so. This issue is not specific to this PR. I just think this PR makes it more likely to manifest.
Would it be possible to keep the "expected" hash at the Tx level?

@dimas-b : In that case, txn can only commit one successful commit?

Sure this does not break the expected-hash calculations for commits?

@snazy: can you please elaborate on this? Without this PR also, concurrent commits from the same client can refresh the catalog and cause breakage of expected-hash.

a) Are you suggesting that this PR will increase the probability of that event happening and we don't want to do that?
b) Even the sequential updates have issues mentioned in the PR description and I want to fix them. Should I keep the changes only for listTables(), listNamespaces(), loadNamespaceMetadata(), setProperties(), removeProperties()?

snazy · 2023-02-20T12:15:11Z

I'm suggesting to provide a fix for it.

dimas-b · 2023-02-21T14:28:19Z

+1 to fixing how Nessie Catalog (and related classes) track expected hash and refresh references.

dimas-b · 2023-02-21T14:33:01Z

[...] In that case, txn can only commit one successful commit?

The Nessie server is able to resolve non-conflicting changes to the catalog, even when the expected hash is behind the current branch HEAD.

ajantha-bhat · 2023-02-24T09:01:43Z

@snazy & @dimas-b:

BaseTransaction.commitTransaction()
-> BaseMetastoreTableOperations.commit() with refresh client 
-> NessieTableOperations.doCommit() 
-> NessieIcebergClient.commitTable with the expected hash.

Based on my analysis, the Nessie commit operation flow is as above. There are chances that we use older commit hash as the expected hash with the latest client. For this, Nessie backend server will fail the commit and Iceberg will retry the transaction/commit using this code[1] by rebasing the metadata and client.

[1] code for rebase with refreshed metadata

iceberg/core/src/main/java/org/apache/iceberg/BaseTransaction.java

Lines 354 to 359 in 3efaee1

    
           .onlyRetryOn(CommitFailedException.class) 
        
           .run( 
        
               underlyingOps -> { 
        
                 try { 
        
                   underlyingOps.refresh(); 
        
                 } catch (NoSuchTableException e) {

So, Even if we add some code to update/refresh the expected hash to be the latest, It is not enough as TableMetadata will hold the old state. So, Nessie client cannot modify table metadata. The existing retry mechanism will handle this refresh.

So, I don't think we need to change anything. Please clarify or guide me if you think it is incorrect.

ajantha-bhat · 2023-02-27T10:07:54Z

@snazy, @dimas-b:
I checked some more code today.
I found that NessieTableOperation works on a copy of the client. Not the actual client from NessieCatalog. as withReference creates a new client object.

iceberg/nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java

Line 197 in cb612ed

client.withReference(tr.getReference(), tr.getHash()),

And the client inside the NessieTableOperation will never be refreshed/out of state. There is also this check.

iceberg/core/src/main/java/org/apache/iceberg/BaseMetastoreTableOperations.java

Line 121 in cb612ed

throw new CommitFailedException("Cannot commit: stale table metadata");

TLDR;
PR changes are for the client of NessieCatalog, not for the client of NessieTableOperations. Hence, I don't think there is any impact on the expected hash calculation of the commits of NessieTableOperations.

Please recheck the PR again and let me know if a change required on this.

dimas-b

Thanks for you work on making this use case more robust @ajantha-bhat ! Some more comments below.

nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java

dimas-b · 2023-03-01T19:22:14Z

nessie/src/test/java/org/apache/iceberg/nessie/TestMultipleClients.java

+  @AfterEach
+  public void afterEach() throws Exception {
+    dropTables(catalog);
+    dropTables(anotherCatalog);


Why bother? The super class will delete/reset all Nessie data. Table files are located in a temp. dir.

If the intention is to remove table's files, wouldn't it be preferable to do that in the base class and the FS level for all tests after resetting Nessie?

I followed TestNessieTable which was cleaning the table files located in a temp dir.
Other test cases doesn't have it.

Agree that this should be at base test class level. I will handle it in a follow up PR.

nessie/src/test/java/org/apache/iceberg/nessie/TestMultipleClients.java

nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java

ajantha-bhat · 2023-03-02T02:45:55Z

@dimas-b: Thanks for the review. I have replied to the comments. Please check.

ajantha-bhat · 2023-03-02T05:25:31Z

This flaky error again !

* What went wrong:
Execution failed for task ':iceberg-spark:iceberg-spark-3.3_2.12:checkstyleMain'.
> A failure occurred while executing org.gradle.api.plugins.quality.internal.CheckstyleAction
   > An unexpected error occurred configuring and executing Checkstyle.
      > java.lang.Error: Error was thrown while processing /home/runner/work/iceberg/iceberg/spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/source/SparkScanBuilder.java

opened this to get more info:
#6982

ajantha-bhat · 2023-03-13T10:49:22Z

While the code looks technically correct, this change introduces an unnecessary extra round-trip. Usually, people work on the HEAD of a Nessie branch, which, for most Nessie operations, does not require the client to know the HEAD's commit-ID and pass it back to the server.
IMO the NessieIcebergClient needs to be able to handle the case of referencing the HEAD of a branch for NessieCatalog (and only pass the branch/tag name to the Nessie server), but be able provide an instance of NessieIcebergClient with the expected hash / commit ID for newTableOps, renameTable, dropTable, etc.

@snazy, @dimas-b, @nastra: I have avoided the unnecessary extra round-trip by fixing it differently than before. Please take a look. Thanks.

snazy

This change adds too many unnecessary code changes. Most of the changes aren't justified, the required fix can likely be achieved with way less changes. The current PR also doesn't clean up renameTable wrt the scope of this PR.

ajantha-bhat · 2023-03-13T14:10:55Z

This change adds too many unnecessary code changes. Most of the changes aren't justified, the required fix can likely be achieved with way less changes. The current PR also doesn't clean up renameTable wrt the scope of this PR.

@snazy: I gave it another try. Please take a look. Also the renameTable and dropTable was not going via TableOperations. So, I have handled it similarly to the existing commitTable logic now.

ajantha-bhat · 2023-03-13T15:11:31Z

one of the builds failed due to #7023

https://github.com/apache/iceberg/actions/runs/4405434484

re-triggering the build.

nessie/src/main/java/org/apache/iceberg/nessie/UpdateableReference.java

nessie/src/main/java/org/apache/iceberg/nessie/NessieIcebergClient.java

nessie/src/main/java/org/apache/iceberg/nessie/UpdateableReference.java

ajantha-bhat · 2023-03-16T06:47:40Z

one of the builds failed due to #7023

https://github.com/apache/iceberg/actions/runs/4434173539/jobs/7779904892

re-triggering the build.

snazy · 2023-03-16T08:32:27Z

nessie/src/main/java/org/apache/iceberg/nessie/UpdateableReference.java

@@ -60,13 +60,6 @@ public String getHash() {
    return reference.getHash();
  }

-  public Branch getAsBranch() {


I deprecated it instead of removing it because it is a public method.

This method a safe cast - you replaced it (not clear for which reason) with an unconditional cast (and BTW left another then unused method).

This method a safe cast

because all the places where this was called, already there is a check for getRef().checkMutable(). So, no need for one more method call that checks if it is a branch. Hence, directly casting to branch.

I will remove isBranch too.

nit: how about returning a Branch from checkMutable to avoid type casts in the calling code? (just a thought, no need to delay this PR because of this).

I think checkMutable is an early check to ensure we are working on a branch which will later be modified by a commit. Whereas getRef or getAsBranch is to get the current state of the reference. I am not sure about combining them at the moment. I will explore this in a follow-up a suggested.

nessie/src/main/java/org/apache/iceberg/nessie/NessieIcebergClient.java

ajantha-bhat · 2023-04-05T06:22:37Z

I think the PR was ready. Please recheck.

ajantha-bhat · 2023-04-10T03:06:49Z

Thanks, @snazy and @dimas-b for the review.
@Fokko can you please help in merging this?

dimas-b · 2023-04-10T15:41:43Z

nessie/src/main/java/org/apache/iceberg/nessie/UpdateableReference.java

@@ -60,13 +60,6 @@ public String getHash() {
    return reference.getHash();
  }

-  public Branch getAsBranch() {


nit: how about returning a Branch from checkMutable to avoid type casts in the calling code? (just a thought, no need to delay this PR because of this).

Fokko

Thanks @ajantha-bhat for the PR, and @snazy & @dimas-b for the review. LGTM!

* Nessie: Handle refresh for catalog APIs that doesn't use table operations * Add commit testcase * Another test case * Address comments * Avoid hash roundtrip * Address new comments * refactor

ajantha-bhat marked this pull request as draft February 9, 2023 16:43

github-actions bot added the NESSIE label Feb 9, 2023

ajantha-bhat commented Feb 9, 2023

View reviewed changes

ajantha-bhat changed the title ~~Nessie: Handle refresh for catalog APIs that doesn't use table operations~~ Nessie: Add refresh for catalog APIs that doesn't use table operations Feb 9, 2023

ajantha-bhat force-pushed the refresh branch from dc01d51 to f4069a6 Compare February 17, 2023 09:05

ajantha-bhat commented Feb 17, 2023

View reviewed changes

ajantha-bhat closed this Feb 17, 2023

ajantha-bhat reopened this Feb 17, 2023

ajantha-bhat marked this pull request as ready for review February 17, 2023 10:56

ajantha-bhat changed the title ~~Nessie: Add refresh for catalog APIs that doesn't use table operations~~ Nessie: Add refresh for catalog APIs Feb 17, 2023

dimas-b reviewed Feb 17, 2023

View reviewed changes

dimas-b reviewed Mar 1, 2023

View reviewed changes

ajantha-bhat closed this Mar 2, 2023

ajantha-bhat reopened this Mar 2, 2023

ajantha-bhat closed this Mar 2, 2023

ajantha-bhat reopened this Mar 2, 2023

ajantha-bhat force-pushed the refresh branch 2 times, most recently from 678f6bd to dccfd6b Compare March 13, 2023 09:57

ajantha-bhat changed the title ~~Nessie: Add refresh for catalog APIs~~ Nessie: Use latest hash for catalog APIs Mar 13, 2023

snazy suggested changes Mar 13, 2023

View reviewed changes

Avoid hash roundtrip

e62c214

ajantha-bhat force-pushed the refresh branch from dccfd6b to e62c214 Compare March 13, 2023 13:26

ajantha-bhat closed this Mar 13, 2023

ajantha-bhat reopened this Mar 13, 2023

snazy suggested changes Mar 15, 2023

View reviewed changes

Address new comments

804bc28

ajantha-bhat closed this Mar 16, 2023

ajantha-bhat reopened this Mar 16, 2023

snazy suggested changes Mar 16, 2023

View reviewed changes

ajantha-bhat force-pushed the refresh branch 2 times, most recently from 3bff65d to f04c264 Compare March 16, 2023 10:16

refactor

cc1c61c

ajantha-bhat force-pushed the refresh branch from f04c264 to cc1c61c Compare March 16, 2023 11:18

snazy approved these changes Apr 6, 2023

View reviewed changes

dimas-b approved these changes Apr 10, 2023

View reviewed changes

Fokko approved these changes Apr 11, 2023

View reviewed changes

Fokko merged commit af4ddd2 into apache:master Apr 11, 2023

ajantha-bhat mentioned this pull request Apr 11, 2023

Nessie: Fix testcase failures #7320

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nessie: Use latest hash for catalog APIs #6789

Nessie: Use latest hash for catalog APIs #6789

ajantha-bhat commented Feb 9, 2023 •

edited

ajantha-bhat Feb 9, 2023

ajantha-bhat Feb 17, 2023

ajantha-bhat commented Feb 17, 2023

ajantha-bhat commented Feb 17, 2023

snazy commented Feb 17, 2023

ajantha-bhat commented Feb 17, 2023 •

edited

dimas-b left a comment

ajantha-bhat commented Feb 17, 2023

dimas-b commented Feb 17, 2023 •

edited

ajantha-bhat commented Feb 20, 2023

snazy commented Feb 20, 2023

dimas-b commented Feb 21, 2023

dimas-b commented Feb 21, 2023

ajantha-bhat commented Feb 24, 2023 •

edited

ajantha-bhat commented Feb 27, 2023 •

edited

dimas-b left a comment

dimas-b Mar 1, 2023

ajantha-bhat Mar 2, 2023

ajantha-bhat commented Mar 2, 2023

ajantha-bhat commented Mar 2, 2023 •

edited

ajantha-bhat commented Mar 13, 2023

snazy left a comment

ajantha-bhat commented Mar 13, 2023

ajantha-bhat commented Mar 13, 2023 •

edited

ajantha-bhat commented Mar 16, 2023

snazy Mar 16, 2023

ajantha-bhat Mar 16, 2023

dimas-b Apr 10, 2023 •

edited

ajantha-bhat Apr 11, 2023

ajantha-bhat commented Apr 5, 2023

ajantha-bhat commented Apr 10, 2023

dimas-b Apr 10, 2023 •

edited

Fokko left a comment

Nessie: Use latest hash for catalog APIs #6789

Nessie: Use latest hash for catalog APIs #6789

Conversation

ajantha-bhat commented Feb 9, 2023 • edited

ajantha-bhat Feb 9, 2023

Choose a reason for hiding this comment

ajantha-bhat Feb 17, 2023

Choose a reason for hiding this comment

ajantha-bhat commented Feb 17, 2023

ajantha-bhat commented Feb 17, 2023

snazy commented Feb 17, 2023

ajantha-bhat commented Feb 17, 2023 • edited

dimas-b left a comment

Choose a reason for hiding this comment

ajantha-bhat commented Feb 17, 2023

dimas-b commented Feb 17, 2023 • edited

ajantha-bhat commented Feb 20, 2023

snazy commented Feb 20, 2023

dimas-b commented Feb 21, 2023

dimas-b commented Feb 21, 2023

ajantha-bhat commented Feb 24, 2023 • edited

ajantha-bhat commented Feb 27, 2023 • edited

dimas-b left a comment

Choose a reason for hiding this comment

dimas-b Mar 1, 2023

Choose a reason for hiding this comment

ajantha-bhat Mar 2, 2023

Choose a reason for hiding this comment

ajantha-bhat commented Mar 2, 2023

ajantha-bhat commented Mar 2, 2023 • edited

ajantha-bhat commented Mar 13, 2023

snazy left a comment

Choose a reason for hiding this comment

ajantha-bhat commented Mar 13, 2023

ajantha-bhat commented Mar 13, 2023 • edited

ajantha-bhat commented Mar 16, 2023

snazy Mar 16, 2023

Choose a reason for hiding this comment

ajantha-bhat Mar 16, 2023

Choose a reason for hiding this comment

dimas-b Apr 10, 2023 • edited

Choose a reason for hiding this comment

ajantha-bhat Apr 11, 2023

Choose a reason for hiding this comment

ajantha-bhat commented Apr 5, 2023

ajantha-bhat commented Apr 10, 2023

dimas-b Apr 10, 2023 • edited

Choose a reason for hiding this comment

Fokko left a comment

Choose a reason for hiding this comment

ajantha-bhat commented Feb 9, 2023 •

edited

ajantha-bhat commented Feb 17, 2023 •

edited

dimas-b commented Feb 17, 2023 •

edited

ajantha-bhat commented Feb 24, 2023 •

edited

ajantha-bhat commented Feb 27, 2023 •

edited

ajantha-bhat commented Mar 2, 2023 •

edited

ajantha-bhat commented Mar 13, 2023 •

edited

dimas-b Apr 10, 2023 •

edited

dimas-b Apr 10, 2023 •

edited