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

Feature Proposal: Setting Connector Class using Client Configuration #5329

Closed
mkarg opened this issue May 12, 2023 · 17 comments · Fixed by #5345
Closed

Feature Proposal: Setting Connector Class using Client Configuration #5329

mkarg opened this issue May 12, 2023 · 17 comments · Fixed by #5345

Comments

@mkarg
Copy link
Member

mkarg commented May 12, 2023

Description

I like to propose a new configuration property that one can provide to ClientBuilder for specifying a container implementation when building a Client instance:

  • org.glassfish.jersey.client.ClientProperties.CONNECTORPROVIDER_CLASS - Defines the implementation of ConnectorProvider to bootstrap. By default auto-selects the first connector provider found. The name of the configuration property is "jersey.config.server.bootstrap.connectorprovider.class".

Usage Example

var clientBuilder = ClientBuilder.newBuilder();
clientBuilder.property(CONNECTORPROVIDER_CLASS, "org.glassfish.jersey.netty.connector.NettyConnectorProvider");
var client = clientBuilder.build(); // client is using Netty connector

Justification

For maximum portability of applications, setting connector class should be possible without using vendor-specific API, to allow writing vanilla JAX-RS client code that is configured solely by means of Class-Path replacements plus external configuration. Hence changing the runtime / configure the runtime can be the deployer's / admin's job, instead of the programmer's job. While in DevOps times both jobs often are consolidated in one person, there still are lots of environments running separate roles. We should be able to serve their requirement for using Netty etc. without code change.

@mkarg
Copy link
Member Author

mkarg commented May 12, 2023

@jansupol We started a discussion about such a new property out-of-bands, but I think it is valuable to restart here in a public place, so everybody can follow the arguments from the beginning.

@jansupol
Copy link
Contributor

Sure.

The ConnectorProvider can be chosen via a META-INF/services/org.glassfish.jersey.client.spi.ConnectorProvider. Having it set by a property duplicates the functionality and it is unclear which option should be preferred. This way, it is possible doing without using vendor-specific API as much as using a property. Both are ignored by other vendors.

@jansupol
Copy link
Contributor

On the other hand, using a property would have been more systematic (as much is configured by properties), and multiple clients each using a different connector could be used.

@jansupol
Copy link
Contributor

The problem can be instantiation, at this stage, the injection framework responsible for instantiations is not ready yet for instantiating the provider.

@mkarg
Copy link
Member Author

mkarg commented May 12, 2023

The ConnectorProvider can be chosen via a META-INF/services/org.glassfish.jersey.client.spi.ConnectorProvider.

Why don't we simply add such a file into each connector JAR then, so selection would be as easy as putting the connector JAR on the classpath? 🤔

@mkarg
Copy link
Member Author

mkarg commented May 12, 2023

The problem can be instantiation, at this stage, the injection framework responsible for instantiations is not ready yet for instantiating the provider.

We could simply forward the String value set into this new property to the code location where Jersey already considers META-INF/services/org.glassfish.jersey.client.spi.ConnectorProvider. If both are given, the property is prefered.

@jansupol
Copy link
Contributor

Why don't we simply add such a file into each connector JAR then, so selection would be as easy as putting the connector JAR on the classpath? 🤔

This would have been a behavior (backward incompatible) change. Two clients, one using the default connector and one fancy would after this change both use the non-default connector.

@mkarg
Copy link
Member Author

mkarg commented May 12, 2023

Behavior change is acceptable for major releases, so we can do it for Jersey 4.

If these two clients do not want to use that non-defaut connector, why did the deployer put that jar on the classpath then?

@jansupol
Copy link
Contributor

If these two clients do not want to use that non-defaut connector, why did the deployer put that jar on the classpath then?

Perhaps they use two clients, each with a different connector?

@mkarg
Copy link
Member Author

mkarg commented May 13, 2023

Perhaps they use two clients, each with a different connector?

Aren't these mostly edge-cases which we could simply ask to set the new proposed property to overrul the service loader's first choice once they migrate to the next feature release of Jersey?

@zUniQueX
Copy link
Contributor

Perhaps they use two clients, each with a different connector?

I think this will be more common in projects with multiple modules. If a transitive dependency uses one connector JAR, the connector will be used for all other clients.

Using exclusions or setting the new property in each inheriting JAR could be tedious.

@mkarg
Copy link
Member Author

mkarg commented Jun 1, 2023

Using exclusions or setting the new property in each inheriting JAR could be tedious.

Just to understand your claim correctly: You actually want to say that adding an additional service file is less tedious that setting an additional property? I do not share this experience, as setting a property can be done at runtime simply by configuration, while adding a file demands a repackaging and redeployment of the JAR.

@zUniQueX
Copy link
Contributor

zUniQueX commented Jun 1, 2023

Just to understand your claim correctly: You actually want to say that adding an additional service file is less tedious that setting an additional property? I do not share this experience, as setting a property can be done at runtime simply by configuration, while adding a file demands a repackaging and redeployment of the JAR.

My claim is that opting this out at every client instantiation can be tedious. The default connector is fine for most use cases, so I'd prefer an opt-in.

@mkarg
Copy link
Member Author

mkarg commented Jun 2, 2023

If these two clients do not want to use that non-defaut connector, why did the deployer put that jar on the classpath then?

Perhaps they use two clients, each with a different connector?

If these clients do not want to use the different connectors but want to use the default connector, why did the deployer put different connectors on the classpath then?

@zUniQueX
Copy link
Contributor

zUniQueX commented Jun 2, 2023

If these clients do not want to use the different connectors but want to use the default connector, why did the deployer put different connectors on the classpath then?

If the project uses a library, which internally uses a non-default connector, that connector JAR would be available on the classpath. Using an exclusion for the connector artifact isn't always possible, because e.g. the Apache connector allows high customization of the client through the customizer class. The connector JAR then cannot be removed from the classpath because the customizer class then wouldn't be present.

@mkarg
Copy link
Member Author

mkarg commented Jun 2, 2023

If the project uses a library, which internally uses a non-default connector, that connector JAR would be available on the classpath. Using an exclusion for the connector artifact isn't always possible, because e.g. the Apache connector allows high customization of the client through the customizer class. The connector JAR then cannot be removed from the classpath because the customizer class then wouldn't be present.

Understood, but actually I assume that neither the application developer nor the library vendor would assume or explicitly know that in this situation you end up with the default connector getting used by the library. To match your assumption, the current behaviour must be prominently told in the docs, which I do not see. IMHO most application programmers simply miss this fact.

The library vendor has deliberately chosen a particular connector for a good reason, and as it currently is getting used only when explicitly configuring Jersey de facto that library does already set an explicit selection property (otherwise the lib never would have loaded that particular connector but would get the default connector loaded, correct?). The application developer OTOH relied on the default, and what "the default" means is rather undocumented and may change in major releases of Jersey (as people assume SemVer these days), so he implicitly agreed to not wanting a particuar implementation, but being fine with any default - even with changing defaults. In fact, "using the default" means that the application developer is happy with any connector, because if he wants explicitly one particular connector (even if it is the default one) then he would also have set an explicit property. So in this scenario, I would rather say the application programmer was expecting to get the lib's connector in his app, and he neither expected to get different connectors nor to get the default being used by the lib.

Nevertheless, the scenario proofs that we MUST have a property for the selection of the connector, as using the service file (as proposed by @jansupol) explicitly would use a SINGLE connector for app and lib.

@zUniQueX
Copy link
Contributor

zUniQueX commented Jun 2, 2023

Nevertheless, the scenario proofs that we MUST have a property for the selection of the connector, as using the service file (as proposed by @jansupol) explicitly would use a SINGLE connector for app and lib.

I agree with that. This wouldn't introduce any undesired behaviour through trasitive dependencies but only change the connector registration strategy 👍

@jansupol jansupol linked a pull request Jun 12, 2023 that will close this issue
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 a pull request may close this issue.

3 participants