You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Roman Coedo <no...@github.com> on 2014/06/19 02:23:34 UTC
[jclouds-labs-aws] JCLOUDS-457: Added initiateMultipartUpload and
uploadPart operations. (#18)
Now the Glacier client supports initiateMultipartUpload and
uploadPart operations.
You can merge this Pull Request by running:
git pull https://github.com/rcoedo/jclouds-labs-aws multipart
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds-labs-aws/pull/18
-- Commit Summary --
* JCLOUDS-457: Added initiateMultipartUpload and uploadPart operations.
-- File Changes --
M glacier/src/main/java/org/jclouds/glacier/GlacierAsyncClient.java (40)
M glacier/src/main/java/org/jclouds/glacier/GlacierClient.java (37)
A glacier/src/main/java/org/jclouds/glacier/binders/BindContentRangeToHeaders.java (40)
A glacier/src/main/java/org/jclouds/glacier/binders/BindPartSizeToHeaders.java (40)
A glacier/src/main/java/org/jclouds/glacier/functions/ParseMultipartUploadIdHeader.java (38)
A glacier/src/main/java/org/jclouds/glacier/functions/ParseMultipartUploadTreeHashHeader.java (38)
A glacier/src/main/java/org/jclouds/glacier/predicates/validators/PartSizeValidator.java (43)
M glacier/src/main/java/org/jclouds/glacier/reference/GlacierHeaders.java (2)
A glacier/src/main/java/org/jclouds/glacier/util/ContentRange.java (88)
M glacier/src/test/java/org/jclouds/glacier/GlacierClientMockTest.java (34)
A glacier/src/test/java/org/jclouds/glacier/predicates/validators/PartSizeValidatorTest.java (52)
-- Patch Links --
https://github.com/jclouds/jclouds-labs-aws/pull/18.patch
https://github.com/jclouds/jclouds-labs-aws/pull/18.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/18
Re: [jclouds-labs-aws] JCLOUDS-457: Added initiateMultipartUpload and
uploadPart operations. (#18)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-aws-pull-requests #37](https://jclouds.ci.cloudbees.com/job/jclouds-labs-aws-pull-requests/37/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/18#issuecomment-46511241
Re: [jclouds-labs-aws] JCLOUDS-457: Added initiateMultipartUpload and
uploadPart operations. (#18)
Posted by Jeremy Daggett <no...@github.com>.
@rcoedo @andrewgaul This PR caught my eye because I noticed that there is a `GlacierAsyncClient` checked in. Can we please use a sync interface and remove the async one?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/18#issuecomment-46608092
Re: [jclouds-labs-aws] JCLOUDS-457: Added initiateMultipartUpload and
uploadPart operations. (#18)
Posted by Andrew Gaul <no...@github.com>.
> + MockResponse mr = buildBaseResponse(204);
> + mr.addHeader(GlacierHeaders.TREE_HASH, TREEHASH);
> + server.enqueue(mr);
> +
> + assertEquals(
> + client.uploadPart(VAULT_NAME, MULTIPART_UPLOAD_ID, ContentRange.Builder.fromPartNumber(0, 4), buildPayload(4 * MiB)),
> + TREEHASH);
> + RecordedRequest request = server.takeRequest();
> + assertEquals(request.getRequestLine(),
> + "PUT /-/vaults/" + VAULT_NAME + "/multipart-uploads/" + MULTIPART_UPLOAD_ID + " " + HTTP);
> + assertEquals(request.getHeader(HttpHeaders.CONTENT_RANGE), "bytes 0-4194303/*");
> + assertEquals(request.getHeader(HttpHeaders.CONTENT_LENGTH), "4194304");
> + }
> +
> + @Test
> + //TODO Change size to 4096 when moving to JDK 7
Also maybe hoist comment above the annotation.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/18/files#r13990351
Re: [jclouds-labs-aws] JCLOUDS-457: Added initiateMultipartUpload and
uploadPart operations. (#18)
Posted by Andrew Gaul <no...@github.com>.
> +
> + @Test
> + public void testUploadPart() throws InterruptedException {
> + MockResponse mr = buildBaseResponse(204);
> + mr.addHeader(GlacierHeaders.TREE_HASH, TREEHASH);
> + server.enqueue(mr);
> +
> + assertEquals(
> + client.uploadPart(VAULT_NAME, MULTIPART_UPLOAD_ID, ContentRange.Builder.fromPartNumber(0, 4), buildPayload(4 * MiB)),
> + TREEHASH);
> + RecordedRequest request = server.takeRequest();
> + assertEquals(request.getRequestLine(),
> + "PUT /-/vaults/" + VAULT_NAME + "/multipart-uploads/" + MULTIPART_UPLOAD_ID + " " + HTTP);
> + assertEquals(request.getHeader(HttpHeaders.CONTENT_RANGE), "bytes 0-4194303/*");
> + assertEquals(request.getHeader(HttpHeaders.CONTENT_LENGTH), "4194304");
> + }
Can you add a unit test for the max size?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/18/files#r13948429
Re: [jclouds-labs-aws] JCLOUDS-457: Added initiateMultipartUpload and
uploadPart operations. (#18)
Posted by Andrew Gaul <no...@github.com>.
> +
> + public static final class Builder {
> + public static ContentRange fromString(String contentRangeString) {
> + if (isNullOrEmpty(contentRangeString) || !contentRangeString.matches("[0-9]+-[0-9]+"))
> + throw exception("The string should be two numbers separated by a hyphen (from-to)");
> + String[] strings = contentRangeString.split("-", 2);
> + long from = Long.parseLong(strings[0]);
> + long to = Long.parseLong(strings[1]);
> + return new ContentRange(from, to);
> + }
> +
> + public static ContentRange fromPartNumber(long partNumber, long partSizeInMB) {
> + if (partNumber < 0)
> + throw exception("The part number cannot be negative");
> + if (partSizeInMB < 1)
> + throw exception("The part size has to be possitive");
spelling: positive
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/18/files#r13990129
Re: [jclouds-labs-aws] JCLOUDS-457: Added initiateMultipartUpload and
uploadPart operations. (#18)
Posted by Andrew Gaul <no...@github.com>.
> + @Override
> + public boolean equals(Object obj) {
> + if (obj == null)
> + return false;
> + if (getClass() != obj.getClass())
> + return false;
> + ContentRange other = (ContentRange) obj;
> + return Objects.equal(this.from, other.to) && Objects.equal(this.from, other.to);
> + }
> +
> + @Override
> + public String toString() {
> + return from + "-" + to;
> + }
> +
> + public static final class Builder {
What benefit does `Builder` give in this case? All of its methods are single factories which return a `ContentRange`. Do these belong as part of the outer class instead of the inner class?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/18/files#r13990114
Re: [jclouds-labs-aws] JCLOUDS-457: Added initiateMultipartUpload and
uploadPart operations. (#18)
Posted by Andrew Gaul <no...@github.com>.
> +
> +import java.util.Iterator;
> +
> +import com.google.common.base.Objects;
> +import com.google.common.base.Splitter;
> +
> +/**
> + * This class represents a range of bytes.
> + */
> +public class ContentRange {
> + private final long from;
> + private final long to;
> +
> + private ContentRange(long from, long to) {
> + this.from = from;
> + this.to = to;
Should these have some kind of validation, e.g., non-negative, from less than to?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/18/files#r13948488
Re: [jclouds-labs-aws] JCLOUDS-457: Added initiateMultipartUpload and
uploadPart operations. (#18)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs-aws #990](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-aws/990/) 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-labs-aws/pull/18#issuecomment-46545286
Re: [jclouds-labs-aws] JCLOUDS-457: Added initiateMultipartUpload and
uploadPart operations. (#18)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-aws-pull-requests #42](https://jclouds.ci.cloudbees.com/job/jclouds-labs-aws-pull-requests/42/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/18#issuecomment-46618454
Re: [jclouds-labs-aws] JCLOUDS-457: Added initiateMultipartUpload and
uploadPart operations. (#18)
Posted by Andrew Gaul <no...@github.com>.
> +import static com.google.common.base.Strings.isNullOrEmpty;
> +
> +import com.google.common.base.Objects;
> +
> +/**
> + * This class represents a range of bytes.
> + */
> +public final class ContentRange {
> + private final long from;
> + private final long to;
> +
> + private ContentRange(long from, long to) {
> + if (from > to)
> + throw Builder.exception("\"from\" should be lower than \"to\"");
> + if (from < 0 || to < 1)
> + throw Builder.exception("\"from\" cannot be negative and \"to\" has to be possitive");
spelling: positive
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/18/files#r13990182
Re: [jclouds-labs-aws] JCLOUDS-457: Added initiateMultipartUpload and
uploadPart operations. (#18)
Posted by Roman Coedo <no...@github.com>.
I found a way of testing the max size upload request without slowing down the tests, however it might be a little awkward. Any thoughts on this?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/18#issuecomment-46545767
Re: [jclouds-labs-aws] JCLOUDS-457: Added initiateMultipartUpload and
uploadPart operations. (#18)
Posted by Andrew Gaul <no...@github.com>.
> + if (getClass() != obj.getClass())
> + return false;
> + ContentRange other = (ContentRange) obj;
> + return Objects.equal(this.from, other.to) && Objects.equal(this.from, other.to);
> + }
> +
> + @Override
> + public String toString() {
> + return from + "-" + to;
> + }
> +
> + public static class Builder {
> + public static ContentRange fromString(String contentRangeString) {
> + if (!contentRangeString.matches("[0-9]+-[0-9]+"))
> + return null;
> + Iterator<String> strings = Splitter.on('-').split(contentRangeString).iterator();
Easier to just call String.split('-', 2)?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/18/files#r13948534
Re: [jclouds-labs-aws] JCLOUDS-457: Added initiateMultipartUpload and
uploadPart operations. (#18)
Posted by Andrew Gaul <no...@github.com>.
> + return from + "-" + to;
> + }
> +
> + public static class Builder {
> + public static ContentRange fromString(String contentRangeString) {
> + if (!contentRangeString.matches("[0-9]+-[0-9]+"))
> + return null;
> + Iterator<String> strings = Splitter.on('-').split(contentRangeString).iterator();
> + long from = Long.parseLong(strings.next());
> + long to = Long.parseLong(strings.next());
> + return new ContentRange(from, to);
> + }
> +
> + public static ContentRange fromPartNumber(long partNumber, int partSizeInMB) {
> + long from = partNumber * partSizeInMB;
> + long to = from + (partSizeInMB << 20) - 1;
Promote partSizeInMB to long? If partSizeInMB is MAX_PART_SIZE = 4096, then this will overflow the int.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/18/files#r13948396
Re: [jclouds-labs-aws] JCLOUDS-457: Added initiateMultipartUpload and
uploadPart operations. (#18)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs-aws #991](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-aws/991/) 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-labs-aws/pull/18#issuecomment-46545666
Re: [jclouds-labs-aws] JCLOUDS-457: Added initiateMultipartUpload and
uploadPart operations. (#18)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs-aws #996](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-aws/996/) 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-labs-aws/pull/18#issuecomment-46618418
Re: [jclouds-labs-aws] JCLOUDS-457: Added initiateMultipartUpload and
uploadPart operations. (#18)
Posted by Andrew Gaul <no...@github.com>.
Needs a little work on the maximum size otherwise just address the nits and we can merge.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/18#issuecomment-46511614
Re: [jclouds-labs-aws] JCLOUDS-457: Added initiateMultipartUpload and
uploadPart operations. (#18)
Posted by Andrew Gaul <no...@github.com>.
> + * 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.glacier.util;
> +
> +import java.util.Iterator;
> +
> +import com.google.common.base.Objects;
> +import com.google.common.base.Splitter;
> +
> +/**
> + * This class represents a range of bytes.
> + */
> +public class ContentRange {
Mark as final?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/18/files#r13948471
Re: [jclouds-labs-aws] JCLOUDS-457: Added initiateMultipartUpload and
uploadPart operations. (#18)
Posted by Andrew Gaul <no...@github.com>.
> +
> + public static ContentRange fromPartNumber(long partNumber, long partSizeInMB) {
> + if (partNumber < 0)
> + throw exception("The part number cannot be negative");
> + if (partSizeInMB < 1)
> + throw exception("The part size has to be possitive");
> + long from = partNumber * (partSizeInMB << 20);
> + long to = from + (partSizeInMB << 20) - 1;
> + return new ContentRange(from, to);
> + }
> +
> + public static ContentRange build(long from, long to) {
> + return new ContentRange(from, to);
> + }
> +
> + protected static IllegalArgumentException exception(String reason) {
Does this helper give us some advantage? Otherwise can we use `Preconditions.checkArgument` instead? It supports custom formatting with its vararg method.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/18/files#r13990291
Re: [jclouds-labs-aws] JCLOUDS-457: Added initiateMultipartUpload and
uploadPart operations. (#18)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs-aws #989](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-aws/989/) 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-labs-aws/pull/18#issuecomment-46544867
Re: [jclouds-labs-aws] JCLOUDS-457: Added initiateMultipartUpload and
uploadPart operations. (#18)
Posted by Andrew Gaul <no...@github.com>.
> + MockResponse mr = buildBaseResponse(204);
> + mr.addHeader(GlacierHeaders.TREE_HASH, TREEHASH);
> + server.enqueue(mr);
> +
> + assertEquals(
> + client.uploadPart(VAULT_NAME, MULTIPART_UPLOAD_ID, ContentRange.Builder.fromPartNumber(0, 4), buildPayload(4 * MiB)),
> + TREEHASH);
> + RecordedRequest request = server.takeRequest();
> + assertEquals(request.getRequestLine(),
> + "PUT /-/vaults/" + VAULT_NAME + "/multipart-uploads/" + MULTIPART_UPLOAD_ID + " " + HTTP);
> + assertEquals(request.getHeader(HttpHeaders.CONTENT_RANGE), "bytes 0-4194303/*");
> + assertEquals(request.getHeader(HttpHeaders.CONTENT_LENGTH), "4194304");
> + }
> +
> + @Test
> + //TODO Change size to 4096 when moving to JDK 7
Can you format this as // TODO: ? This highlights in some IDEs in specific ways.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/18/files#r13990334
Re: [jclouds-labs-aws] JCLOUDS-457: Added initiateMultipartUpload and
uploadPart operations. (#18)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-aws-pull-requests #40](https://jclouds.ci.cloudbees.com/job/jclouds-labs-aws-pull-requests/40/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/18#issuecomment-46545191
Re: [jclouds-labs-aws] JCLOUDS-457: Added initiateMultipartUpload and
uploadPart operations. (#18)
Posted by Andrew Gaul <no...@github.com>.
Closed #18.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/18#event-133291848
Re: [jclouds-labs-aws] JCLOUDS-457: Added initiateMultipartUpload and
uploadPart operations. (#18)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-aws-pull-requests #39](https://jclouds.ci.cloudbees.com/job/jclouds-labs-aws-pull-requests/39/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/18#issuecomment-46544732
Re: [jclouds-labs-aws] JCLOUDS-457: Added initiateMultipartUpload and
uploadPart operations. (#18)
Posted by Andrew Gaul <no...@github.com>.
@jdaggett de-async is outside the scope of this commit. Can you start a thread on jclouds-dev describing the status of de-async? Do we have any providers without async today?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/18#issuecomment-46608881
Re: [jclouds-labs-aws] JCLOUDS-457: Added initiateMultipartUpload and
uploadPart operations. (#18)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-aws-pull-requests #41](https://jclouds.ci.cloudbees.com/job/jclouds-labs-aws-pull-requests/41/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/18#issuecomment-46545321
Re: [jclouds-labs-aws] JCLOUDS-457: Added initiateMultipartUpload and
uploadPart operations. (#18)
Posted by Andrew Gaul <no...@github.com>.
> + */
> +package org.jclouds.glacier.predicates.validators;
> +
> +import org.jclouds.javax.annotation.Nullable;
> +import org.jclouds.predicates.Validator;
> +
> +/**
> + * Validates the part size parameter used when initiating multipart uploads.
> + */
> +public final class PartSizeValidator extends Validator<Integer> {
> + private static final int MIN_PART_SIZE = 1;
> + private static final int MAX_PART_SIZE = 4096;
> +
> + @Override
> + public void validate(@Nullable Integer partSizeInMB) throws IllegalArgumentException {
> + if (partSizeInMB == null || partSizeInMB < MIN_PART_SIZE ||
Do `Preconditions.checkArgument` and `checkNotNull` make this more succinct?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/18/files#r13948592
Re: [jclouds-labs-aws] JCLOUDS-457: Added initiateMultipartUpload and
uploadPart operations. (#18)
Posted by Andrew Gaul <no...@github.com>.
Tested locally and pushed to master. I believe jclouds can do better with these 4 GB parts although this is outside the scope of Glacier.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/18#issuecomment-46619355