Skip to content

Conversation

difin
Copy link
Contributor

@difin difin commented Oct 9, 2025

What changes were proposed in this pull request?

Added Java integration test for testing HMSRestCatalogClient with Oauth2 mode and changed existing RESCatalog client q-tests (those that use HMS and Gravitino as a server) to use Oauth2.

Why are the changes needed?

To test HMSRestCatalogClient with Oauth2 so that it could be used in production systems.

Does this PR introduce any user-facing change?

No, this PR only adds/updates tests.

How was this patch tested?

New and existing tests.

@difin difin force-pushed the oauth2_rest_catalog_client branch from b200287 to b5bccec Compare October 10, 2025 01:34
@okumin okumin changed the title HIVE-29020: Iceberg: Validate HMS REST Catalog Client with OAuth2 HIVE-29234: Iceberg: Validate HMS REST Catalog Client with OAuth2 Oct 10, 2025
Copy link
Contributor

@okumin okumin left a comment

Choose a reason for hiding this comment

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

Overall, +1

Please follow the addressable advice from SonarQube.
https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6124&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true

I changed the JIRA ticket number in the title because I suspect HIVE-29234 is probably the intended one. Please revert it if I'm wrong

.replace("OAUTH2_JWKS_URI", getJwksUri())
.replace("OAUTH2_CLIENT_ID", OAUTH2_SERVER_ICEBERG_CLIENT_ID)
.replace("OAUTH2_CLIENT_SECRET", OAUTH2_SERVER_ICEBERG_CLIENT_SECRET)
// .replace("localhost", "host.docker.internal")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, it was a leftover that I didn't notice, done.

Copy link

@difin difin merged commit ad55d58 into apache:master Oct 13, 2025
2 checks passed
@difin
Copy link
Contributor Author

difin commented Oct 13, 2025

Thanks for core review, @okumin!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants