You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Timur Alperovich <no...@github.com> on 2018/10/25 01:36:54 UTC
[jclouds/jclouds] Filesystem: Fix the MPU ETags to match S3. (#1252)
Prior commit introduced a bug in the computation of the MPU ETag value,
where it was concatenating strings, rather than operating on the bytes
of the integer value.
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds/pull/1252
-- Commit Summary --
* Filesystem: Fix the MPU ETags to match S3.
-- File Changes --
M apis/filesystem/src/test/java/org/jclouds/filesystem/integration/FilesystemBlobIntegrationTest.java (47)
M blobstore/src/main/java/org/jclouds/blobstore/config/LocalBlobStore.java (9)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/1252.patch
https://github.com/jclouds/jclouds/pull/1252.diff
--
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/1252
Re: [jclouds/jclouds] Filesystem: Fix the MPU ETags to match S3.
(#1252)
Posted by Andrew Gaul <no...@github.com>.
Closed #1252.
--
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/1252#event-1944542925
Re: [jclouds/jclouds] Filesystem: Fix the MPU ETags to match S3.
(#1252)
Posted by Timur Alperovich <no...@github.com>.
timuralp commented on this pull request.
> @@ -838,10 +839,10 @@ public String completeMultipartUpload(MultipartUpload mpu, List<MultipartPart> p
throw propagate(ioe);
}
streams.add(is);
- partHashes.append(blobPart.getMetadata().getETag());
+ md5Hasher.putBytes(new BigInteger(blobPart.getMetadata().getETag(), 16).toByteArray());
Done.
--
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/1252#discussion_r230231187
Re: [jclouds/jclouds] Filesystem: Fix the MPU ETags to match S3.
(#1252)
Posted by Andrew Phillips <no...@github.com>.
demobox commented on this pull request.
Just a couple of minor questions; fine to proceed if someone with more blobstore knowledge is good with this.
Thanks for submitting, @timuralp!
--
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/1252#pullrequestreview-168619381
Re: [jclouds/jclouds] Filesystem: Fix the MPU ETags to match S3.
(#1252)
Posted by Andrew Phillips <no...@github.com>.
demobox commented on this pull request.
> .append("-")
.append(partsList.size())
.append("\"")
.toString();
assertThat(blob.getMetadata().getETag()).isEqualTo(expectedETag);
}
+ @Test(groups = { "integration", "live" })
+ public void testMultipartUploadMultiplePartsKnownETag() throws Exception {
+ BlobStore blobStore = view.getBlobStore();
+ String container = getContainerName();
+ // Pre-computed ETag returned by AWS S3 for the MPU consisting of two 5MB parts filled with 'b'
+ String expectedETag = "\"84462a16f6a60478d50148808aa609c1-2\"";
> The initial proposal I had was to use AWS S3-style ETags because it allows for an easy way to emulate S3 using jcloud
Clear, thanks!
--
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/1252#discussion_r228363705
Re: [jclouds/jclouds] Filesystem: Fix the MPU ETags to match S3.
(#1252)
Posted by Andrew Phillips <no...@github.com>.
demobox commented on this pull request.
> .append("-")
.append(partsList.size())
.append("\"")
.toString();
assertThat(blob.getMetadata().getETag()).isEqualTo(expectedETag);
}
+ @Test(groups = { "integration", "live" })
+ public void testMultipartUploadMultiplePartsKnownETag() throws Exception {
+ BlobStore blobStore = view.getBlobStore();
+ String container = getContainerName();
+ // Pre-computed ETag returned by AWS S3 for the MPU consisting of two 5MB parts filled with 'b'
+ String expectedETag = "\"84462a16f6a60478d50148808aa609c1-2\"";
Ah, just seeing #1251. A follow-up question to that (also /cc @gaul): is S3-compatibility considered a filesystem provider goal? How (if at all) do other blobstore providers treat this case?
--
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/1252#discussion_r228362387
Re: [jclouds/jclouds] Filesystem: Fix the MPU ETags to match S3.
(#1252)
Posted by Andrew Gaul <no...@github.com>.
gaul requested changes on this pull request.
> .append("-")
.append(partsList.size())
.append("\"")
.toString();
assertThat(blob.getMetadata().getETag()).isEqualTo(expectedETag);
}
+ @Test(groups = { "integration", "live" })
+ public void testMultipartUploadMultiplePartsKnownETag() throws Exception {
+ BlobStore blobStore = view.getBlobStore();
+ String container = getContainerName();
+ // Pre-computed ETag returned by AWS S3 for the MPU consisting of two 5MB parts filled with 'b'
+ String expectedETag = "\"84462a16f6a60478d50148808aa609c1-2\"";
S3 has emerged as the de facto standard for object storage. I think the average user would prefer the local blobstore acts more similarly to S3, especially since one of the main consumers of this is S3Proxy. :-)
> @@ -838,10 +839,10 @@ public String completeMultipartUpload(MultipartUpload mpu, List<MultipartPart> p
throw propagate(ioe);
}
streams.add(is);
- partHashes.append(blobPart.getMetadata().getETag());
+ md5Hasher.putBytes(new BigInteger(blobPart.getMetadata().getETag(), 16).toByteArray());
This is clever but would `BaseEncoding.base16.decode` address this more naturally?
> + @Test(groups = { "integration", "live" })
+ public void testMultipartUploadMultiplePartsKnownETag() throws Exception {
+ BlobStore blobStore = view.getBlobStore();
+ String container = getContainerName();
+ // Pre-computed ETag returned by AWS S3 for the MPU consisting of two 5MB parts filled with 'b'
+ String expectedETag = "\"84462a16f6a60478d50148808aa609c1-2\"";
+ int partSize = 5 * (int) Math.pow(2, 20);
+ try {
+ String name = "blob-name";
+ BlobBuilder blobBuilder = blobStore.blobBuilder(name);
+ Blob blob = blobBuilder.build();
+ MultipartUpload mpu = blobStore.initiateMultipartUpload(container, blob.getMetadata(), new PutOptions());
+
+ byte[] content = new byte[partSize];
+ Arrays.fill(content, (byte) 'b');
+ Payload payload = Payloads.newByteArrayPayload(content);
Would `TestUtils.randomdByteSource(seed)` work as well?
> .append("-")
.append(partsList.size())
.append("\"")
.toString();
assertThat(blob.getMetadata().getETag()).isEqualTo(expectedETag);
}
+ @Test(groups = { "integration", "live" })
+ public void testMultipartUploadMultiplePartsKnownETag() throws Exception {
+ BlobStore blobStore = view.getBlobStore();
+ String container = getContainerName();
+ // Pre-computed ETag returned by AWS S3 for the MPU consisting of two 5MB parts filled with 'b'
+ String expectedETag = "\"84462a16f6a60478d50148808aa609c1-2\"";
+ int partSize = 5 * (int) Math.pow(2, 20);
Easier to read `5 * 1024 * 1024`.
--
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/1252#pullrequestreview-169960399
Re: [jclouds/jclouds] Filesystem: Fix the MPU ETags to match S3.
(#1252)
Posted by Andrew Phillips <no...@github.com>.
demobox commented on this pull request.
> + MultipartPart part1 = blobStore.uploadMultipartPart(mpu, 1, payload);
+ MultipartPart part2 = blobStore.uploadMultipartPart(mpu, 2, payload);
+
+ List<MultipartPart> parts = blobStore.listMultipartUpload(mpu);
+ assertThat(parts).hasSize(2);
+ assertThat(parts.get(0).partNumber()).isEqualTo(part1.partNumber());
+ assertThat(parts.get(0).partSize()).isEqualTo(part1.partSize());
+ assertThat(parts.get(0).partETag()).isEqualTo(part1.partETag());
+ assertThat(parts.get(1).partNumber()).isEqualTo(part2.partNumber());
+ assertThat(parts.get(1).partSize()).isEqualTo(part2.partSize());
+ assertThat(parts.get(1).partETag()).isEqualTo(part2.partETag());
+
+ blobStore.completeMultipartUpload(mpu, ImmutableList.of(part1, part2));
+
+ BlobMetadata newBlobMetadata = blobStore.blobMetadata(container, name);
+ assertThat(newBlobMetadata.getETag()).isEqualTo(expectedETag);
Sorry, I wasn't clear: I think it's fine to have *fewer* validations, so it's clear to the reader immediately what the "main" point of this test is. E.g. skip the assertions on number and size of parts, if that's not what we care about here?
--
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/1252#discussion_r228362685
Re: [jclouds/jclouds] Filesystem: Fix the MPU ETags to match S3.
(#1252)
Posted by Timur Alperovich <no...@github.com>.
timuralp commented on this pull request.
> .append("-")
.append(partsList.size())
.append("\"")
.toString();
assertThat(blob.getMetadata().getETag()).isEqualTo(expectedETag);
}
+ @Test(groups = { "integration", "live" })
+ public void testMultipartUploadMultiplePartsKnownETag() throws Exception {
+ BlobStore blobStore = view.getBlobStore();
+ String container = getContainerName();
+ // Pre-computed ETag returned by AWS S3 for the MPU consisting of two 5MB parts filled with 'b'
+ String expectedETag = "\"84462a16f6a60478d50148808aa609c1-2\"";
Good question. Some providers return a similar value, like Swift for static large objects. Others have a different ETag, like Azure, which uses an opaque identifier. The initial proposal I had was to use AWS S3-style ETags because it allows for an easy way to emulate S3 using jclouds (like with the S3Proxy project). That's the motivation behind this PR and the prior one related to it: https://github.com/jclouds/jclouds/pull/1251.
--
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/1252#discussion_r228363312
Re: [jclouds/jclouds] Filesystem: Fix the MPU ETags to match S3.
(#1252)
Posted by Andrew Phillips <no...@github.com>.
demobox commented on this pull request.
> + MultipartPart part1 = blobStore.uploadMultipartPart(mpu, 1, payload);
+ MultipartPart part2 = blobStore.uploadMultipartPart(mpu, 2, payload);
+
+ List<MultipartPart> parts = blobStore.listMultipartUpload(mpu);
+ assertThat(parts).hasSize(2);
+ assertThat(parts.get(0).partNumber()).isEqualTo(part1.partNumber());
+ assertThat(parts.get(0).partSize()).isEqualTo(part1.partSize());
+ assertThat(parts.get(0).partETag()).isEqualTo(part1.partETag());
+ assertThat(parts.get(1).partNumber()).isEqualTo(part2.partNumber());
+ assertThat(parts.get(1).partSize()).isEqualTo(part2.partSize());
+ assertThat(parts.get(1).partETag()).isEqualTo(part2.partETag());
+
+ blobStore.completeMultipartUpload(mpu, ImmutableList.of(part1, part2));
+
+ BlobMetadata newBlobMetadata = blobStore.blobMetadata(container, name);
+ assertThat(newBlobMetadata.getETag()).isEqualTo(expectedETag);
[minor] Is there anything other than this final assertion (regarding the expected ETag) that we're trying to verify in this test?
--
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/1252#pullrequestreview-168619146
Re: [jclouds/jclouds] Filesystem: Fix the MPU ETags to match S3.
(#1252)
Posted by Timur Alperovich <no...@github.com>.
timuralp commented on this pull request.
> .append("-")
.append(partsList.size())
.append("\"")
.toString();
assertThat(blob.getMetadata().getETag()).isEqualTo(expectedETag);
}
+ @Test(groups = { "integration", "live" })
+ public void testMultipartUploadMultiplePartsKnownETag() throws Exception {
+ BlobStore blobStore = view.getBlobStore();
+ String container = getContainerName();
+ // Pre-computed ETag returned by AWS S3 for the MPU consisting of two 5MB parts filled with 'b'
+ String expectedETag = "\"84462a16f6a60478d50148808aa609c1-2\"";
+ int partSize = 5 * (int) Math.pow(2, 20);
Done.
--
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/1252#discussion_r230231474
Re: [jclouds/jclouds] Filesystem: Fix the MPU ETags to match S3.
(#1252)
Posted by Timur Alperovich <no...@github.com>.
timuralp commented on this pull request.
> + @Test(groups = { "integration", "live" })
+ public void testMultipartUploadMultiplePartsKnownETag() throws Exception {
+ BlobStore blobStore = view.getBlobStore();
+ String container = getContainerName();
+ // Pre-computed ETag returned by AWS S3 for the MPU consisting of two 5MB parts filled with 'b'
+ String expectedETag = "\"84462a16f6a60478d50148808aa609c1-2\"";
+ int partSize = 5 * (int) Math.pow(2, 20);
+ try {
+ String name = "blob-name";
+ BlobBuilder blobBuilder = blobStore.blobBuilder(name);
+ Blob blob = blobBuilder.build();
+ MultipartUpload mpu = blobStore.initiateMultipartUpload(container, blob.getMetadata(), new PutOptions());
+
+ byte[] content = new byte[partSize];
+ Arrays.fill(content, (byte) 'b');
+ Payload payload = Payloads.newByteArrayPayload(content);
Most likely -- the challenging part is that I'd need to pre-compute the ETag for that output. I picked the easy content of "b"s so I can easily test what AWS S3 returns and used it as a test value (otherwise, like the original patch pointed out, we have a problem of the test itself being buggy in the same way).
--
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/1252#discussion_r230231453
Re: [jclouds/jclouds] Filesystem: Fix the MPU ETags to match S3.
(#1252)
Posted by Timur Alperovich <no...@github.com>.
rebuild please
--
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/1252#issuecomment-433188506
Re: [jclouds/jclouds] Filesystem: Fix the MPU ETags to match S3.
(#1252)
Posted by Timur Alperovich <no...@github.com>.
timuralp commented on this pull request.
> + MultipartPart part1 = blobStore.uploadMultipartPart(mpu, 1, payload);
+ MultipartPart part2 = blobStore.uploadMultipartPart(mpu, 2, payload);
+
+ List<MultipartPart> parts = blobStore.listMultipartUpload(mpu);
+ assertThat(parts).hasSize(2);
+ assertThat(parts.get(0).partNumber()).isEqualTo(part1.partNumber());
+ assertThat(parts.get(0).partSize()).isEqualTo(part1.partSize());
+ assertThat(parts.get(0).partETag()).isEqualTo(part1.partETag());
+ assertThat(parts.get(1).partNumber()).isEqualTo(part2.partNumber());
+ assertThat(parts.get(1).partSize()).isEqualTo(part2.partSize());
+ assertThat(parts.get(1).partETag()).isEqualTo(part2.partETag());
+
+ blobStore.completeMultipartUpload(mpu, ImmutableList.of(part1, part2));
+
+ BlobMetadata newBlobMetadata = blobStore.blobMetadata(container, name);
+ assertThat(newBlobMetadata.getETag()).isEqualTo(expectedETag);
Gotcha! Will remove the extraneous checks.
--
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/1252#discussion_r228365019
Re: [jclouds/jclouds] Filesystem: Fix the MPU ETags to match S3.
(#1252)
Posted by Timur Alperovich <no...@github.com>.
timuralp commented on this pull request.
> + MultipartPart part1 = blobStore.uploadMultipartPart(mpu, 1, payload);
+ MultipartPart part2 = blobStore.uploadMultipartPart(mpu, 2, payload);
+
+ List<MultipartPart> parts = blobStore.listMultipartUpload(mpu);
+ assertThat(parts).hasSize(2);
+ assertThat(parts.get(0).partNumber()).isEqualTo(part1.partNumber());
+ assertThat(parts.get(0).partSize()).isEqualTo(part1.partSize());
+ assertThat(parts.get(0).partETag()).isEqualTo(part1.partETag());
+ assertThat(parts.get(1).partNumber()).isEqualTo(part2.partNumber());
+ assertThat(parts.get(1).partSize()).isEqualTo(part2.partSize());
+ assertThat(parts.get(1).partETag()).isEqualTo(part2.partETag());
+
+ blobStore.completeMultipartUpload(mpu, ImmutableList.of(part1, part2));
+
+ BlobMetadata newBlobMetadata = blobStore.blobMetadata(container, name);
+ assertThat(newBlobMetadata.getETag()).isEqualTo(expectedETag);
No, there isn't. I considered validating the content itself, but there are existing tests that do that. I can add the content validation if you see value to it.
--
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/1252#discussion_r228362221
Re: [jclouds/jclouds] Filesystem: Fix the MPU ETags to match S3.
(#1252)
Posted by Andrew Phillips <no...@github.com>.
demobox commented on this pull request.
> .append("-")
.append(partsList.size())
.append("\"")
.toString();
assertThat(blob.getMetadata().getETag()).isEqualTo(expectedETag);
}
+ @Test(groups = { "integration", "live" })
+ public void testMultipartUploadMultiplePartsKnownETag() throws Exception {
+ BlobStore blobStore = view.getBlobStore();
+ String container = getContainerName();
+ // Pre-computed ETag returned by AWS S3 for the MPU consisting of two 5MB parts filled with 'b'
+ String expectedETag = "\"84462a16f6a60478d50148808aa609c1-2\"";
Just double-checking that the value that S3 returns is also the value that we'd want here, even with the filesystem provider. Assuming it's worth it, have we been able to verify that other providers produce the same value?
--
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/1252#pullrequestreview-168618950
Re: [jclouds/jclouds] Filesystem: Fix the MPU ETags to match S3.
(#1252)
Posted by Andrew Gaul <no...@github.com>.
Pushed to master as 896e99df097cf20759f89167fb74962854ab19b5. Thank you for your contribution @timuralp!
--
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/1252#issuecomment-435618995