-
Notifications
You must be signed in to change notification settings - Fork 5.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for REST catalog in Iceberg connector #22417
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the documentation! Just two nits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good overall except that I think I disagree with leaving the refactor until later on IcebergResourceFactory
. Might even be better to have this PR with 2 commits: one for the refactor and one for the REST catalog?
How were you thinking about refactoring it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! (docs)
Pull updated branch, new local build. Everything looks good.
For reference, I agree with @ZacBlanco, use a separate commit for refactoring |
@ZacBlanco @hantangwangd love the idea of doing two separate commits. That would be the best of both worlds since I can keep it separate from the functionality but also get the refactor done sooner than later As for the re-factor, there are certainly several ways to go about it. I'm currently (at least once I'm back from vacation) considering two methods. First is passing a Please feel free to suggest additional ways and to weigh in on the above! As mentioned, I won't be back to this in another day or two 🙂 |
3759ffb
to
3667787
Compare
I took a brief look. Re-factor overall looks good! I will perform a detailed review once out of draft stage |
874c730
to
abd351b
Compare
c6890d3
to
2a6589f
Compare
Hi @kiersten-stokes, can you rebase your branch onto the newest master branch? it's a bit too far behind :-). |
2a6589f
to
52a4410
Compare
nit, suggest adding PR # into each item of the release note entry:
|
52a4410
to
8f7bf77
Compare
8f7bf77
to
3c95a7c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! (docs)
Pull updated branch, new local docs build, looks good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for adding this great feature. I took a look at the first commit, overall it looks good to me. But I'm a little confused by the large amount of error logs printed by the test cases.
After check the code, I found that the error log is printed by RESTCatalogServlet
, and by comparing the test implementations of Iceberg and Trino, I notice that they both use certain methods to hide these error messages (that is to say, the error messages are still logged, but not displayed). Iceberg inject a NOPLogger, while Trino overwrite the servlet RestCatalogServlet
and injected its own configurable io.airlift.log.Logger
.
So should we as well do something to get rid of these annoying error messages? What's your opinion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mostly just nits
...iceberg/src/main/java/com/facebook/presto/iceberg/nessie/IcebergNessieSessionProperties.java
Outdated
Show resolved
Hide resolved
restJdbcBackend.setConf(hdfsEnvironment.getConfiguration(new HdfsContext(SESSION), new Path(location.getAbsolutePath()))); | ||
|
||
Map<String, String> properties = ImmutableMap.<String, String>builder() | ||
.put(URI, "jdbc:h2:mem:test" + System.nanoTime() + "_" + ThreadLocalRandom.current().nextInt()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: will we actually need nanoTime and a random variable? - also, maybe add another underscore between :test
and `System.nanoTime()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed that both is probably overkill, although this pattern came up a few different times in other tests that use this type of catalog. I was guessing it was a 'better safe than sorry' situation. Definitely agree on the underscore!
presto-iceberg/src/test/java/com/facebook/presto/iceberg/rest/TestIcebergRestConfig.java
Outdated
Show resolved
Hide resolved
presto-iceberg/src/test/java/com/facebook/presto/iceberg/rest/TestIcebergRestConfig.java
Outdated
Show resolved
Hide resolved
presto-iceberg/src/test/java/com/facebook/presto/iceberg/rest/TestIcebergSmokeRest.java
Outdated
Show resolved
Hide resolved
presto-iceberg/src/test/java/com/facebook/presto/iceberg/rest/TestIcebergSmokeRest.java
Outdated
Show resolved
Hide resolved
presto-jdbc/src/test/java/com/facebook/presto/jdbc/TestPrestoDriverAuth.java
Outdated
Show resolved
Hide resolved
presto-iceberg/src/test/java/com/facebook/presto/iceberg/rest/TestIcebergDistributedRest.java
Outdated
Show resolved
Hide resolved
presto-iceberg/src/test/java/com/facebook/presto/iceberg/rest/TestIcebergDistributedRest.java
Outdated
Show resolved
Hide resolved
3c95a7c
to
0f9baa2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just took a whole look at all the commits, it's a very pretty refactoring, overall looks good to me. Just one thing for discussion, and some nits.
presto-iceberg/src/test/java/org/apache/iceberg/rest/IcebergRestCatalogServlet.java
Outdated
Show resolved
Hide resolved
presto-iceberg/src/test/java/org/apache/iceberg/rest/IcebergRestCatalogServlet.java
Outdated
Show resolved
Hide resolved
presto-iceberg/src/test/java/org/apache/iceberg/rest/IcebergRestCatalogServlet.java
Outdated
Show resolved
Hide resolved
presto-iceberg/src/test/java/org/apache/iceberg/rest/IcebergRestCatalogServlet.java
Outdated
Show resolved
Hide resolved
presto-iceberg/src/main/java/com/facebook/presto/iceberg/rest/IcebergRestConfig.java
Show resolved
Hide resolved
presto-iceberg/src/main/java/com/facebook/presto/iceberg/rest/AuthenticationType.java
Show resolved
Hide resolved
presto-iceberg/src/main/java/com/facebook/presto/iceberg/rest/SessionType.java
Show resolved
Hide resolved
presto-iceberg/src/test/java/org/apache/iceberg/rest/IcebergRestCatalogServlet.java
Outdated
Show resolved
Hide resolved
...iceberg/src/main/java/com/facebook/presto/iceberg/nessie/IcebergNessieSessionProperties.java
Outdated
Show resolved
Hide resolved
0f9baa2
to
560e4b9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix, only some little nits. Again, it's a very pretty refactoring, make things more clearer.
presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergHiveModule.java
Outdated
Show resolved
Hide resolved
...to-iceberg/src/main/java/com/facebook/presto/iceberg/nessie/IcebergNessieCatalogFactory.java
Outdated
Show resolved
Hide resolved
...to-iceberg/src/main/java/com/facebook/presto/iceberg/nessie/IcebergNessieCatalogFactory.java
Outdated
Show resolved
Hide resolved
presto-iceberg/src/main/java/com/facebook/presto/iceberg/rest/IcebergRestConfig.java
Show resolved
Hide resolved
Make nessie config optional in session properties
560e4b9
to
7062bba
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the great work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome refactor and nice work--thank you!
Description
This PR is two commits:
com.fasterxml.jackson.core:jackson-core
and:jackson-databind
to be >2.11 in the root pomMotivation and Context
Closes #20422
Impact
Test Plan
Tests have been added that spin up a testing HTTP server to create an Iceberg REST catalog with JDBC backing. Currently, three of the distributed smoke tests fail when using this set up due to a bug in Iceberg that I intend to follow up on
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.