Nessie: Extract Catalog client code to NessieClient for Trino Consumption #4491
Conversation
nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java
Outdated
Show resolved
Hide resolved
nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java
Outdated
Show resolved
Hide resolved
nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java
Outdated
Show resolved
Hide resolved
nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java
Outdated
Show resolved
Hide resolved
nastra
force-pushed
the
simplify-nessie-catalog
branch
from
66c5a4e
to
8c6dc22
Compare
nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java
Outdated
Show resolved
Hide resolved
nastra
force-pushed
the
simplify-nessie-catalog
branch
2 times, most recently
from
848f4a4
to
162a886
Compare
| // to catch all kinds of network errors (e.g. connection reset). Network code implementation | ||
| // details and all kinds of network devices can induce unexpected behavior. So better be | ||
| // safe than sorry. | ||
| throw new CommitStateUnknownException(ex); |
There was a problem hiding this comment.
This should only be used when committing to the table. While this may not know whether the table was renamed or not, this exception should be reserved for commit problems.
There was a problem hiding this comment.
we can probably change this in a follow-up. This is just code that was moved from the NessieCatalog to this place
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
||
| public class NessieIcebergClient implements AutoCloseable { |
There was a problem hiding this comment.
I don't think that this should be public. When would anyone need to use this directly?
Also, this looks like it implements most of the catalog API. Is it really cleaner to move code here? What is the distinct purpose of this class vs the NessieCatalog class?
There was a problem hiding this comment.
as discussed on Slack, the idea was initially to clean things up in the NessieCatalog. However, I realized that we can re-use this entire code in Trino (where we're currently integrating Nessie as well). That's the main reason for making this class public
This adds a new `NessieIcebergClient` class that encapsulates some of the features that have been used across the `NessieCatalog`. It also slightly improves the error messages to include the reference or the table name when something fails. Note that the `testFailure()` method was updated, because we're now always using the same `UpdateableReference` between `NessieCatalog` and `NessieTableOperations`, so that it's higly unlikely that those two get out-of-sync.
nastra
force-pushed
the
simplify-nessie-catalog
branch
from
162a886
to
66a7b81
Compare
nessie/src/main/java/org/apache/iceberg/nessie/NessieIcebergClient.java
Outdated
Show resolved
Hide resolved
nessie/src/test/java/org/apache/iceberg/nessie/TestNessieTable.java
Outdated
Show resolved
Hide resolved
RussellSpitzer
changed the title
|
@nastra Merged! Thanks for the PR and I hope to see that Nessie Trino Support soon! |
This adds a new
NessieIcebergClientclass that encapsulates some ofthe features that have been used across the
NessieCatalog.It also slightly improves the error messages to include the reference or
the table name when something fails.
Note that the
testFailure()method was updated, because we're nowalways using the same
UpdateableReferencebetweenNessieCatalogandNessieTableOperations, so that it's higly unlikely that those two getout-of-sync.