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 >
