You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Klaus <no...@github.com> on 2014/06/25 10:02:31 UTC
[jclouds] Update AzureBlobStore.java (#422)
JCLOUDS-391:
set user-defined metadata for Azure with multipart blobs
You can merge this Pull Request by running:
git pull https://github.com/klausm/jclouds master
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds/pull/422
-- Commit Summary --
* Update AzureBlobStore.java
-- File Changes --
M providers/azureblob/src/main/java/org/jclouds/azureblob/blobstore/AzureBlobStore.java (9)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/422.patch
https://github.com/jclouds/jclouds/pull/422.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/422
Re: [jclouds] Update AzureBlobStore.java (#422)
Posted by Adrian Cole <no...@github.com>.
> }
> return putBlob(container, blob);
> - }
> + }
supernit
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/422/files#r14244756
Re: [jclouds] Update AzureBlobStore.java (#422)
Posted by Andrew Gaul <no...@github.com>.
> @@ -208,10 +208,15 @@ public String putBlob(String container, Blob blob) {
> @Override
> public String putBlob(String container, Blob blob, PutOptions options) {
> if (options.isMultipart()) {
> - return multipartUploadStrategy.get().execute(container, blob);
> + String result = multipartUploadStrategy.get().execute(container, blob);
> + if (result != null) {
This change causes `testMultipartChunkedFileStream` and `testMultipartChunkedFileStreamPowerOfTwoSize` to fail since the separate call to `setBlobMetadata` changes the ETag (Azure uses a timestamp instead of content hash).
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/422/files#r22136571
Re: [jclouds] Update AzureBlobStore.java (#422)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1405](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1405/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/422#issuecomment-47076113
Re: [jclouds] Update AzureBlobStore.java (#422)
Posted by Andrew Gaul <no...@github.com>.
Closed #422.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/422#event-210476074
Re: [jclouds] Update AzureBlobStore.java (#422)
Posted by Andrew Phillips <no...@github.com>.
> @@ -208,10 +208,15 @@ public String putBlob(String container, Blob blob) {
> @Override
> public String putBlob(String container, Blob blob, PutOptions options) {
> if (options.isMultipart()) {
> - return multipartUploadStrategy.get().execute(container, blob);
> + String result = multipartUploadStrategy.get().execute(container, blob);
> + if (result != null) {
Looking at [this comment](https://issues.apache.org/jira/browse/JCLOUDS-391?focusedCommentId=14032527&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14032527), this looks like "the patch way" rather than the "proper" solution. That's fine, but should we add a comment to indicate that the "more proper" way to solve this would be to do X..?
/cc @adriancole
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/422/files#r15662610
Re: [jclouds] Update AzureBlobStore.java (#422)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #934](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/934/) 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/422#issuecomment-47075724
Re: [jclouds] Update AzureBlobStore.java (#422)
Posted by Adrian Cole <no...@github.com>.
this can be solved with a mock test. I will see about helping with this, else ping me in a week.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/422#issuecomment-57894631
Re: [jclouds] Update AzureBlobStore.java (#422)
Posted by Klaus <no...@github.com>.
sorry for my delay,
@andrewgaul where I have to add the live test for user defined meta data with multipart blobs?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/422#issuecomment-50758319
Re: [jclouds] Update AzureBlobStore.java (#422)
Posted by Adrian Cole <no...@github.com>.
LGTM
aside, I know that there's no baseline to make a mock test for this, but if you'd like to help, you could make a copy of this and edit it so that it reaches the codepath you've added https://github.com/jclouds/jclouds/blob/master/apis/s3/src/test/java/org/jclouds/s3/S3ClientMockTest.java
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/422#issuecomment-47236892
Re: [jclouds] Update AzureBlobStore.java (#422)
Posted by Klaus <no...@github.com>.
set blob metadata on the way out
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/422#issuecomment-47071870
Re: [jclouds] Update AzureBlobStore.java (#422)
Posted by Andrew Gaul <no...@github.com>.
@klausm this looks good although can you also add a live test for this?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/422#issuecomment-47271765
Re: [jclouds] Update AzureBlobStore.java (#422)
Posted by Andrew Phillips <no...@github.com>.
> where I have to add the live test for user defined meta data with multipart blobs?
I'm not the expert here, but [AzureBlobIntegrationLiveTest](https://github.com/jclouds/jclouds/blob/master/providers/azureblob/src/test/java/org/jclouds/azureblob/blobstore/integration/AzureBlobIntegrationLiveTest.java) looks like a suitable place. Perhaps in `testMultipartChunkedFileStream`, or as a separate test.
Some possible checks are in [BaseBlobIntegrationTest.testMetadata](https://github.com/jclouds/jclouds/blob/master/blobstore/src/test/java/org/jclouds/blobstore/integration/internal/BaseBlobIntegrationTest.java#L585)
/cc @andrewgaul @shrinandj You two are certainly better placed to comment here ;-)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/422#issuecomment-50804969
Re: [jclouds] Update AzureBlobStore.java (#422)
Posted by Andrew Gaul <no...@github.com>.
> @@ -208,10 +208,15 @@ public String putBlob(String container, Blob blob) {
> @Override
> public String putBlob(String container, Blob blob, PutOptions options) {
> if (options.isMultipart()) {
> - return multipartUploadStrategy.get().execute(container, blob);
> + String result = multipartUploadStrategy.get().execute(container, blob);
> + if (result != null) {
I reimplemented this the proper way in #628, which sets metadata with the existing `putBlobList` instead of an additional call to `setBlobMetadata`. I also added a live test.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/422/files#r22138164