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/25 22:24:35 UTC
[jclouds-labs-aws] JCLOUDS-457: Added initiateJob (#25)
Now the Glacier client supports the initiateJob operation.
You can merge this Pull Request by running:
git pull https://github.com/rcoedo/jclouds-labs-aws jobs
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds-labs-aws/pull/25
-- Commit Summary --
* JCLOUDS-457: Added initiateJob
-- File Changes --
M glacier/src/main/java/org/jclouds/glacier/GlacierAsyncClient.java (14)
M glacier/src/main/java/org/jclouds/glacier/GlacierClient.java (13)
A glacier/src/main/java/org/jclouds/glacier/binders/BindJobRequestToJsonPayload.java (41)
A glacier/src/main/java/org/jclouds/glacier/domain/ArchiveRetrievalJobRequest.java (117)
A glacier/src/main/java/org/jclouds/glacier/domain/InventoryRetrievalJobRequest.java (133)
A glacier/src/main/java/org/jclouds/glacier/domain/InventoryRetrievalParameters.java (103)
A glacier/src/main/java/org/jclouds/glacier/domain/JobRequest.java (35)
A glacier/src/main/java/org/jclouds/glacier/functions/ParseJobIdHeader.java (37)
M glacier/src/main/java/org/jclouds/glacier/reference/GlacierHeaders.java (1)
M glacier/src/test/java/org/jclouds/glacier/GlacierClientMockTest.java (64)
-- Patch Links --
https://github.com/jclouds/jclouds-labs-aws/pull/25.patch
https://github.com/jclouds/jclouds-labs-aws/pull/25.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/25
Re: [jclouds-labs-aws] JCLOUDS-457: Added initiateJob (#25)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-aws-pull-requests #56](https://jclouds.ci.cloudbees.com/job/jclouds-labs-aws-pull-requests/56/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/25#issuecomment-47276190
Re: [jclouds-labs-aws] JCLOUDS-457: Added initiateJob (#25)
Posted by Roman Coedo <no...@github.com>.
> +import org.jclouds.glacier.domain.JobRequest;
> +import org.jclouds.http.HttpRequest;
> +import org.jclouds.json.Json;
> +import org.jclouds.rest.Binder;
> +
> +import com.google.inject.Inject;
> +
> +public class BindJobRequestToJsonPayload implements Binder {
> + @Inject
> + private Json json;
> +
> + @Override
> + public <R extends HttpRequest> R bindToRequest(R request, Object input) {
> + checkArgument(checkNotNull(input, "input") instanceof JobRequest,
> + "This binder is only valid for JobRequest");
> + checkNotNull(request, "request");
I think an IllegalState wouldn't be appropriate here. The state of the binder is correct, it's the argument the one that is Illegal. Since we are using checkNotNull for most null arguments, I think checkNotNull would be the right choice here. Anyway, this argument won't be null when used by the RestAnnotationProcessor.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/25/files#r14287005
Re: [jclouds-labs-aws] JCLOUDS-457: Added initiateJob (#25)
Posted by Roman Coedo <no...@github.com>.
I will address the binder situation on a later PR with the rest of the binders.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/25#issuecomment-47176010
Re: [jclouds-labs-aws] JCLOUDS-457: Added initiateJob (#25)
Posted by Roman Coedo <no...@github.com>.
> + private String endDate;
> + @SerializedName("Limit")
> + private Integer limit;
> + @SerializedName("Marker")
> + private String marker;
> +
> + @ConstructorProperties({ "StartDate", "EndDate", "Limit", "Marker" })
> + private InventoryRetrievalParameters(@Nullable String startDate, @Nullable String endDate,
> + @Nullable Integer limit, @Nullable String marker) {
> + this.startDate = startDate;
> + this.endDate = endDate;
> + this.limit = limit;
> + this.marker = marker;
> + }
> +
> + public InventoryRetrievalParameters() {
I think I misunderstood a conversation on the dev list about this. I thought I had to support both serialization and deserialization. I'll remove it.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/25/files#r14220221
Re: [jclouds-labs-aws] JCLOUDS-457: Added initiateJob (#25)
Posted by Roman Coedo <no...@github.com>.
It's fine by me.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/25#issuecomment-47275545
Re: [jclouds-labs-aws] JCLOUDS-457: Added initiateJob (#25)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-aws-pull-requests #54](https://jclouds.ci.cloudbees.com/job/jclouds-labs-aws-pull-requests/54/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/25#issuecomment-47154586
Re: [jclouds-labs-aws] JCLOUDS-457: Added initiateJob (#25)
Posted by Andrew Phillips <no...@github.com>.
> +import org.jclouds.javax.annotation.Nullable;
> +
> +import com.google.common.base.Objects;
> +import com.google.gson.annotations.SerializedName;
> +
> +public class InventoryRetrievalJobRequest extends JobRequest {
> + private static final String TYPE = "inventory-retrieval";
> +
> + @SerializedName("Description")
> + private final String description;
> + @SerializedName("Format")
> + private final String format;
> + @SerializedName("InventoryRetrievalParameters")
> + private final InventoryRetrievalParameters parameters;
> +
> + @ConstructorProperties({ "Description", "Format", "InventoryRetrievalParameters" })
Three properties mentioned here but only two in the actual constructor?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/25/files#r14219796
Re: [jclouds-labs-aws] JCLOUDS-457: Added initiateJob (#25)
Posted by Andrew Gaul <no...@github.com>.
Looks almost ready to commit; any other comments?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/25#issuecomment-47271480
Re: [jclouds-labs-aws] JCLOUDS-457: Added initiateJob (#25)
Posted by Roman Coedo <no...@github.com>.
This should never happen. We will have a JobId or an HttpError.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/25#issuecomment-47175621
Re: [jclouds-labs-aws] JCLOUDS-457: Added initiateJob (#25)
Posted by Andrew Gaul <no...@github.com>.
> + @Override
> + public String toString() {
> + return "InventoryRetrievalParameters [archiveId=" + archiveId + ", description=" + description + ", range="
> + + range + "]";
> + }
> +
> + public static Builder builder() {
> + return new Builder();
> + }
> +
> + public static class Builder {
> + private String archiveId;
> + private String description;
> + private ContentRange range;
> +
> + public Builder() {
Should this be package-access instead of public-access? Callers should use `ArchiveRetrievalJobRequest.builder` correct?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/25/files#r14262427
Re: [jclouds-labs-aws] JCLOUDS-457: Added initiateJob (#25)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs-aws #1040](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-aws/1040/) 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/25#issuecomment-47276239
Re: [jclouds-labs-aws] JCLOUDS-457: Added initiateJob (#25)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs-aws #1038](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-aws/1038/) 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/25#issuecomment-47176289
Re: [jclouds-labs-aws] JCLOUDS-457: Added initiateJob (#25)
Posted by Roman Coedo <no...@github.com>.
> +import static com.google.common.base.Preconditions.checkNotNull;
> +
> +import org.jclouds.glacier.domain.JobRequest;
> +import org.jclouds.http.HttpRequest;
> +import org.jclouds.json.Json;
> +import org.jclouds.rest.Binder;
> +
> +import com.google.inject.Inject;
> +
> +public class BindJobRequestToJsonPayload implements Binder {
> + @Inject
> + private Json json;
> +
> + @Override
> + public <R extends HttpRequest> R bindToRequest(R request, Object input) {
> + checkArgument(checkNotNull(input, "input") instanceof JobRequest,
Wouldn't the error message always print "but was: Object"? Any idea to improve this?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/25/files#r14286274
Re: [jclouds-labs-aws] JCLOUDS-457: Added initiateJob (#25)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-aws-pull-requests #55](https://jclouds.ci.cloudbees.com/job/jclouds-labs-aws-pull-requests/55/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/25#issuecomment-47176376
Re: [jclouds-labs-aws] JCLOUDS-457: Added initiateJob (#25)
Posted by Andrew Phillips <no...@github.com>.
> +import static com.google.common.base.Preconditions.checkNotNull;
> +
> +import org.jclouds.glacier.domain.JobRequest;
> +import org.jclouds.http.HttpRequest;
> +import org.jclouds.json.Json;
> +import org.jclouds.rest.Binder;
> +
> +import com.google.inject.Inject;
> +
> +public class BindJobRequestToJsonPayload implements Binder {
> + @Inject
> + private Json json;
> +
> + @Override
> + public <R extends HttpRequest> R bindToRequest(R request, Object input) {
> + checkArgument(checkNotNull(input, "input") instanceof JobRequest,
Don't need a `checkNotNull` here because `null instanceof JobRequest` is false, so it will fail. We might want to amend the error message to `"Requires a JobRequest instance but was " + input` or so?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/25/files#r14219718
Re: [jclouds-labs-aws] JCLOUDS-457: Added initiateJob (#25)
Posted by Andrew Phillips <no...@github.com>.
Thanks, @rcoedo! A couple of comments.
Also: do we need a test to ensure we behave correctly if a JobId is not sent in the response from the server, or is that really a situation that should never happen?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/25#issuecomment-47174853
Re: [jclouds-labs-aws] JCLOUDS-457: Added initiateJob (#25)
Posted by Andrew Phillips <no...@github.com>.
> +import org.jclouds.glacier.reference.GlacierHeaders;
> +import org.jclouds.http.HttpException;
> +import org.jclouds.http.HttpResponse;
> +
> +import com.google.common.base.Function;
> +
> +/**
> + * Parses the jobId from the HttpResponse.
> + */
> +public class ParseJobIdHeader implements Function<HttpResponse, String> {
> +
> + @Override
> + public String apply(HttpResponse from) {
> + String id = from.getFirstHeaderOrNull(GlacierHeaders.JOB_ID);
> + if (id == null)
> + throw new HttpException("Did not receive JobId");
[minor] Write as
```
if (id == null) {
throw new HttpException("Did not receive JobId");
}
```
? Thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/25/files#r14219907
Re: [jclouds-labs-aws] JCLOUDS-457: Added initiateJob (#25)
Posted by Andrew Gaul <no...@github.com>.
Squashed and pushed to master as 5cf26f6f7a418c83b93ea1ee2c8495101cdb3a26.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/25#issuecomment-47280537
Re: [jclouds-labs-aws] JCLOUDS-457: Added initiateJob (#25)
Posted by Roman Coedo <no...@github.com>.
> +import org.jclouds.glacier.domain.JobRequest;
> +import org.jclouds.http.HttpRequest;
> +import org.jclouds.json.Json;
> +import org.jclouds.rest.Binder;
> +
> +import com.google.inject.Inject;
> +
> +public class BindJobRequestToJsonPayload implements Binder {
> + @Inject
> + private Json json;
> +
> + @Override
> + public <R extends HttpRequest> R bindToRequest(R request, Object input) {
> + checkArgument(checkNotNull(input, "input") instanceof JobRequest,
> + "This binder is only valid for JobRequest");
> + checkNotNull(request, "request");
Yes, I think I've seen it in the atmos provider. I will make a new PR tomorrow refactoring the binders. thanks @demobox!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/25/files#r14219869
Re: [jclouds-labs-aws] JCLOUDS-457: Added initiateJob (#25)
Posted by Andrew Phillips <no...@github.com>.
> + private String endDate;
> + @SerializedName("Limit")
> + private Integer limit;
> + @SerializedName("Marker")
> + private String marker;
> +
> + @ConstructorProperties({ "StartDate", "EndDate", "Limit", "Marker" })
> + private InventoryRetrievalParameters(@Nullable String startDate, @Nullable String endDate,
> + @Nullable Integer limit, @Nullable String marker) {
> + this.startDate = startDate;
> + this.endDate = endDate;
> + this.limit = limit;
> + this.marker = marker;
> + }
> +
> + public InventoryRetrievalParameters() {
Is this needed? If so, quick comment why?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/25/files#r14219825
Re: [jclouds-labs-aws] JCLOUDS-457: Added initiateJob (#25)
Posted by Andrew Phillips <no...@github.com>.
> +import org.jclouds.glacier.domain.JobRequest;
> +import org.jclouds.http.HttpRequest;
> +import org.jclouds.json.Json;
> +import org.jclouds.rest.Binder;
> +
> +import com.google.inject.Inject;
> +
> +public class BindJobRequestToJsonPayload implements Binder {
> + @Inject
> + private Json json;
> +
> + @Override
> + public <R extends HttpRequest> R bindToRequest(R request, Object input) {
> + checkArgument(checkNotNull(input, "input") instanceof JobRequest,
> + "This binder is only valid for JobRequest");
> + checkNotNull(request, "request");
[lazy question] Do we do this in other binders? A `null` request here sounds more like an illegal state?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/25/files#r14219763
Re: [jclouds-labs-aws] JCLOUDS-457: Added initiateJob (#25)
Posted by Andrew Gaul <no...@github.com>.
Closed #25.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/25#event-135661325
Re: [jclouds-labs-aws] JCLOUDS-457: Added initiateJob (#25)
Posted by Andrew Phillips <no...@github.com>.
> + private String endDate;
> + @SerializedName("Limit")
> + private Integer limit;
> + @SerializedName("Marker")
> + private String marker;
> +
> + @ConstructorProperties({ "StartDate", "EndDate", "Limit", "Marker" })
> + private InventoryRetrievalParameters(@Nullable String startDate, @Nullable String endDate,
> + @Nullable Integer limit, @Nullable String marker) {
> + this.startDate = startDate;
> + this.endDate = endDate;
> + this.limit = limit;
> + this.marker = marker;
> + }
> +
> + public InventoryRetrievalParameters() {
> I thought I had to support both serialization and deserialization.
Wait...you may be quite correct - I could well be the one getting things wrong here ;-)
@nacx: thoughts..?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/25/files#r14220285