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.
