-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Feature/config builder enhacements #1499
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/config builder enhacements #1499
Conversation
TBH, I don’t see much value in the changes you suggest.
I absolutely want to keep core AHC dependencies minimal, ie only Netty.
But AsyncHttpClientConfig is an interface. Why not add an alternative Typesafe config based implementation in the extras modules?
Ok, I'll show you my case.
Async http client config can looks like:
org.asynchttpclient.threadPoolName=AsyncHttpClient
org.asynchttpclient.maxConnections=-1
org.asynchttpclient.maxConnectionsPerHost=-1
org.asynchttpclient.connectTimeout=5000
org.asynchttpclient.pooledConnectionIdleTimeout=60000
...
I'm using typesafe config in my scala project. I'd like to have configuration of my http client as follows:
http-client {
threadPoolName=AsyncHttpClient
maxConnections=-1
maxConnectionsPerHost=-1
connectTimeout=5000
pooledConnectionIdleTimeout=60000
...
}
I'm aware that there is AsyncHttpClientConfig interface and I can implement it in my project. But it is tedious. But guessed that you don't want to have unnecessary dependencies in you project. It's ok and I fully understand it.
But still, there should be a way to dynamically convert typesafe config (or any other lib config) to some abstract/base format which is understandable by asynchttpclient. I've noticed that you use java properties. I can do such conversion (typesafe config -> java properties) on my side. But in async http client there is no way to pass Properties instance to builder. But there is a Builder constructor which accepts AsyncHttpClientConfig. And I've decided to create PropertiesAsyncHttpClientConfig which can be instantiated with Properties and implements AsyncHttpClientConfig. It also uses same property names as AsyncHttpClientConfigDefaults and take advantage of its default values.
Now my case can be resolved in following way:
- loading typesafe config
- dynamically converting typesafe config to java properties
- creating
PropertiesAsyncHttpClientConfig - initializing AsyncHttpClient Builder with created config
And as you see, I don't have to implement AsyncHttpClientConfig in my code. AsyncHttpClient code don't need any new dependencies.
I’m not telling you to implement it in your project but contribute it as a sub module (see extras directory). An alternative implementation that would take a Config input would surely benefit other people.
Convertion between typesafe config and java properties can be done with https://github.com/andr83/scalaconfig. But still, to achieve that I need a way to intialialize Builder with Properties instance.
So, after my explanations, do you understand my point of view? Does the PR make sense for you now?
No, I really don't.
Instead of adding a new PropertiesAsyncHttpClientConfig(java.util.Properties) that no one but you would use and that would still require you to transform from com.typesafe.config.Config to java.util.Properties thanks to an additional library, why not directly add a new TypesafeConfigAsyncHttpClientConfig(com.typesafe.config.Config), all the more as such implementation could easily read ahc-default.properties?
Because this solution is more generic and you will be able to initialize AHC with any config lib you use, only by providing generic converter from {ANY CONFIG LIB} to java.Properties.
Then you can add to extras ready-to-use wrapper from org.typesafe.Config to AsyncHttpClientConfig which will integrate eg. https://github.com/andr83/scalaconfig with PropertiesAsyncHttpClientConfig. Two generic solutions integrated to achieve a goal instead one for specific case.
The only thing I need from AHC is to create AsyncHttpClientConfig from java.Properties.
Why do you think this is a problem?
Because this solution is more generic and you will be able to initialize AHC with any config lib you use
IMO, Typesafe config doesn't have much competitors and is becoming a standard.
If someone knows of a different popular config library that produces java.util.Properties, please chime in.
Then you can add to extras ready-to-use wrapper from org.typesafe.Config to AsyncHttpClientConfig which will integrate eg. https://github.com/andr83/scalaconfig
There won't ever be an official integration for any Typesafe config Scala wrappers.
First, AHC is a Java lib.
Then, there's a new Typesafe config Scala wrapper every month.
If your Scala wrapper doesn't expose the underlying com.typesafe.config.Config, that's a pity and probably something that should be added there.
Why do you think this is a problem?
Because I don't want to introduce APIs that are not used or don't meet the actual need. My feeling is the actual need is to do Config -> AHCConfig, not Config -> Properties -> AHCConfig.
OK, so, forget about typesafe config.
Suppose, I'd like to create java.Properties in runtime and initialize AHC with it. At the moment I cannot do that. I can initialize AHC using file with properties. My solution will allow me to do so.
We have many applications in java and scala, where we'd like to use AHC. Dynamic initialization at the moment is only one thing which is missing and block us to use it.
I still fail to understand how having a Config based implementation wouldn't meet your needs.
Other example:
suppose I have all AHC properties exposed in web gui. And eg. I have 'maxConnection' field where I can set some number, click save and my backend application have to reconfigure its AHC. Now I have to implement AsyncHttpClientConfig, because there is no possibility to initilize AHC Builder in other way dynamically. But I would like to create some object, initialize it in runtime and then create AHC with this object. I noticed that you use java.Properties to do that underneath. If only I could pass my java.Properties object ...
Look, if you were to have an AsyncHttpClientConfig implementation that can be built from a com.typesafe.config.Config, you could achieve the same thing you're trying to do (introduction an AsyncHttpClientConfig implementation that can be built from a java.util.properties) with com.typesafe.config.ConfigFactory#parseProperties(java.util.Properties).
DefaultAsyncHttpClientConfig is a kind of poor man Typesafe Config so AHC doesn't have any dependencies. But if one really wants additional config features, providing a Typesafe Config based alternative implementation is the way to go.
ok, so to be clear current solution is not acceptable (honestly, don't understand why, because it uses all mechanisms proposed by you and changes almost nothing). But OK. Will you accept solution providing org.typesafe.Config -> AsyncHttpClientConfig in extras module?
Will you accept solution providing org.typesafe.Config -> AsyncHttpClientConfig in extras module?
Absolutely
Ok, will implement it.
sirianni
commented
Jan 11, 2020
Just came across this. Thanks @coutoPL for proposing PropertiesAsyncHttpClientConfig and patiently explaining the use case. Sad to see that this PR was not merged
My use case is to initialize an AsyncHttpClientConfig from a kubernetes configmap (via a helm chart) using YAML. We are using dropwizard. I'd like to bind a YAML map to a java.util.Properties object using jackson and then use that to instantiate an AsyncHttpClientConfig. Unfortunately to do that now I'd need to implement my own AsyncHttpClientConfig subclass with 50 methods
The library already have the ability to translate from java.util.Properties to an AsyncHtttpClientConfig object in order to support the ahc-defaults mechanism. All that's being asked is to refactor that capability so that it's exposed to clients. Right now the logic is strangely inverted so that it's coupled to the defaults concept instead of just using the setters
public static String defaultUserAgent() {
return AsyncHttpClientConfigHelper.getAsyncHttpClientConfig().getString(ASYNC_CLIENT_CONFIG_ROOT + USER_AGENT_CONFIG);
}
@sirianni typesafe config is not the standard for you? :P I'm glad that you share my point. I wanted to provide solution that could solve the problem in more generic (standard) way, but we ended up with sth like this: #1500
So, now you have to generate typesafe config from your yaml and feed the ahc or write your own extras-integration :P
sirianni
commented
Jan 14, 2020
If someone knows of a different popular config library that produces java.util.Properties
Spring Boot?
At the moment there is no option for load config from other source than java properties. It would be acceptable if I could convert on fly eg. typesafe config to java properties and apply it while creating Async Http Client instance.
So, I've added
PropertiesAsyncHttpClientConfigclass which implementsAsyncHttpClientConfiginterface and usePropertiesinstance as a store for settings (with fallback to default ones when some settings aren't set).From now, anyone can use auxiliary Builder constructor and pass to it
PropertiesAsyncHttpClientConfigobject which can be created in runtime fromPropertiesclass object.