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.

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

Nessie: Infer default API version from URI #9459

Merged
merged 3 commits into from
Jan 16, 2024
Merged

Conversation

ajantha-bhat
Copy link
Member

@ajantha-bhat ajantha-bhat commented Jan 11, 2024

Currently default version is v1.
So, If users need V2, they need to configure v2 URI and set the client-api-version to "2".

With this change, just configuring v2 URI is enough and default version will be inferred from URI.
client-api-version can still be used for some special cases where custom URI doesn't end with version number.

Copy link
Contributor

@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.

LGTM 👍 Thanks, @ajantha-bhat !

throw new IllegalArgumentException(
String.format(
"URI doesn't end with the version: %s. "
+ "Please configure `client-api-version` in the catalog properties explicitly.",
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder whether we can just assume v2 in this case... but I'm fine with throwing an exception too.

Copy link
Member Author

Choose a reason for hiding this comment

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

If the uri is not ending versions, then it can be a custom implementation which can be based on v1 or v2. So, I didn't want to assume the version there.

Copy link
Contributor

@nastra nastra left a comment

Choose a reason for hiding this comment

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

I think it would be good to add some tests around this (especially with an invalid URI). Additionally, it would be good to configure both the client-api-version and a URI with v1/v2 to make sure the right setting takes precedence

temp.toUri().toString(),
"client-api-version",
apiVersion == NessieApiVersion.V2 ? "2" : "1");
temp.toUri().toString());
Copy link
Member Author

Choose a reason for hiding this comment

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

Infers API version using URI.

@ajantha-bhat
Copy link
Member Author

I think it would be good to add some tests around this (especially with an invalid URI). Additionally, it would be good to configure both the client-api-version and a URI with v1/v2 to make sure the right setting takes precedence

done.

@nastra nastra merged commit 8018ab8 into apache:main Jan 16, 2024
41 checks passed
geruh pushed a commit to geruh/iceberg that referenced this pull request Jan 26, 2024
adnanhemani pushed a commit to adnanhemani/iceberg that referenced this pull request Jan 30, 2024
devangjhabakh pushed a commit to cdouglas/iceberg that referenced this pull request Apr 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants