You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jclouds.apache.org by Shri Javadekar <no...@github.com> on 2013/12/03 00:30:32 UTC
[jclouds] Remove LocalAsyncBlobStore. (#220)
Requests from LocalBlobStore were being forwarded to
LocalAsyncBlobStore. Since the async blobstore is going to be removed, this
commit removes the LocalAsyncBlobStore and the corresponding
LocalAsyncBlobStore <-> LocalBlobStore binding. It ports the
functionality from LocalAsyncBlobStore to LocalBlobStore.
Verified that unit tests for the transient blobstore, filesystem blobstore,
swift, atmos and s3 complete successfully.
The main changes are in *ContextModule.java and BinderUtils.java for
the bindings. The other big change is in LocalBlobStore.java. Other
files have no functional changes.
Also, this will have to be rebased if pull request #126 [1] goes in
before this.
[1] https://github.com/jclouds/jclouds/pull/126
You can merge this Pull Request by running:
git pull https://github.com/maginatics/jclouds del-localasyncblobstore
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds/pull/220
-- Commit Summary --
* Remove LocalAsyncBlobStore.
-- File Changes --
M apis/atmos/src/test/java/org/jclouds/atmos/internal/StubAtmosAsyncClient.java (53)
M apis/filesystem/src/main/java/org/jclouds/filesystem/config/FilesystemBlobStoreContextModule.java (12)
D apis/filesystem/src/test/java/org/jclouds/filesystem/FilesystemAsyncBlobStoreTest.java (865)
M apis/s3/src/main/java/org/jclouds/s3/blobstore/internal/S3BlobStoreContextImpl.java (12)
M apis/s3/src/test/java/org/jclouds/s3/internal/StubS3AsyncClient.java (44)
M apis/swift/src/test/java/org/jclouds/openstack/swift/internal/StubSwiftAsyncClient.java (39)
D blobstore/src/main/java/org/jclouds/blobstore/LocalAsyncBlobStore.java (552)
M blobstore/src/main/java/org/jclouds/blobstore/config/LocalBlobStore.java (522)
M blobstore/src/main/java/org/jclouds/blobstore/config/TransientBlobStoreContextModule.java (11)
M blobstore/src/main/java/org/jclouds/blobstore/internal/BlobStoreContextImpl.java (7)
M blobstore/src/main/java/org/jclouds/blobstore/strategy/internal/MarkersDeleteDirectoryStrategy.java (22)
M blobstore/src/test/java/org/jclouds/blobstore/TransientBlobRequestSignerTest.java (6)
M core/src/main/java/org/jclouds/rest/config/BinderUtils.java (26)
M providers/aws-s3/src/main/java/org/jclouds/aws/s3/blobstore/internal/AWSS3BlobStoreContextImpl.java (12)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/220.patch
https://github.com/jclouds/jclouds/pull/220.diff
Re: [jclouds] Remove LocalAsyncBlobStore. (#220)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #885](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/885/) UNSTABLE
Looks like there's a problem with this pull request
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/220#issuecomment-36658910
Re: [jclouds] Remove LocalAsyncBlobStore. (#220)
Posted by Andrew Phillips <no...@github.com>.
> * @author Alfredo "Rainbowbreeze" Morresi
> */
> public class FilesystemBlobStoreContextModule extends AbstractModule {
>
> @Override
> protected void configure() {
> - bind(AsyncBlobStore.class).to(LocalAsyncBlobStore.class).asEagerSingleton();
> - // forward all requests from TransientBlobStore to TransientAsyncBlobStore. needs above binding as cannot proxy a class
> - bindSyncToAsyncApi(binder(), LocalBlobStore.class, AsyncBlobStore.class);
> - bind(BlobStore.class).to(LocalBlobStore.class);
> + bind(BlobStore.class).to(LocalBlobStore.class).in(Scopes.SINGLETON);
Any reason for the change in binding? Or is this to mirror the `asEagerSingleton()` for `LocalAsyncBlobStore`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/220/files#r8053739
Re: [jclouds] Remove LocalAsyncBlobStore. (#220)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1107](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1107/) UNSTABLE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/220#issuecomment-36686460
Re: [jclouds] Remove LocalAsyncBlobStore. (#220)
Posted by Andrew Phillips <no...@github.com>.
> @@ -137,7 +141,8 @@ public URI apply(String from) {
> public ListenableFuture<Void> deletePath(String path) {
> if (path.indexOf('/') == path.length() - 1) {
> // chop off the trailing slash
> - return Futures.transform(blobStore.deleteContainerIfEmpty(path.substring(0, path.length() - 1)),
> + return Futures.transform(immediateFuture(blobStore
> + .deleteContainerIfEmpty(path.substring(0, path.length() - 1))),
[minor] Move to previous line or wrap at `path.substring`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/220/files#r13571614
Re: [jclouds] Remove LocalAsyncBlobStore. (#220)
Posted by Shri Javadekar <no...@github.com>.
> @@ -80,9 +79,8 @@
> @Inject
> MarkersDeleteDirectoryStrategy(
> @Named(Constants.PROPERTY_USER_THREADS) ListeningExecutorService userExecutor,
> - AsyncBlobStore ablobstore, BlobStore blobstore) {
> + BlobStore blobstore) {
Sorry about this. I'm finding it a little hard to make Eclipse understand the jclouds coding standard. Some of these are fallouts of that.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/220/files#r8087114
Re: [jclouds] Remove LocalAsyncBlobStore. (#220)
Posted by Shri Javadekar <no...@github.com>.
> @@ -72,7 +72,9 @@
> private final Closer closer;
>
> @Inject
> - private StubAtmosAsyncClient(LocalAsyncBlobStore blobStore, AtmosObject.Factory objectProvider,
> + private StubAtmosAsyncClient(
> + LocalBlobStore blobStore,
> + AtmosObject.Factory objectProvider,
Done.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/220/files#r8086965
Re: [jclouds] Remove LocalAsyncBlobStore. (#220)
Posted by Andrew Phillips <no...@github.com>.
> @@ -72,7 +69,7 @@ public BlobStore getBlobStore() {
>
> @Override
> public AsyncBlobStore getAsyncBlobStore() {
> - return ablobStore;
> + return null;
> }
Should this PR then add a `@Deprecated` or so, explaining not to call this? Or at least some Javadoc? Or is the intention to remove this practically immediately afterwards, so there's little risk of users blowing up here?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/220/files#r13571778
Re: [jclouds] Remove LocalAsyncBlobStore. (#220)
Posted by Andrew Phillips <no...@github.com>.
> + if (!options.isDetailed()) {
> + for (StorageMetadata md : contents) {
> + md.getUserMetadata().clear();
> + }
> + }
> + }
> +
> + return new PageSetImpl<StorageMetadata>(contents, marker);
> +
> + }
> +
> + private ContainerNotFoundException cnfe(final String name) {
> + return new ContainerNotFoundException(name, String.format(
> + "container %s not in %s", name,
> + storageStrategy.getAllContainerNames()));
> + }
Inline this method? Or at least rename to `makeContainerNotFoundException` or so? I know it's "inherited code", but... ;-)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/220/files#r8053853
Re: [jclouds] Remove LocalAsyncBlobStore. (#220)
Posted by Shri Javadekar <no...@github.com>.
Closed #220.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/220#event-147810874
Re: [jclouds] Remove LocalAsyncBlobStore. (#220)
Posted by Shri Javadekar <no...@github.com>.
> }
> if (options.getIfModifiedSince() != null) {
> Date modifiedSince = dateService.rfc822DateParse(options.getIfModifiedSince());
> if (modifiedSince.after(object.getMetadata().getLastModified()))
> - return immediateFailedFuture(LocalAsyncBlobStore.returnResponseException(412));
> + return immediateFailedFuture(LocalBlobStore
> + .returnResponseException(412));
Done.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/220/files#r8087016
Re: [jclouds] Remove LocalAsyncBlobStore. (#220)
Posted by Shri Javadekar <no...@github.com>.
> * @author Adrian Cole
> */
> // NOTE:without testName, this will not call @Before* and fail w/NPE during surefire
> @Test(groups = "unit", testName = "TransientBlobRequestSignerTest")
> -public class TransientBlobRequestSignerTest extends BaseAsyncClientTest<LocalAsyncBlobStore> {
> +public class TransientBlobRequestSignerTest extends
> + BaseAsyncClientTest<LocalBlobStore> {
Done. BaseAsyncClientTest extends BaseAsyncApiTest which in turn extends BaseRestApiTest. AFAICT there isn't anything about BaseAsyncClientTest (everything I see it does is done synchronously). I think this may have been named Async since the tests built on top this were supposed to be async. But I may be wrong.
Happy to change this in a separate commit. For now, would prefer this commit be only about LocalAsyncBlobStore <-> LocalBlobStore.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/220/files#r8087170
Re: [jclouds] Remove LocalAsyncBlobStore. (#220)
Posted by Shri Javadekar <no...@github.com>.
> @@ -41,7 +41,7 @@
> import org.jclouds.atmos.domain.UserMetadata;
> import org.jclouds.atmos.options.ListOptions;
> import org.jclouds.atmos.options.PutOptions;
> -import org.jclouds.blobstore.LocalAsyncBlobStore;
> +import org.jclouds.blobstore.config.LocalBlobStore;
No idea. I can move it out of config in a separate change.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/220/files#r8086952
Re: [jclouds] Remove LocalAsyncBlobStore. (#220)
Posted by Andrew Phillips <no...@github.com>.
> +
> + /**
> + * Override parent method because it uses strange futures and listenables
> + * that creates problem in the test if more than one test that deletes the
> + * container is executed
> + *
> + * @param container
> + * @return
> + */
> + @Override
> + public void deleteContainer(final String container) {
> + deleteAndVerifyContainerGone(container);
> + }
> +
> + public boolean deleteContainerIfEmpty(final String container) {
> + Boolean returnVal = true;
`boolean`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/220/files#r13571731
Re: [jclouds] Remove LocalAsyncBlobStore. (#220)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #891](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/891/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/220#issuecomment-29670602
Re: [jclouds] Remove LocalAsyncBlobStore. (#220)
Posted by Andrew Phillips <no...@github.com>.
> @@ -72,7 +72,9 @@
> private final Closer closer;
>
> @Inject
> - private StubAtmosAsyncClient(LocalAsyncBlobStore blobStore, AtmosObject.Factory objectProvider,
> + private StubAtmosAsyncClient(
> + LocalBlobStore blobStore,
> + AtmosObject.Factory objectProvider,
Avoid partially reformatting code? I.e. keep the declarations on the same line as in the original?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/220/files#r8053714
Re: [jclouds] Remove LocalAsyncBlobStore. (#220)
Posted by Andrew Phillips <no...@github.com>.
> @@ -99,7 +99,9 @@ private StubAtmosAsyncClient(LocalAsyncBlobStore blobStore, AtmosObject.Factory
> container = directoryName;
> path = null;
> }
> - return Futures.transform(blobStore.createContainerInLocation(null, container), new Function<Boolean, URI>() {
> + return Futures.transform(immediateFuture(blobStore
> + .createContainerInLocation(null, container)),
[minor] Move to previous line?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/220/files#r13571600
Re: [jclouds] Remove LocalAsyncBlobStore. (#220)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #637](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/637/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/220#issuecomment-36686148
Re: [jclouds] Remove LocalAsyncBlobStore. (#220)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1106](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1106/) UNSTABLE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/220#issuecomment-36661249
Re: [jclouds] Remove LocalAsyncBlobStore. (#220)
Posted by Andrew Phillips <no...@github.com>.
> }
> if (options.getIfModifiedSince() != null) {
> Date modifiedSince = dateService.rfc822DateParse(options.getIfModifiedSince());
> if (modifiedSince.after(object.getMetadata().getLastModified()))
> - return immediateFailedFuture(LocalAsyncBlobStore.returnResponseException(412));
> + return immediateFailedFuture(LocalBlobStore
> + .returnResponseException(412));
Do not wrap?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/220/files#r8053765
Re: [jclouds] Remove LocalAsyncBlobStore. (#220)
Posted by Andrew Phillips <no...@github.com>.
> jclouds » jclouds #885 UNSTABLE
> jclouds-java-7-pull-requests #1106 UNSTABLE
Both [unrelated](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/org.apache.jclouds$jclouds-compute/885/testReport/junit/org.jclouds.compute.callables/BlockUntilInitScriptStatusIsZeroThenReturnOutputTest/testloopUntilTrueOrThrowCancellationExceptionReturnsWhenPredicateIsTrueSecondTimeWhileNotCancelled/) [test failures](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/org.apache.jclouds$jclouds-compute/1106/testReport/junit/org.jclouds.compute.util/ConcurrentOpenSocketFinderTest/testChecksSocketsConcurrently/)
There's a bunch of new [Checkstyle violations](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1106/org.apache.jclouds$jclouds-blobstore/violations/), though. Could you have a look at these to see if they're related?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/220#issuecomment-36664792
Re: [jclouds] Remove LocalAsyncBlobStore. (#220)
Posted by Shri Javadekar <no...@github.com>.
> @@ -160,22 +162,26 @@ private StubS3AsyncClient(@Named(Constants.PROPERTY_USER_THREADS) ListeningExecu
> Blob object = source.get(sourceObject);
> if (options.getIfMatch() != null) {
> if (!object.getMetadata().getETag().equals(options.getIfMatch()))
> - return immediateFailedFuture(LocalAsyncBlobStore.returnResponseException(412));
> + return immediateFailedFuture(LocalBlobStore
> + .returnResponseException(412));
Done.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/220/files#r8087012
Re: [jclouds] Remove LocalAsyncBlobStore. (#220)
Posted by Andrew Phillips <no...@github.com>.
@andrewgaul: would you have a chance to take a quick look at this, as one of the "BlobStore experts"?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/220#issuecomment-30095087
Re: [jclouds] Remove LocalAsyncBlobStore. (#220)
Posted by Shri Javadekar <no...@github.com>.
> + if (!options.isDetailed()) {
> + for (StorageMetadata md : contents) {
> + md.getUserMetadata().clear();
> + }
> + }
> + }
> +
> + return new PageSetImpl<StorageMetadata>(contents, marker);
> +
> + }
> +
> + private ContainerNotFoundException cnfe(final String name) {
> + return new ContainerNotFoundException(name, String.format(
> + "container %s not in %s", name,
> + storageStrategy.getAllContainerNames()));
> + }
Done.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/220/files#r8087073
Re: [jclouds] Remove LocalAsyncBlobStore. (#220)
Posted by Shri Javadekar <no...@github.com>.
Is there anything else that needs to be done here?
There is one feature that I am developing in jclouds that is made easier because of this change. Let me know if there's anything I can do to help make progress on this.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/220#issuecomment-45361526
Re: [jclouds] Remove LocalAsyncBlobStore. (#220)
Posted by Andrew Phillips <no...@github.com>.
> @@ -148,7 +153,8 @@ public Void apply(Boolean from) {
> } else {
> String container = path.substring(0, path.indexOf('/'));
> path = path.substring(path.indexOf('/') + 1);
> - return blobStore.removeBlob(container, path);
> + blobStore.removeBlob(container, path);
> + return immediateFuture(null);
`immediateFuture` null?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/220/files#r13571625
Re: [jclouds] Remove LocalAsyncBlobStore. (#220)
Posted by Shri Javadekar <no...@github.com>.
> @@ -197,7 +203,8 @@ private StubS3AsyncClient(@Named(Constants.PROPERTY_USER_THREADS) ListeningExecu
> final PutObjectOptions options = (nullableOptions.length == 0) ? new PutObjectOptions() : nullableOptions[0];
> if (options.getAcl() != null)
> keyToAcl.put(bucketName + "/" + object.getMetadata().getKey(), options.getAcl());
> - return blobStore.putBlob(bucketName, object2Blob.apply(object));
> + return immediateFuture(blobStore.putBlob(bucketName,
> + object2Blob.apply(object)));
Done.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/220/files#r8087024
Re: [jclouds] Remove LocalAsyncBlobStore. (#220)
Posted by Andrew Phillips <no...@github.com>.
> jclouds-java-7-pull-requests #1339 SUCCESS
Some [Checkstyle violations](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1339/violations/) - not sure which of these are related to this change, though. Could you have a quick look, @shrinandj? Thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/220#issuecomment-45559803
Re: [jclouds] Remove LocalAsyncBlobStore. (#220)
Posted by Shri Javadekar <no...@github.com>.
>
> }
> if (options.getIfUnmodifiedSince() != null) {
> Date unmodifiedSince = dateService.rfc822DateParse(options.getIfUnmodifiedSince());
> if (unmodifiedSince.before(object.getMetadata().getLastModified()))
> - return immediateFailedFuture(LocalAsyncBlobStore.returnResponseException(412));
> + return immediateFailedFuture(LocalBlobStore
> + .returnResponseException(412));
Done.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/220/files#r8087020
Re: [jclouds] Remove LocalAsyncBlobStore. (#220)
Posted by Andrew Phillips <no...@github.com>.
> @@ -197,7 +203,8 @@ private StubS3AsyncClient(@Named(Constants.PROPERTY_USER_THREADS) ListeningExecu
> final PutObjectOptions options = (nullableOptions.length == 0) ? new PutObjectOptions() : nullableOptions[0];
> if (options.getAcl() != null)
> keyToAcl.put(bucketName + "/" + object.getMetadata().getKey(), options.getAcl());
> - return blobStore.putBlob(bucketName, object2Blob.apply(object));
> + return immediateFuture(blobStore.putBlob(bucketName,
> + object2Blob.apply(object)));
Do not wrap?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/220/files#r8053773
Re: [jclouds] Remove LocalAsyncBlobStore. (#220)
Posted by Shri Javadekar <no...@github.com>.
> @@ -104,7 +103,8 @@
>
> @Inject
> private StubS3AsyncClient(@Named(Constants.PROPERTY_USER_THREADS) ListeningExecutorService userExecutor,
> - LocalAsyncBlobStore blobStore, ConcurrentMap<String, ConcurrentMap<String, Blob>> containerToBlobs,
> + LocalBlobStore blobStore,
> + ConcurrentMap<String, ConcurrentMap<String, Blob>> containerToBlobs,
Done
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/220/files#r8087007
Re: [jclouds] Remove LocalAsyncBlobStore. (#220)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #892](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/892/) UNSTABLE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/220#issuecomment-29784470
Re: [jclouds] Remove LocalAsyncBlobStore. (#220)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #429](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/429/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/220#issuecomment-29784444
Re: [jclouds] Remove LocalAsyncBlobStore. (#220)
Posted by Andrew Gaul <no...@github.com>.
I have not looked at this pull request yet, but let's address any blobstore de-async in 1.8.0.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/220#issuecomment-30174154
Re: [jclouds] Remove LocalAsyncBlobStore. (#220)
Posted by Shri Javadekar <no...@github.com>.
> +import com.google.common.collect.ImmutableMap;
> +import com.google.common.collect.Iterables;
> +import com.google.common.util.concurrent.ListeningExecutorService;
> +
> +public class LocalBlobStore extends BaseBlobStore {
> +
> + @Resource
> + protected Logger logger = Logger.NULL;
> +
> + protected final ContentMetadataCodec contentMetadataCodec;
> + protected final IfDirectoryReturnNameStrategy ifDirectoryReturnName;
> + protected final Factory blobFactory;
> + protected final LocalStorageStrategy storageStrategy;
> +
> + @Inject
> + protected LocalBlobStore(
>From LocalAsyncBlobStore.java.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/220/files#r8087077
Re: [jclouds] Remove LocalAsyncBlobStore. (#220)
Posted by Shri Javadekar <no...@github.com>.
> @@ -72,7 +69,7 @@ public BlobStore getBlobStore() {
>
> @Override
> public AsyncBlobStore getAsyncBlobStore() {
> - return ablobStore;
> + return null;
> }
That's right. The other pull request completely gets rid of the getAsyncBlobStore() method so that an async blobstore object cannot be used.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/220/files#r8087090
Re: [jclouds] Remove LocalAsyncBlobStore. (#220)
Posted by Andrew Phillips <no...@github.com>.
> @@ -104,7 +103,8 @@
>
> @Inject
> private StubS3AsyncClient(@Named(Constants.PROPERTY_USER_THREADS) ListeningExecutorService userExecutor,
> - LocalAsyncBlobStore blobStore, ConcurrentMap<String, ConcurrentMap<String, Blob>> containerToBlobs,
> + LocalBlobStore blobStore,
> + ConcurrentMap<String, ConcurrentMap<String, Blob>> containerToBlobs,
Avoid partial reformatting?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/220/files#r8053759
Re: [jclouds] Remove LocalAsyncBlobStore. (#220)
Posted by Andrew Phillips <no...@github.com>.
> @@ -72,7 +69,7 @@ public BlobStore getBlobStore() {
>
> @Override
> public AsyncBlobStore getAsyncBlobStore() {
> - return ablobStore;
> + return null;
> }
I'm guessing this should disappear? But that's in the other pull request, from what I recall...
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/220/files#r8053904
Re: [jclouds] Remove LocalAsyncBlobStore. (#220)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #868](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/868/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/220#issuecomment-45555320
Re: [jclouds] Remove LocalAsyncBlobStore. (#220)
Posted by Shri Javadekar <no...@github.com>.
> * @author Alfredo "Rainbowbreeze" Morresi
> */
> public class FilesystemBlobStoreContextModule extends AbstractModule {
>
> @Override
> protected void configure() {
> - bind(AsyncBlobStore.class).to(LocalAsyncBlobStore.class).asEagerSingleton();
> - // forward all requests from TransientBlobStore to TransientAsyncBlobStore. needs above binding as cannot proxy a class
> - bindSyncToAsyncApi(binder(), LocalBlobStore.class, AsyncBlobStore.class);
> - bind(BlobStore.class).to(LocalBlobStore.class);
> + bind(BlobStore.class).to(LocalBlobStore.class).in(Scopes.SINGLETON);
I think it is required to keep this Singleton. Some unit tests fail otherwise. If it is not singleton the injected blobstore objects turn out to be different from those used in the BeforeSuite/BeforeTest.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/220/files#r8087003
Re: [jclouds] Remove LocalAsyncBlobStore. (#220)
Posted by Andrew Phillips <no...@github.com>.
> @@ -41,7 +41,7 @@
> import org.jclouds.atmos.domain.UserMetadata;
> import org.jclouds.atmos.options.ListOptions;
> import org.jclouds.atmos.options.PutOptions;
> -import org.jclouds.blobstore.LocalAsyncBlobStore;
> +import org.jclouds.blobstore.config.LocalBlobStore;
Any reason why `LocalBlobStore` is in the `config` package, by the way?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/220/files#r8053684
Re: [jclouds] Remove LocalAsyncBlobStore. (#220)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #887](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/887/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/220#issuecomment-36684201
Re: [jclouds] Remove LocalAsyncBlobStore. (#220)
Posted by Andrew Gaul <no...@github.com>.
> Is there anything else that needs to be done here?
>
> There is one feature that I am developing in jclouds that is made easier because of this change. Let me know if there's anything I can do to help make progress on this.
I do not think `LocalAsyncBlobStore` is the appropriate place to start de-asyncification of blobstore providers. These fake providers should mimic the real providers as they exist today, and not as they might exist in the future. We should be trying to make the fake providers *more* similar to real providers, e.g., #382.
Perhaps you can start a thread on jclouds-dev discussing if and how to make progress on de-asyncification of blobstore? While the compute committers have made good progress on this task, it is not clear that this is the best fit for blobstore or that we have sufficient volunteers to complete this. Starting with the s3 or openstack-swift providers would give us much more experience on how this will work with a purely synchronous interface. I have not investigated this at all and have concerns about how operations such as `clearContainer`, cancellation, or long-lived operations like Glacier will work.
Do you have a specific task that this pull request blocks? Perhaps we can address that in some other way?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/220#issuecomment-45560184
Re: [jclouds] Remove LocalAsyncBlobStore. (#220)
Posted by Andrew Phillips <no...@github.com>.
>
> }
> if (options.getIfUnmodifiedSince() != null) {
> Date unmodifiedSince = dateService.rfc822DateParse(options.getIfUnmodifiedSince());
> if (unmodifiedSince.before(object.getMetadata().getLastModified()))
> - return immediateFailedFuture(LocalAsyncBlobStore.returnResponseException(412));
> + return immediateFailedFuture(LocalBlobStore
> + .returnResponseException(412));
Do not wrap?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/220/files#r8053767
Re: [jclouds] Remove LocalAsyncBlobStore. (#220)
Posted by Andrew Phillips <no...@github.com>.
> Is there anything else that needs to be done here?
Looks like it needs at least a rebase - can't be merged as-is right now?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/220#issuecomment-45497254
Re: [jclouds] Remove LocalAsyncBlobStore. (#220)
Posted by Everett Toews <no...@github.com>.
During release week we like to do a little house cleaning in the jclouds world. That means sweeping out the pull request queue.
This PR is over 6 months old. Please update us on its status here. If we don't hear anything, we will take that as lazy consensus that the PR is no longer relevant and it will be closed on Friday, Aug 8.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/220#issuecomment-50782267
Re: [jclouds] Remove LocalAsyncBlobStore. (#220)
Posted by Andrew Phillips <no...@github.com>.
> jclouds-java-7-pull-requests #892 UNSTABLE
Unrelated [test failure](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/org.apache.jclouds$jclouds-compute/892/testReport/junit/org.jclouds.compute.callables/BlockUntilInitScriptStatusIsZeroThenReturnOutputTest/testloopUntilTrueOrThrowCancellationExceptionReturnsWhenPredicateIsTrueSecondTimeWhileNotCancelled/)
@shrinandj: Thanks for the changes and explanations. +1 - looks good to me. Would be nice to follow this up with a `org.jclouds.blobstore.config.LocalBlobStore` -> `org.jclouds.blobstore.LocalBlobStore` change, but agree that this would better be a separate PR.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/220#issuecomment-29785492
Re: [jclouds] Remove LocalAsyncBlobStore. (#220)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #428](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/428/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/220#issuecomment-29670566
Re: [jclouds] Remove LocalAsyncBlobStore. (#220)
Posted by Andrew Phillips <no...@github.com>.
> @@ -213,17 +226,12 @@ public AtmosObject newObject() {
> public ListenableFuture<Boolean> pathExists(final String path) {
> if (path.indexOf('/') == path.length() - 1) {
> // chop off the trailing slash
> - return blobStore.containerExists(path.substring(0, path.length() - 1));
> + return immediateFuture(blobStore.containerExists(path.substring(0,
[minor] Wrap at `path.substring`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/220/files#r13571637
Re: [jclouds] Remove LocalAsyncBlobStore. (#220)
Posted by Shri Javadekar <no...@github.com>.
I agree that we should make the filesystem and transient blobstores similar to the real blobstores. However, I'm not sure if keeping the LocalAsyncBlobStore around helps that. Has it been useful for test and dev of the long running operations that you mention? Is it useful today for these operations, now that the decision to get rid of the AsyncBlobStore is made?
I wanted to introduce a new method on the BlobStore object called removeBlobs(Collection<String> blobNames). This would internally identify whether the api/provider is a capable of doing bulk delete and make the calls accordingly.
Without this change, I need to add the removeBlobs method to the AsyncBlobStore and whoever depends on it. This is unnecessary code which is going to get deleted any way. I'm planning to send out an email on the jclouds-dev list with some more details and request suggestions before I actually send out the pull request. But this LocalAsyncBlobstore was getting in my way.
This is what prompted me to make this change in the first place.
Thoughts?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/220#issuecomment-45668395
Re: [jclouds] Remove LocalAsyncBlobStore. (#220)
Posted by Andrew Phillips <no...@github.com>.
> +import com.google.common.collect.ImmutableMap;
> +import com.google.common.collect.Iterables;
> +import com.google.common.util.concurrent.ListeningExecutorService;
> +
> +public class LocalBlobStore extends BaseBlobStore {
> +
> + @Resource
> + protected Logger logger = Logger.NULL;
> +
> + protected final ContentMetadataCodec contentMetadataCodec;
> + protected final IfDirectoryReturnNameStrategy ifDirectoryReturnName;
> + protected final Factory blobFactory;
> + protected final LocalStorageStrategy storageStrategy;
> +
> + @Inject
> + protected LocalBlobStore(
Just to ensure I understand: most of this is copied from the previous AsyncLocalBlobStore?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/220/files#r8053872
Re: [jclouds] Remove LocalAsyncBlobStore. (#220)
Posted by Andrew Phillips <no...@github.com>.
> @@ -160,22 +162,26 @@ private StubS3AsyncClient(@Named(Constants.PROPERTY_USER_THREADS) ListeningExecu
> Blob object = source.get(sourceObject);
> if (options.getIfMatch() != null) {
> if (!object.getMetadata().getETag().equals(options.getIfMatch()))
> - return immediateFailedFuture(LocalAsyncBlobStore.returnResponseException(412));
> + return immediateFailedFuture(LocalBlobStore
> + .returnResponseException(412));
Do not wrap?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/220/files#r8053763
Re: [jclouds] Remove LocalAsyncBlobStore. (#220)
Posted by Andrew Phillips <no...@github.com>.
> @@ -80,9 +79,8 @@
> @Inject
> MarkersDeleteDirectoryStrategy(
> @Named(Constants.PROPERTY_USER_THREADS) ListeningExecutorService userExecutor,
> - AsyncBlobStore ablobstore, BlobStore blobstore) {
> + BlobStore blobstore) {
Formatting? Alignment is not consistent with previous line now.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/220/files#r8053915
Re: [jclouds] Remove LocalAsyncBlobStore. (#220)
Posted by Andrew Phillips <no...@github.com>.
In general, looks good. Only a couple of minor questions, and comments about formatting. Glad to see so much code hopefully go away soon!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/220#issuecomment-29693590
Re: [jclouds] Remove LocalAsyncBlobStore. (#220)
Posted by Andrew Phillips <no...@github.com>.
> * @author Adrian Cole
> */
> // NOTE:without testName, this will not call @Before* and fail w/NPE during surefire
> @Test(groups = "unit", testName = "TransientBlobRequestSignerTest")
> -public class TransientBlobRequestSignerTest extends BaseAsyncClientTest<LocalAsyncBlobStore> {
> +public class TransientBlobRequestSignerTest extends
> + BaseAsyncClientTest<LocalBlobStore> {
Don't wrap? And why an **Async**ClientTest?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/220/files#r8053932
Re: [jclouds] Remove LocalAsyncBlobStore. (#220)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1339](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1339/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/220#issuecomment-45555476
Re: [jclouds] Remove LocalAsyncBlobStore. (#220)
Posted by Andrew Phillips <no...@github.com>.
> jclouds-java-7-pull-requests #1107 UNSTABLE
Unrelated [test failure](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/org.apache.jclouds$jclouds-compute/1107/testReport/org.jclouds.compute.util/ConcurrentOpenSocketFinderTest/testChecksSocketsConcurrently/). +1 from me. @andrewgaul: Good to go for you..?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/220#issuecomment-36733401
Re: [jclouds] Remove LocalAsyncBlobStore. (#220)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #636](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/636/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/220#issuecomment-36660397
Re: [jclouds] Remove LocalAsyncBlobStore. (#220)
Posted by Shri Javadekar <no...@github.com>.
Fixed the checkStyle violations.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/220#issuecomment-36678985
Re: [jclouds] Remove LocalAsyncBlobStore. (#220)
Posted by Shri Javadekar <no...@github.com>.
Looks like, the next release will be 1.8.0. I'll rebase this patch on current TOT.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/220#issuecomment-36652435