Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Support additional configurations for Nessie catalog #17725

Merged
merged 2 commits into from
Nov 2, 2023
Merged

Support additional configurations for Nessie catalog #17725

merged 2 commits into from
Nov 2, 2023

Conversation

ajantha-bhat
Copy link
Member

@ajantha-bhat ajantha-bhat commented Jun 1, 2023 •
edited by ebyhr
Loading

Description

Nessie server can be deployed with Auth mode like keycloak.
So, need to expose the Nessie client configurations to handle the Auth.
Along with that, some common Nessie server configurations like read-timeout-ms,
connect-timeout-ms, and compression-enabled properties are exposed to have finer
control over the Nessie commits.

Additional context and related issues

https://projectnessie.org/tools/client_config/

Release notes

(x) Release notes are required, with the following suggested text:

# Iceberg
* Add support for bearer authentication in the Nessie catalog. ({issue}`17725`)

@cla-bot cla-bot bot added the cla-signed label Jun 1, 2023
@github-actions github-actions bot added docs iceberg Iceberg connector labels Jun 1, 2023
dimas-b
dimas-b reviewed Jun 1, 2023
View reviewed changes
Copy link

@dimas-b dimas-b left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice improvement 👍

...iceberg/src/main/java/io/trino/plugin/iceberg/catalog/nessie/IcebergNessieCatalogConfig.java Outdated Show resolved Hide resolved
...iceberg/src/main/java/io/trino/plugin/iceberg/catalog/nessie/IcebergNessieCatalogModule.java Outdated Show resolved Hide resolved
ebyhr
ebyhr reviewed Jun 1, 2023
View reviewed changes
Copy link
Member

@ebyhr ebyhr left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add tests (config test is insufficient especially for auth) and fix the commit title https://github.com/trinodb/trino/blob/master/.github/DEVELOPMENT.md#format-git-commit-messages

ebyhr

This comment was marked as duplicate.

Sign in to view

ebyhr
ebyhr reviewed Jun 1, 2023
View reviewed changes
...n/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/nessie/AuthenticationType.java Outdated Show resolved Hide resolved
...iceberg/src/main/java/io/trino/plugin/iceberg/catalog/nessie/IcebergNessieCatalogConfig.java Outdated Show resolved Hide resolved
...iceberg/src/main/java/io/trino/plugin/iceberg/catalog/nessie/IcebergNessieCatalogConfig.java Outdated Show resolved Hide resolved
...iceberg/src/main/java/io/trino/plugin/iceberg/catalog/nessie/IcebergNessieCatalogConfig.java Outdated Show resolved Hide resolved
...iceberg/src/main/java/io/trino/plugin/iceberg/catalog/nessie/IcebergNessieCatalogConfig.java Outdated Show resolved Hide resolved
...iceberg/src/main/java/io/trino/plugin/iceberg/catalog/nessie/IcebergNessieCatalogConfig.java Outdated Show resolved Hide resolved
...iceberg/src/main/java/io/trino/plugin/iceberg/catalog/nessie/IcebergNessieCatalogConfig.java Outdated Show resolved Hide resolved
...iceberg/src/main/java/io/trino/plugin/iceberg/catalog/nessie/IcebergNessieCatalogConfig.java Outdated Show resolved Hide resolved
...iceberg/src/main/java/io/trino/plugin/iceberg/catalog/nessie/IcebergNessieCatalogConfig.java Outdated Show resolved Hide resolved
docs/src/main/sphinx/connector/iceberg.rst Outdated Show resolved Hide resolved
@ajantha-bhat ajantha-bhat changed the title Iceberg: Support additional configurations for Nessie catalog Support additional configurations for Nessie catalog for Iceberg connector Jun 1, 2023
@ajantha-bhat ajantha-bhat changed the title Support additional configurations for Nessie catalog for Iceberg connector Support additional configurations for Nessie catalog in Iceberg connector Jun 1, 2023
@ajantha-bhat ajantha-bhat force-pushed the config branch 2 times, most recently from 824df79 to 24b4643 Compare June 2, 2023 07:44
@ajantha-bhat
Copy link
Member Author

@ebyhr: Thanks for an awesome review.

Please add tests (config test is insufficient especially for auth)

testDefaults checks for the NONE auth and testExplicitPropertyMapping checks for BEARER auth now.
If you meant an end-to-end integration test for auth. I think it is possible with Nessie container and keycloak docker. I am figuring it out.

@ajantha-bhat ajantha-bhat force-pushed the config branch 2 times, most recently from ac7f884 to 0f9f61e Compare June 5, 2023 02:04
@ajantha-bhat ajantha-bhat mentioned this pull request Jun 6, 2023
Open
4 tasks
@ajantha-bhat ajantha-bhat force-pushed the config branch 2 times, most recently from 1a1bfec to a23af13 Compare June 8, 2023 10:41
@ajantha-bhat
Copy link
Member Author

@ebyhr: The PR is ready. Can you please take another look? thanks.

@ajantha-bhat
Copy link
Member Author

Gentle reminder @ebyhr, @electrum

@ajantha-bhat ajantha-bhat mentioned this pull request Jun 21, 2023
Closed
@EternalDeiwos
Copy link

@ajantha-bhat thanks for adding this. BEARER auth is strictly necessary to use the Nessie catalog.

@ajantha-bhat
Copy link
Member Author

@ajantha-bhat thanks for adding this. BEARER auth is strictly necessary to use the Nessie catalog.

True. Nessie is usually deployed with Auth in production and this PR is definitely a high priority for Using Nessie + Trino in production environment.

@findinpath findinpath self-requested a review June 21, 2023 20:28
@ajantha-bhat
Copy link
Member Author

@findinpath: Thanks for self requesting the review.
Looking forward to the review and help in merging this.

@ajantha-bhat
Copy link
Member Author

I just rebased the PR since there was a conflict in pom file.

@ajantha-bhat
Copy link
Member Author

Just pushed the rebase (no conflicts). I will work on comments on the latest code.

@ajantha-bhat ajantha-bhat force-pushed the config branch 2 times, most recently from eeda12c to da29c08 Compare October 23, 2023 10:59
@ajantha-bhat
Copy link
Member Author

@ebyhr : Addressed all the comments except #17725 (comment) and #17725 (comment). (Both related to test refactoring)

Let me know how to proceed further.

@ajantha-bhat
Copy link
Member Author

I just did a rebase since there was conflict in pom.xml

@ajantha-bhat
Support additional configurations for Nessie catalog
26e5bce 
Nessie server configurations like read-timeout-ms, connect-timeout-ms
and enable-compression properties are exposed to have finer control
over the Nessie commits.
@ajantha-bhat
Copy link
Member Author

Rebased agian to resolve conflict from Iceberg 1.4.1 version bump.

@ajantha-bhat ajantha-bhat force-pushed the config branch 2 times, most recently from b78b5ae to 688d637 Compare October 31, 2023 09:03
@ajantha-bhat
Copy link
Member Author

ebyhr force-pushed the config branch from 688d637 to 602e03b
19 hours ago

LGTM.

@ajantha-bhat
Copy link
Member Author

Added an empty commit to retrigger the build as the testcase failure seems to be flaky (unrelated to Nessie)

@ebyhr
Copy link
Member

ebyhr commented Nov 2, 2023

CI hit #18134

@ajantha-bhat @ebyhr
Support authentication configurations for Nessie catalog
aee00b9 
Nessie server can be deployed with Authentication mode with keycloak.
So, need to expose the Nessie client configurations to handle the Auth.
@ebyhr ebyhr merged commit 73b1f75 into trinodb:master Nov 2, 2023
45 of 46 checks passed
@github-actions github-actions bot added this to the 432 milestone Nov 2, 2023
@ajantha-bhat
Copy link
Member Author

Thanks @ebyhr for the thorough review and the guidance.

@martint martint mentioned this pull request Nov 2, 2023
Merged
@colebow colebow mentioned this pull request Nov 2, 2023
Merged
martint added a commit to martint/trino that referenced this pull request Nov 2, 2023
@martint
Remove unnecessary logging
cd85703 
It generates unnecessary clutter when running tests.

Follow up to trinodb#17725
martint added a commit that referenced this pull request Nov 2, 2023
@martint
Remove unnecessary logging
406d0f8 
It generates unnecessary clutter when running tests.

Follow up to #17725
@mosabua
Copy link
Member

mosabua commented Nov 2, 2023

Congratulations @ajantha-bhat .. great job. Thank you so much to @ebyhr for all the help and guidance.

shreyas-dview pushed a commit to dview-io/trino that referenced this pull request Dec 12, 2023
@martint @shreyas-dview
Remove unnecessary logging
47d4fd9 
It generates unnecessary clutter when running tests.

Follow up to trinodb#17725
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ebyhr ebyhr ebyhr left review comments

@findinpath findinpath findinpath approved these changes

@dimas-b dimas-b dimas-b approved these changes

@mosabua mosabua mosabua approved these changes

Assignees
No one assigned
Labels
cla-signed docs iceberg Iceberg connector
Milestone
432
Development

Successfully merging this pull request may close these issues.
6 participants
@ajantha-bhat @EternalDeiwos @findinpath @ebyhr @mosabua @dimas-b