Sounds good. Regading 2, instead of using the lazy Guice injection,
you could use a Memoized supplier (which is pretty much the same but
perhaps a bit cleaner). Something like:
@Memoized
@Provides
protected final Supplier<CloseableHttpAsyncClient>
memoizedAyncClient(Supplier<CloseableHttpAsyncClient> asyncClient) {
return Suppliers.memoize(asyncClient);
}
@Provides
protected final Supplier<CloseableHttpAsyncClient> asyncClient(final
HttpUtils httpUtils) {
return new Supplier<CloseableHttpAsyncClient>() {
@Override
public CloseableHttpAsyncClient get() {
final HttpAsyncClientBuilder httpAsyncClientBuilder =
HttpAsyncClients.custom();
httpAsyncClientBuilder.setMaxConnTotal(httpUtils.getMaxConnections())
.setMaxConnPerRoute(httpUtils.getMaxConnectionsPerHost());
CloseableHttpAsyncClient httpAsyncClient =
httpAsyncClientBuilder.build();
httpAsyncClient.start();
return httpAsyncClient;
}
};
}
Then in the driver class you can inject the "@Memoized
Supplier<CloseableHttpAsyncClient> async" and everything should be ok.
On 21 April 2017 at 05:45, kishore kumar <kishore25ku...@gmail.com> wrote:
>
>
> On 2017-04-20 16:06 (+0530), Ignasi Barrera <n...@apache.org> wrote:
>> Thanks Spandan!
>>
>> I've had a quick look at the PR and it looks promising! Here are some
>> initial comments:
>>
>> * Do we need the Async response parser? Looking at the ETag implementation,
>> it looks like in most cases they will just wrap a regular response parser
>> in a future. We could just use regular response parsers in async methods
>> let the InvokeHttpMethod wrap them in a future if the method is anotated
>> @Async. That would simplify how methods are written and would remove the
>> need to duplicate all response parsers.
>> * In the Apache HTTP driver, you are initializing and starting the async
>> client in the constructor. I'm not very familiar with that driver, but what
>> does the "asyncClient.start()" method do? In most cases, the invoked
>> methods will be synchronous ones, and having the async client started by
>> default is kinda unnecessary? Could we start it the first time an async
>> request is made? Or lazy load it by providing a memoized supplier or
>> something like that?
>> * For (mostly informational) purposes such as logging, it would make sense
>> to propagate a "boolean async" flag to the HttpCommand object.
>> * The command executor service has several logic that is now duplicated in
>> the invoke and invokeAsync methods. It would be great if that could be
>> refactored so we don't have to apply changes to several places when
>> changing that class.
>> * Regarding the abstraction (BlobStore), instead of adding the methods
>> there and throwing an error by default, what about creating a subinterface
>> that provides just the async methods, so only the providers where we add
>> async implementations implement it?. A getAsyncBlobStore() method could be
>> added to the blobstore context (that method could easily fail if there is
>> no async implementation bound to the Guice context) and this way users will
>> get eh blobstore they want to work with, and that will only expose the
>> operations that are actually supported. Otherwise it is easier that errors
>> appear at runtime because users are calling methods that are not really
>> available.
>>
>>
>> Great job!
>>
>> On 20 April 2017 at 07:03, Spandan Thakur <spandan.thakur1...@gmail.com>
>> wrote:
>>
>> > Hi,
>> >
>> > We decided that we will take forward the true Async implementation in
>> > JClouds.
>> >
>> > According to your recommendations from the last time we posted our Async
>> > POC (last month in this dev list) we have done the following changes:
>> > 1. Isolated all the Apache HTTP client changes in drivers/apachehc.
>> > 2. Created a Async Response Parser and Async transformer.
>> > 3. Used Listenable Futures instead of Completable Futures.
>> >
>> > We were looking for early feedback as we continue moving forward. It will
>> > help us ensure we are on the right track before submitting a real pull
>> > request.
>> >
>> > You can find the changes here: https://github.com/
>> > SpandanThakur/jclouds/pull/2/files
>> >
>> > Do note we still have to write tests (with proper mocking), make getBlob
>> > Async as well, etc.
>> >
>> > Regards,
>> > Spandan Thakur
>> >
>>
>
> Hi Ignasi,
> Thanks for the feedback.
> 1. There is no need of async parser. I will refactor the code accordingly
> 2. asyncClient.start() will start a reactor thread which will be in infinite
> I/O select loop. I tried to create the instance on first access. I couldn't
> think of a better way than this.
> if (this.httpAsyncClient == null) {
> synchronized (this) {
> if(this.httpAsyncClient == null) {
> final HttpAsyncClientBuilder httpAsyncClientBuilder =
> HttpAsyncClients.custom();
>
> httpAsyncClientBuilder.setMaxConnTotal(httpUtils.getMaxConnections())
>
> .setMaxConnPerRoute(httpUtils.getMaxConnectionsPerHost());
> this.httpAsyncClient = httpAsyncClientBuilder.build();
> this.httpAsyncClient.start();
> }
> }
> }
> Any comments on this.
>
> 3. I am thinking of adding an overloaded constructor with async true or
> false. Is that ok?
> 4. I will see what refactoring can be done so that no to repeat the code.
> 5. I have to look into it.
>
> 1 and 2 are done and pushed into the repo
> https://github.com/SpandanThakur/jclouds/pull/2/files.
