jclouds-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ignasi Barrera <notificati...@github.com>
Subject Re: [jclouds/jclouds] Async interface changes for putBlob (#1114)
Date Wed, 21 Jun 2017 07:28:13 GMT
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
Mime
View raw message