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 Thu, 06 Jul 2017 07:01:55 GMT
nacx commented on this pull request.



>        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;
+      }

Apologies for the late reply.

The idea is to create a base module class for all blobstore context modules. Something like:

```java
public abstract class BaseBlobStoreServiceContextModule extends AbstractModule {
   @Override
   protected void configure() {
   }

   @Provides
   @Singleton
   protected final Optional<AsyncBlobStore> provideAsyncBlobStore(Injector i) {
      Binding<AsyncBlobStore> binding = i.getExistingBinding(Key.get(AsyncBlobStore.class));
      return binding == null ? Optional.<AsyncBlobStore> absent() : Optional.of(binding.getProvider().get());
   }
}
```
Then you can have the `BlobStoreContextImpl` directly inject the `Optional<AsyncBlobStore>`
like this. The previous module populates an optional to the Guice injector based on the presence
of a binding for the `AyncBlobStore`. Whether is present or not, the base module class configures
the optional in a generic way, and you can have it directly injected in your class, without
having to resolve the binding in the constructor:

```java
@Singleton
public class BlobStoreContextImpl extends BaseView implements BlobStoreContext {
   ...
   private final Optional<AsyncBlobStore> asyncBlobStore;

   @Inject
   public BlobStoreContextImpl(@Provider Context backend, @Provider TypeToken<? extends
Context> backendType,
         Utils utils, ConsistencyModel consistencyModel, BlobStore blobStore, BlobRequestSigner
blobRequestSigner,
         Optional<AsyncBlobStore> asyncBlobStore) {
      ...
      this.asyncBlobStore = asyncBlobStore;
   }
}
```
This configuration will cause the `BlobStoreContextImpl` to get an `absent()` injected for
all providers. The only thing that has to be done in those providers where we support async,
is to bind the async implementation. In Azure:
```java
public class AzureBlobStoreContextModule extends BaseBlobStoreServiceContextModule {
   @Override
   protected void configure() {
      ...
      bind(AsyncBlobStore.class).to(AsyncAzureBlobStore.class).in(Scopes.SINGLETON);
   }
}
```

So, with the proposed change:

* We have a Guice module that configures the `Optional<AsyncBlobStore>` by default.
* Modules that support async only have to define the binding. A proper optional will be always
injected to the `BlobStoreContextImpl` (modules not defining the binding will get an `absent`
injected).

The only implication of this approach is that **all blobstore modules, of all providers**
(whether they use async or not) need to be changed to extend the new `BaseBlobStoreServiceContextModule`
module since it defines the Guice binding for the optional, which is now needed by the `BlobStoreContextImpl`
constructor.
This implies small changes in all blobstore providers but IMO it is a cleaner implementation
and it makes sense to have a base class for all blobstore Guice modules where we can define
common behavior.

Does this make sense?

-- 
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#discussion_r125824375
Mime
View raw message