nacx commented on this pull request.
Thanks, @kishore25kumar!
I've added a couple comments for discussion but in general, it looks good!
> super(backend, backendType);
this.consistencyModel = checkNotNull(consistencyModel,
"consistencyModel");
this.blobStore = checkNotNull(blobStore, "blobStore");
this.utils = checkNotNull(utils, "utils");
this.blobRequestSigner = checkNotNull(blobRequestSigner,
"blobRequestSigner");
+ Binding<AsyncBlobStore> asyncBlobStoreBinding =
injector.getExistingBinding(Key.get(AsyncBlobStore.class));
+ if (asyncBlobStoreBinding != null) {
+ asyncBlobStore = asyncBlobStoreBinding.getProvider().get();
+ } else {
+ asyncBlobStore = null;
+ }
Instead of resolving the injection in the constructor, better bind directly an
`Optional<AsyncBlobStore>` in a Guice module like we do for the [compute
extensions](https://github.com/jclouds/jclouds/blob/master/compute/src/main/java/org/jclouds/compute/config/BaseComputeServiceContextModule.java#L301-L321).
This way you can directly inject the Optional and return it in the getter
without the need for null checks after construction.
> + setResultOrException(command, finalFuture, response);
+ } else {
+ cleanup(nativeRequest);
+ setResultOrException(command, finalFuture, response);
+ }
+ }
+
+ @Override
+ public void onFailure(Throwable t) {
+ handleInvokeAsyncFailure(t, command, finalFuture);
+ }
+ });
+ } catch (Exception e) {
+ handleInvokeAsyncFailure(e, command, finalFuture);
+
+ }
I'm a bit concerned here about the duplicated logic: the retry logic and
exception handling duplicate the one in the sync invoke method, with small
differences to accommodate the async futures. Would it be worth refactoring the
`sync` implementation to use, say, the Guava's
[ImmediateFuture](https://google.github.io/guava/releases/19.0/api/docs/com/google/common/util/concurrent/Futures.html#immediateFuture(V))
so we can have just one implementation of both approaches? Would this
introduce a non-negligible overhead to the `sync` methods (the common use-case)
to make it not worth it?
> @@ -127,6 +128,11 @@ protected HttpResponse invoke(HttpURLConnection
> connection) throws IOException,
}
@Override
+ protected ListenableFuture<HttpResponse> invokeAsync(final
HttpURLConnection nativeRequest) {
+ throw new UnsupportedOperationException("unsupported operation");
Add some more information to let users know that what is not supported are the
Async calls in general, and suggest using a different HTTP driver.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1114#pullrequestreview-45343810
