Skip to content

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

Conversation

coutoPL
Copy link

@coutoPL coutoPL commented Jan 17, 2018

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 PropertiesAsyncHttpClientConfig class which implements AsyncHttpClientConfig interface and use Properties instance 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 PropertiesAsyncHttpClientConfig object which can be created in runtime from Properties class object.

@slandelle
Copy link
Contributor

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?

@coutoPL
Copy link
Author

coutoPL commented Jan 18, 2018

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:

  1. loading typesafe config
  2. dynamically converting typesafe config to java properties
  3. creating PropertiesAsyncHttpClientConfig
  4. 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.

@slandelle
Copy link
Contributor

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.

@coutoPL
Copy link
Author

coutoPL commented Jan 18, 2018

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?

@slandelle
Copy link
Contributor

slandelle commented Jan 18, 2018

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?

@coutoPL
Copy link
Author

coutoPL commented Jan 18, 2018

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?

@slandelle
Copy link
Contributor

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.

@coutoPL
Copy link
Author

coutoPL commented Jan 18, 2018

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.

@coutoPL
Copy link
Author

coutoPL commented Jan 18, 2018

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.

@slandelle
Copy link
Contributor

I still fail to understand how having a Config based implementation wouldn't meet your needs.

@coutoPL
Copy link
Author

coutoPL commented Jan 18, 2018

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

@slandelle
Copy link
Contributor

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.

@coutoPL
Copy link
Author

coutoPL commented Jan 18, 2018

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?

@slandelle
Copy link
Contributor

Will you accept solution providing org.typesafe.Config -> AsyncHttpClientConfig in extras module?

Absolutely

@coutoPL
Copy link
Author

coutoPL commented Jan 18, 2018

Ok, will implement it.

@coutoPL coutoPL closed this Jan 18, 2018
@coutoPL coutoPL deleted the feature/config_builder_enhacements branch January 20, 2018 18:44
@sirianni
Copy link

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 ☹️ and keep this up to date as the library evolves.

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);
  }

@coutoPL
Copy link
Author

coutoPL commented Jan 11, 2020

@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
Copy link

If someone knows of a different popular config library that produces java.util.Properties

Spring Boot?

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