You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by mvrueden <no...@github.com> on 2014/07/01 14:40:13 UTC
[jclouds] JCLOUDS-619 (#427)
Hello,
this is the fix for the issue described here[1].
Please review and let me know if you need something else.
Thanks and kind regards
Markus
[1] https://issues.apache.org/jira/browse/JCLOUDS-619
You can merge this Pull Request by running:
git pull https://github.com/mvrueden/jclouds JCLOUDS-619
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds/pull/427
-- Commit Summary --
* JCLOUDS-619: Introduce MultipartNamingStrategy to generate part names correctly.
* JCLOUDS-619: Add Integration test to verify MultiUploadNamingStrategy works as expected.
-- File Changes --
A apis/swift/src/main/java/org/jclouds/openstack/swift/blobstore/strategy/internal/MultipartNamingStrategy.java (12)
M apis/swift/src/main/java/org/jclouds/openstack/swift/blobstore/strategy/internal/SequentialMultipartUploadStrategy.java (7)
M apis/swift/src/test/java/org/jclouds/openstack/swift/blobstore/integration/SwiftBlobIntegrationLiveTest.java (76)
A apis/swift/src/test/java/org/jclouds/openstack/swift/blobstore/strategy/internal/MultipartNamingStrategyTest.java (54)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/427.patch
https://github.com/jclouds/jclouds/pull/427.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/427
Re: [jclouds] JCLOUDS-619 (#427)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1437](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1437/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/427#issuecomment-48020270
Re: [jclouds] JCLOUDS-619 (#427)
Posted by Andrew Phillips <no...@github.com>.
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +package org.jclouds.openstack.swift.blobstore.strategy.internal;
> +
> +public class MultipartNamingStrategy {
> +
> + private static final String PART_SEPARATOR = "/";
> +
> + protected String getPartName(String key, int partNumber, int totalParts) {
> + int base = (int)Math.log10(totalParts) + 1;
[minor] Is this intended to have the same result as `(int) Math.ceil(Math.log10(totalParts))`? If so, perhaps this is (mathematically) a bit more obvious?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/427/files#r14410182
Re: [jclouds] JCLOUDS-619 (#427)
Posted by Andrew Phillips <no...@github.com>.
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +package org.jclouds.openstack.swift.blobstore.strategy.internal;
> +
> +import org.testng.Assert;
> +import org.testng.annotations.Test;
> +
> +@Test(testName = "MultipartNamingStrategyTest")
> +public class MultipartNamingStrategyTest {
> +
> + @Test
> + public void testGetPartNameFirstOneHundret() {
[minor] Typo? `Hundred`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/427/files#r14410773
Re: [jclouds] JCLOUDS-619 (#427)
Posted by mvrueden <no...@github.com>.
> - .payload(fileToUpload)
> - .build();
> - blobStore.putBlob(containerName, blob, PutOptions.Builder.multipart());
> + private File createFileExactly(long fileSize, String filename) throws IOException {
> + File fileToUpload = new File("target", filename);
> + fileToUpload.delete();
> + final String content = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz";
> + final byte[] contentBytes = content.getBytes("UTF-8");
> + Random random = new Random();
> + ByteSink byteSink = Files.asByteSink(fileToUpload, FileWriteMode.APPEND);
> + for (int i = 0; i < fileSize; i++) {
> + int position = random.nextInt(content.length());
> + byte contentByte = contentBytes[position];
> + byteSink.write(new byte[]{contentByte});
> + }
> + assertEquals(fileSize, fileToUpload.length());
yes
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/427/files#r14412854
Re: [jclouds] JCLOUDS-619 (#427)
Posted by Andrew Phillips <no...@github.com>.
> + String objectName = "file.txt";
> + long countBefore = blobStore.countBlobs(containerName);
> +
> + // we want 11 parts
> + File fileToUpload = createFileExactly(PART_SIZE * 11, objectName);
> + addMultipartBlobToContainer(containerName, objectName, fileToUpload);
> +
> + // did we create enough parts?
> + long countAfter = blobStore.countBlobs(containerName);
> + assertNotEquals(countBefore, countAfter,
> + "No blob was created");
> + assertTrue(countAfter - countBefore > 1,
> + "A multipart blob wasn't actually created - " +
> + "there was only 1 extra blob but there should be one manifest blob and multiple chunk blobs");
> + assertEquals(countBefore + 12, countAfter,
> + "12 parts (11 objects + 1 manifest) were expected.");
`assertEquals(countAfter, countBefore + 12, ...)` TestNG uses "actual, expected".
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/427/files#r14410414
Re: [jclouds] JCLOUDS-619 (#427)
Posted by Andrew Phillips <no...@github.com>.
> + File fileToUpload = createFileExactly(PART_SIZE * 11, objectName);
> + addMultipartBlobToContainer(containerName, objectName, fileToUpload);
> +
> + // did we create enough parts?
> + long countAfter = blobStore.countBlobs(containerName);
> + assertNotEquals(countBefore, countAfter,
> + "No blob was created");
> + assertTrue(countAfter - countBefore > 1,
> + "A multipart blob wasn't actually created - " +
> + "there was only 1 extra blob but there should be one manifest blob and multiple chunk blobs");
> + assertEquals(countBefore + 12, countAfter,
> + "12 parts (11 objects + 1 manifest) were expected.");
> +
> + // download and check again
> + Blob read = blobStore.getBlob(containerName, objectName);
> + File outFile = new File("target", "file-read.txt");
Create a temporary file here instead?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/427/files#r14410625
Re: [jclouds] JCLOUDS-619 (#427)
Posted by Andrew Phillips <no...@github.com>.
> as requested now with 3 indentation spaces.
Thanks, @mvrueden and @andrewgaul!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/427#issuecomment-48063518
Re: [jclouds] JCLOUDS-619 (#427)
Posted by Andrew Gaul <no...@github.com>.
> - blobStore.createContainerInLocation(null, containerName);
> - Blob blob = blobStore.blobBuilder(key)
> - .payload(fileToUpload)
> - .build();
> - blobStore.putBlob(containerName, blob, PutOptions.Builder.multipart());
> + private File createFileExactly(long fileSize, String filename) throws IOException {
> + File fileToUpload = new File("target", filename);
> + fileToUpload.delete();
> + final String content = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz";
> + final byte[] contentBytes = content.getBytes("UTF-8");
> + Random random = new Random();
> + ByteSink byteSink = Files.asByteSink(fileToUpload, FileWriteMode.APPEND);
> + for (int i = 0; i < fileSize; i++) {
> + int position = random.nextInt(content.length());
> + byte contentByte = contentBytes[position];
> + byteSink.write(new byte[]{contentByte});
All of this code will go away with the suggested `ByteSource` changes, but note that this code is wildly inefficient, requiring one system call per byte.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/427/files#r14439033
Re: [jclouds] JCLOUDS-619 (#427)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1305](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1305/) 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/427#issuecomment-47673839
Re: [jclouds] JCLOUDS-619 (#427)
Posted by Andrew Phillips <no...@github.com>.
> + File fileToUpload = createFileExactly(PART_SIZE * 11, objectName);
> + addMultipartBlobToContainer(containerName, objectName, fileToUpload);
> +
> + // did we create enough parts?
> + long countAfter = blobStore.countBlobs(containerName);
> + assertNotEquals(countBefore, countAfter,
> + "No blob was created");
> + assertTrue(countAfter - countBefore > 1,
> + "A multipart blob wasn't actually created - " +
> + "there was only 1 extra blob but there should be one manifest blob and multiple chunk blobs");
> + assertEquals(countBefore + 12, countAfter,
> + "12 parts (11 objects + 1 manifest) were expected.");
> +
> + // download and check again
> + Blob read = blobStore.getBlob(containerName, objectName);
> + File outFile = new File("target", "file-read.txt");
> The current implementation is consistent to the ones already existing in that class.
Ah, lazy me for not looking, then! Let's keep as-is - thanks for explaining.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/427/files#r14413591
Re: [jclouds] JCLOUDS-619 (#427)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1419](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1419/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/427#issuecomment-47657995
Re: [jclouds] JCLOUDS-619 (#427)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1323](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1323/) 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/427#issuecomment-47876477
Re: [jclouds] JCLOUDS-619 (#427)
Posted by Andrew Gaul <no...@github.com>.
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +package org.jclouds.openstack.swift.blobstore.strategy.internal;
> +
> +public class MultipartNamingStrategy {
> +
> + private static final String PART_SEPARATOR = "/";
> +
> + protected String getPartName(String key, int partNumber, int totalParts) {
> + int base = (int)Math.log10(totalParts) + 1;
> + String format = String.format("%%s%%s%%0%dd", base);
> + return String.format(format, key, PART_SEPARATOR, partNumber);
Can you rewrite this more simply as:
```
return String.format("%s%s%0" + base + "d", key, PART_SEPARATOR, partNumber);
```
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/427/files#r14438482
Re: [jclouds] JCLOUDS-619 (#427)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1303](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1303/) 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/427#issuecomment-47659365
Re: [jclouds] JCLOUDS-619 (#427)
Posted by Andrew Gaul <no...@github.com>.
Squashed commits and pushed to master as a39eadce50e3bc37abc0e47564ef1698529bec81 and 1.7.x as 0f0207a045ef53157dac6c3d1d9daaee10671f52. Thank you for your contribution @mvrueden and especially for you persistence on testing! I will continue my crusade against `File`-based tests. Some tests predate Guava `ByteSource` and have not yet modernized. I have some pending commits but try to prioritize user pull requests. ;-)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/427#issuecomment-48018973
Re: [jclouds] JCLOUDS-619 (#427)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1302](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1302/) FAILURE
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/427#issuecomment-47653340
Re: [jclouds] JCLOUDS-619 (#427)
Posted by Andrew Phillips <no...@github.com>.
Thanks for the updates, @mvrueden and the detailed review, @andrewgaul!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/427#issuecomment-47846953
Re: [jclouds] JCLOUDS-619 (#427)
Posted by Andrew Phillips <no...@github.com>.
> @@ -45,6 +44,7 @@
> private final BlobToObject blob2Object;
> private final MultipartUploadSlicingAlgorithm algorithm;
> private final PayloadSlicer slicer;
> + private final MultipartNamingStrategy namingStrategy = new MultipartNamingStrategy();
Should this be injected by Guice, rather than instantiated here? That would seem more consistent with all the other "pluggable logic" parts of the code.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/427/files#r14410270
Re: [jclouds] JCLOUDS-619 (#427)
Posted by Andrew Phillips <no...@github.com>.
> @@ -45,6 +44,7 @@
> private final BlobToObject blob2Object;
> private final MultipartUploadSlicingAlgorithm algorithm;
> private final PayloadSlicer slicer;
> + private final MultipartNamingStrategy namingStrategy = new MultipartNamingStrategy();
> Can you point me in the right direction?
Erm...that is a good question. I can't see any obvious place, either. @andrewgaul @zack-shoylev Any ideas?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/427/files#r14414280
Re: [jclouds] JCLOUDS-619 (#427)
Posted by Andrew Gaul <no...@github.com>.
> + long countAfter = blobStore.countBlobs(containerName);
> + assertNotEquals(countBefore, countAfter,
> + "No blob was created");
> + assertTrue(countAfter - countBefore > 1,
> + "A multipart blob wasn't actually created - " +
> + "there was only 1 extra blob but there should be one manifest blob and multiple chunk blobs");
> + assertEquals(countBefore + 12, countAfter,
> + "12 parts (11 objects + 1 manifest) were expected.");
> +
> + // download and check again
> + Blob read = blobStore.getBlob(containerName, objectName);
> + File outFile = new File("target", "file-read.txt");
> + ByteStreams.copy(read.getPayload(), Files.asByteSink(outFile));
> + HashCode expectedHashCode = Files.hash(fileToUpload, Hashing.md5());
> + HashCode actualHashCode = Files.hash(outFile, Hashing.md5());
> + assertEquals(actualHashCode, expectedHashCode);
You should compare the test output without using a temporary file and without hashing, e.g.,
```
assertTrue(Files.asByteSource(fileToUpload).contentEquals(ByteSources.asByteSource(read.getPayload())));
```
This will read better when you convert `fileToUpload` into a pure `ByteSource` instead of a `File`. Note that assertj has an `assertEquals(InputStream, InputStream)` if we bring in that dependency.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/427/files#r14438764
Re: [jclouds] JCLOUDS-619 (#427)
Posted by Andrew Gaul <no...@github.com>.
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +package org.jclouds.openstack.swift.blobstore.strategy.internal;
> +
> +public class MultipartNamingStrategy {
> +
> + private static final String PART_SEPARATOR = "/";
> +
> + protected String getPartName(String key, int partNumber, int totalParts) {
> + int base = (int)Math.log10(totalParts) + 1;
Do we need this complexity? We can name parts with a fixed 8 character width base as in the Swift examples. This should allow 10 million parts and even with the minimum 1 MB part size this allows 100 TB objects. Trying to pick the minimum base also precludes streaming large objects where the caller does not know the final object size.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/427/files#r14438613
Re: [jclouds] JCLOUDS-619 (#427)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1421](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1421/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/427#issuecomment-47666616
Re: [jclouds] JCLOUDS-619 (#427)
Posted by Andrew Phillips <no...@github.com>.
> + public void testMultipartChunkedFilenames() throws InterruptedException, IOException {
> + String containerName = getContainerName();
> + try {
> + BlobStore blobStore = view.getBlobStore();
> + String objectName = "file.txt";
> + long countBefore = blobStore.countBlobs(containerName);
> +
> + // we want 11 parts
> + File fileToUpload = createFileExactly(PART_SIZE * 11, objectName);
> + addMultipartBlobToContainer(containerName, objectName, fileToUpload);
> +
> + // did we create enough parts?
> + long countAfter = blobStore.countBlobs(containerName);
> + assertNotEquals(countBefore, countAfter,
> + "No blob was created");
> + assertTrue(countAfter - countBefore > 1,
Redundant check? If the following assertion is true, this is definitely true? If we want to keep this as a special case check, perhaps `assertNotEquals(countAfter, countBefore + 1, ...)` would be clearer?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/427/files#r14410510
Re: [jclouds] JCLOUDS-619 (#427)
Posted by Andrew Phillips <no...@github.com>.
@mvrueden: Code looks good, just one style comment @andrewgaul noticed. The changes use 4-space indents, but jclouds uses 3 spaces (see the [coding standards doc](https://wiki.apache.org/jclouds/Coding%20Standards)). Could you kindly fix that?
Thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/427#issuecomment-47964490
Re: [jclouds] JCLOUDS-619 (#427)
Posted by mvrueden <no...@github.com>.
@andrewgaul you are right.
The thing is `count1` should be `11` but it is `20`.
Just try
String content = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz\n"; /* 53 chars */
byte[] contentBytes = content.getBytes(Charsets.UTF_8);
ByteSource byteSource = ByteSources.repeatingArrayByteSource(contentBytes).slice(0, 1024);
PayloadSlicer slicer = new BasePayloadSlicer();
Payload payload = new ByteSourcePayload(byteSource);
Iterable<Payload> slice = slicer.slice(payload, 100);
And tell me how many slices would you expect?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/427#issuecomment-47792068
Re: [jclouds] JCLOUDS-619 (#427)
Posted by mvrueden <no...@github.com>.
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +package org.jclouds.openstack.swift.blobstore.strategy.internal;
> +
> +public class MultipartNamingStrategy {
> +
> + private static final String PART_SEPARATOR = "/";
> +
> + protected String getPartName(String key, int partNumber, int totalParts) {
> + int base = (int)Math.log10(totalParts) + 1;
@demobox No it is not. It would be the same than `(int) Math.floor(Math.log10(totalParts))`
@andrewgaul Just tell me how you want it. My thought was the same, but with this solution it just does not matter.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/427/files#r14443364
Re: [jclouds] JCLOUDS-619 (#427)
Posted by Andrew Phillips <no...@github.com>.
> - blobStore.createContainerInLocation(null, containerName);
> - Blob blob = blobStore.blobBuilder(key)
> - .payload(fileToUpload)
> - .build();
> - blobStore.putBlob(containerName, blob, PutOptions.Builder.multipart());
> + private File createFileExactly(long fileSize, String filename) throws IOException {
> + File fileToUpload = new File("target", filename);
> + fileToUpload.delete();
> + final String content = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz";
> + final byte[] contentBytes = content.getBytes("UTF-8");
> + Random random = new Random();
> + ByteSink byteSink = Files.asByteSink(fileToUpload, FileWriteMode.APPEND);
> + for (int i = 0; i < fileSize; i++) {
> + int position = random.nextInt(content.length());
> + byte contentByte = contentBytes[position];
> + byteSink.write(new byte[]{contentByte});
[minor] Add spaces, i.e. `new byte[] { contentByte }`? Thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/427/files#r14410746
Re: [jclouds] JCLOUDS-619 (#427)
Posted by mvrueden <no...@github.com>.
@andrewgaul it works now. I rebased my commits against master.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/427#issuecomment-47872856
Re: [jclouds] JCLOUDS-619 (#427)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1418](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1418/) FAILURE
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/427#issuecomment-47654092
Re: [jclouds] JCLOUDS-619 (#427)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #948](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/948/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/427#issuecomment-47657692
Re: [jclouds] JCLOUDS-619 (#427)
Posted by mvrueden <no...@github.com>.
> + public void testMultipartChunkedFilenames() throws InterruptedException, IOException {
> + String containerName = getContainerName();
> + try {
> + BlobStore blobStore = view.getBlobStore();
> + String objectName = "file.txt";
> + long countBefore = blobStore.countBlobs(containerName);
> +
> + // we want 11 parts
> + File fileToUpload = createFileExactly(PART_SIZE * 11, objectName);
> + addMultipartBlobToContainer(containerName, objectName, fileToUpload);
> +
> + // did we create enough parts?
> + long countAfter = blobStore.countBlobs(containerName);
> + assertNotEquals(countBefore, countAfter,
> + "No blob was created");
> + assertTrue(countAfter - countBefore > 1,
I am fine with both.
I am not sure if we should check fore the object count in general. There is another test which already tests the object/manifest relation.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/427/files#r14412792
Re: [jclouds] JCLOUDS-619 (#427)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #960](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/960/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/427#issuecomment-47877572
Re: [jclouds] JCLOUDS-619 (#427)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1427](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1427/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/427#issuecomment-47761632
Re: [jclouds] JCLOUDS-619 (#427)
Posted by Andrew Gaul <no...@github.com>.
@mvrueden why should the count2 for slice2 be 11? You provide 53 bytes as the maximum slice size, and ceil(1024 / 53) = 20. Also note that you can call Guava `Iterables.size(Iterable)` instead of using a for loop. The array size provided to `repeatingArrayByteSource` should not matter; it repeats its input infinitely and only `ByteSource.slice` limits it.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/427#issuecomment-47784686
Re: [jclouds] JCLOUDS-619 (#427)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1314](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1314/) 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/427#issuecomment-47760276
Re: [jclouds] JCLOUDS-619 (#427)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -45,6 +44,7 @@
> private final BlobToObject blob2Object;
> private final MultipartUploadSlicingAlgorithm algorithm;
> private final PayloadSlicer slicer;
> + private final MultipartNamingStrategy namingStrategy = new MultipartNamingStrategy();
It should be pretty straightforward. Just annotate the `MultipartNamingStrategy` as `@Singleton`, as it does not have state and it seems like a good singleton candidate, and add it as a parameter to this class constructor. That's it! :)
You can also consider extracting an interface, and annotate it with the `@ImplementedBy` annotation pointing to the default implementation, but given that currently there is only one, I don't think this is necessary.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/427/files#r14428579
Re: [jclouds] JCLOUDS-619 (#427)
Posted by mvrueden <no...@github.com>.
> @@ -45,6 +44,7 @@
> private final BlobToObject blob2Object;
> private final MultipartUploadSlicingAlgorithm algorithm;
> private final PayloadSlicer slicer;
> + private final MultipartNamingStrategy namingStrategy = new MultipartNamingStrategy();
Yes it definetly should be injected by Guice.
But I didn't find the "entry point".
Can you point me in the right direction?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/427/files#r14412932
Re: [jclouds] JCLOUDS-619 (#427)
Posted by Andrew Phillips <no...@github.com>.
> - .payload(fileToUpload)
> - .build();
> - blobStore.putBlob(containerName, blob, PutOptions.Builder.multipart());
> + private File createFileExactly(long fileSize, String filename) throws IOException {
> + File fileToUpload = new File("target", filename);
> + fileToUpload.delete();
> + final String content = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz";
> + final byte[] contentBytes = content.getBytes("UTF-8");
> + Random random = new Random();
> + ByteSink byteSink = Files.asByteSink(fileToUpload, FileWriteMode.APPEND);
> + for (int i = 0; i < fileSize; i++) {
> + int position = random.nextInt(content.length());
> + byte contentByte = contentBytes[position];
> + byteSink.write(new byte[]{contentByte});
> + }
> + assertEquals(fileSize, fileToUpload.length());
Which one is actual, which one expected here? Should be `assertEquals(actual, expected)`
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/427/files#r14410726
Re: [jclouds] JCLOUDS-619 (#427)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #956](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/956/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/427#issuecomment-47761344
Re: [jclouds] JCLOUDS-619 (#427)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1431](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1431/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/427#issuecomment-47878071
Re: [jclouds] JCLOUDS-619 (#427)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #950](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/950/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/427#issuecomment-47665860
Re: [jclouds] JCLOUDS-619 (#427)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #947](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/947/) FAILURE
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/427#issuecomment-47653848
Re: [jclouds] JCLOUDS-619 (#427)
Posted by Andrew Gaul <no...@github.com>.
@mvrueden Please convert test inputs into Guava `ByteSource` since these have less overhead and require less code. I have already done this to a few instances in jclouds code and plan to do more; we should not add more `File`-based test inputs.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/427#issuecomment-47732008
Re: [jclouds] JCLOUDS-619 (#427)
Posted by mvrueden <no...@github.com>.
@demobox as requested now with 3 indentation spaces.
I just want to point out, that the `SwiftBlobIntegrationLiveTest` may need some additional care in the future, e.g. @andrewgaul mentioned, that File-based tests should not be used anymore :)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/427#issuecomment-48014604
Re: [jclouds] JCLOUDS-619 (#427)
Posted by Andrew Gaul <no...@github.com>.
Closed #427.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/427#event-138224633
Re: [jclouds] JCLOUDS-619 (#427)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1331](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1331/) 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/427#issuecomment-48017703
Re: [jclouds] JCLOUDS-619 (#427)
Posted by Andrew Gaul <no...@github.com>.
>
> - BlobStore blobStore = view.getBlobStore();
> - blobStore.createContainerInLocation(null, containerName);
> - Blob blob = blobStore.blobBuilder(key)
> - .payload(fileToUpload)
> - .build();
> - blobStore.putBlob(containerName, blob, PutOptions.Builder.multipart());
> + private File createFileExactly(long fileSize, String filename) throws IOException {
You an you create an in-memory test input with a ByteSource more cheaply, e.g.,
```
byte[] array = new byte[1024];
Random.nextBytes(array);
ByteSource byteSource = ByteSources.repeatingArrayByteSource(array);
```
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/427/files#r14438698
Re: [jclouds] JCLOUDS-619 (#427)
Posted by mvrueden <no...@github.com>.
> + File fileToUpload = createFileExactly(PART_SIZE * 11, objectName);
> + addMultipartBlobToContainer(containerName, objectName, fileToUpload);
> +
> + // did we create enough parts?
> + long countAfter = blobStore.countBlobs(containerName);
> + assertNotEquals(countBefore, countAfter,
> + "No blob was created");
> + assertTrue(countAfter - countBefore > 1,
> + "A multipart blob wasn't actually created - " +
> + "there was only 1 extra blob but there should be one manifest blob and multiple chunk blobs");
> + assertEquals(countBefore + 12, countAfter,
> + "12 parts (11 objects + 1 manifest) were expected.");
> +
> + // download and check again
> + Blob read = blobStore.getBlob(containerName, objectName);
> + File outFile = new File("target", "file-read.txt");
I am fine with that too.
The current implementation is consistent to the ones already existing in that class.
Just let me know what you want.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/427/files#r14412831
Re: [jclouds] JCLOUDS-619 (#427)
Posted by Zack Shoylev <no...@github.com>.
Looks good, esp with the tests. Thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/427#issuecomment-47955449
Re: [jclouds] JCLOUDS-619 (#427)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #966](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/966/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/427#issuecomment-48019665
Re: [jclouds] JCLOUDS-619 (#427)
Posted by Andrew Phillips <no...@github.com>.
> +import org.testng.Assert;
> +import org.testng.annotations.Test;
> +
> +@Test(testName = "MultipartNamingStrategyTest")
> +public class MultipartNamingStrategyTest {
> +
> + @Test
> + public void testGetPartNameFirstOneHundret() {
> + final MultipartNamingStrategy strategy = new MultipartNamingStrategy();
> + final String key = "file.txt";
> + final int numberParts = 100;
> +
> + // check the first 100
> + for (int i = 0; i < numberParts; i++) {
> + String partName = strategy.getPartName(key, i + 1, numberParts);
> + Assert.assertEquals(String.format("file.txt/%03d", i + 1), partName);
[minor] Here and below, static import `assertEquals`? Also, should be `assertEquals(partName, ...)` (i.e. "actual, expected").
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/427/files#r14410828
Re: [jclouds] JCLOUDS-619 (#427)
Posted by mvrueden <no...@github.com>.
I considered your feedback and updated the code.
I stumbled upon another issue.
It is a combination of `ByteSources.repeatingArrayByteSource(...)` and `PayloadSlicer.slice(Payload, Int)` method.
Have a look at this test case
@Test
public void testIterableSliceWithRepeatingByteSource() throws IOException {
String content = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz\n"; /* 53 chars */
byte[] contentBytes = content.getBytes(Charsets.UTF_8);
ByteSource byteSource = ByteSources.repeatingArrayByteSource(contentBytes).slice(0, 1024);
PayloadSlicer slicer = new BasePayloadSlicer();
Payload payload = new ByteSourcePayload(byteSource);
Iterable<Payload> slice = slicer.slice(payload, 100);
Iterable<Payload> slice2 = slicer.slice(payload, 53);
int count1 = 0, count2 = 0;
for (Payload eachPayload : slice) count1++;
for (Payload eachPayload : slice2) count2++;
assertNotEquals(20, count2);
assertEquals(11, count1);
}
The correct slice count would be 11, but it is actually 20.
This problem should always occur when:
Using `Bytesources.repeatingArrayByteSource(someArray)` method and the chunk size is greater than the length of `someArray` (e.g. 100 vs 53). Then `slicer.slice(payload, chunkSize)` will always return a Payload with content length of `someArray.length` (e.g. 53)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/427#issuecomment-47757796
Re: [jclouds] JCLOUDS-619 (#427)
Posted by Andrew Phillips <no...@github.com>.
Thanks for the rebase, @mvrueden.
@andrewgaul: Good to go for you (after a squash, I guess)?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/427#issuecomment-47944510
Re: [jclouds] JCLOUDS-619 (#427)
Posted by Andrew Gaul <no...@github.com>.
@mvrueden `BasePayloadSlicer.getNextPayload` did not handle short reads correctly and I pushed a fix to master as d43c3ea3e0e31663657bb8d99209a3535b5bfcc6 and 1.7.x as c85d728a5a6e8bec3283703aa2fda22aef4dd925. Please rebase your commit against master and test again.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/427#issuecomment-47845899