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

API: Add a config nessie.client-api-version #6917

Closed
wants to merge 1 commit into from

Conversation

ajantha-bhat
Copy link
Contributor

needed for exposing client API version at Iceberg side: apache/iceberg#6712

@ajantha-bhat ajantha-bhat changed the title Add a config nessie.client-api-version API: Add a config nessie.client-api-version May 26, 2023
* Config property name ({@value #CONF_NESSIE_CLIENT_API_VERSION}) for specifying the client API
* version.
*/
public static final String CONF_NESSIE_CLIENT_API_VERSION = "nessie.client-api-version";
Copy link
Member

Choose a reason for hiding this comment

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

I suppose for the sake of consistency we should respect this option in the Nessie client builder. For example, automatically choose/validate the API class during client construction when it is set.

Copy link
Member

Choose a reason for hiding this comment

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

Specifically, I suppose with nessie.client-api-version set to 2 we can automatically promote the result of HttpClientBuilder.build(NessieApiV1.class) to be a NessieApiV2.

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't there a conflict lurking here between setting the api version with a configuration option vs. setting the api version through the parameter passed to HttpClientBuilder.build(Class<NessieApi>)? Imho the proper way to move forward would be to deprecate HttpClientBuilder.build(Class<NessieApi>) and create a new HttpClientBuilder.build() method the honours both the new and old ways.

Copy link
Member

@dimas-b dimas-b May 30, 2023

Choose a reason for hiding this comment

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

I think having a build(Class<NessieApi>) is still useful for proper type checks on the clients side, especially when the client specifically wants NessieApiV2.

My idea was that version-agnostic clients (like the NessieCatalog in Iceberg) would call build(NessieApiV1.class) since they are compiled to that interface, but use the config property for controlling actual wire format.

Note: API v2 can represent API v1 requests and responses correctly in java.

@ajantha-bhat
Copy link
Contributor Author

@dimas-b:

I agree that we could add some more checks on the client side using this property.

But can that be handled in a follow-up? Because I didn't get time to explore this and merging this for 0.61.0 release will definitely help in simplifying the Iceberg side PR and getting it merged.

@dimas-b
Copy link
Member

dimas-b commented Jun 12, 2023

But can that be handled in a follow-up? Because I didn't get time to explore this and merging this for 0.61.0 release will definitely help in simplifying the apache/iceberg#6712 and getting it merged.

Having a constant in Nessie code, whose only use case is in Iceberg code, looks odd to me. Iceberg code can already utilize this config setting, so no functionality is affected, as far as I can tell.

@ajantha-bhat
Copy link
Contributor Author

Having a constant in Nessie code, whose only use case is in Iceberg code, looks odd to me.

we already have something like that. CONF_NESSIE_CLIENT_BUILDER_IMPL.
So, I was just following the style.

public static final String CONF_NESSIE_CLIENT_BUILDER_IMPL = "nessie.client-builder-impl";

@ajantha-bhat
Copy link
Contributor Author

Currently no need of this and Iceberg side PR got merged.

In future if, we really need a config level control for controlling write path, we can open up this again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants