You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Christopher Dancy <no...@github.com> on 2015/08/07 18:54:27 UTC
[jclouds-labs] JCLOUDS-985: Add members API (#196)
Members API has been added with mock and integration tests of which all pass successfully. I refactored/formatted some code so a few more files than I would have liked were touched and made this PR just a little bit bigger than I would have liked.
Please pick it apart and let me know how things look!
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds-labs/pull/196
-- Commit Summary --
* JCLOUDS-985: Add members API
* Fix tester
-- File Changes --
M etcd/src/main/java/org/jclouds/etcd/EtcdApi.java (6)
M etcd/src/main/java/org/jclouds/etcd/EtcdApiMetadata.java (18)
M etcd/src/main/java/org/jclouds/etcd/config/EtcdHttpApiModule.java (10)
A etcd/src/main/java/org/jclouds/etcd/domain/members/Member.java (51)
A etcd/src/main/java/org/jclouds/etcd/fallbacks/EtcdFallbacks.java (48)
A etcd/src/main/java/org/jclouds/etcd/features/MembersApi.java (72)
M etcd/src/main/java/org/jclouds/etcd/features/StatisticsApi.java (16)
A etcd/src/main/java/org/jclouds/etcd/handlers/EtcdErrorHandler.java (94)
A etcd/src/test/java/org/jclouds/etcd/features/MembersApiLiveTest.java (103)
A etcd/src/test/java/org/jclouds/etcd/features/MembersApiMockTest.java (166)
A etcd/src/test/resources/members-add-existent.json (1)
A etcd/src/test/resources/members-add-illegal-format.json (1)
A etcd/src/test/resources/members-add-malformed-url.json (1)
A etcd/src/test/resources/members-added.json (9)
A etcd/src/test/resources/members-delete-nonexistent.json (1)
A etcd/src/test/resources/members.json (24)
-- Patch Links --
https://github.com/jclouds/jclouds-labs/pull/196.patch
https://github.com/jclouds/jclouds-labs/pull/196.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/196
Re: [jclouds-labs] JCLOUDS-985: Add members API (#196)
Posted by Christopher Dancy <no...@github.com>.
> +
> + switch (response.getStatusCode()) {
> + case 404:
> + if (command.getCurrentRequest().getMethod().equals("DELETE")) {
> + if (command.getCurrentRequest().getEndpoint().getPath().startsWith(
> + "/" + EtcdApiMetadata.API_VERSION + "/members") && message.contains("No such member")) {
> + exception = new ResourceNotFoundException(message,
> + new HttpResponseException(command, response, message));
> + }
> + }
> + break;
> + case 409:
> + if (command.getCurrentRequest().getMethod().equals("POST")) {
> + if (command.getCurrentRequest().getEndpoint().getPath().endsWith(
> + EtcdApiMetadata.API_VERSION + "/members") && message.contains("etcdserver: ID exists"))
> + exception = new HttpResponseException(message, command, response);
Makes sense now as the API is smaller. Once I flesh things out we may need to return and do it this way but for now, if only for simplicities sake, I'm on board.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/196/files#r37198776
Re: [jclouds-labs] JCLOUDS-985: Add members API (#196)
Posted by Ignasi Barrera <no...@github.com>.
Works for me @cdancy. @zack-shoylev?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/196#issuecomment-140014451
Re: [jclouds-labs] JCLOUDS-985: Add members API (#196)
Posted by Ignasi Barrera <no...@github.com>.
> + protected void init() {
> + selfID = api.statisticsApi().self().id();
> + assertNotNull(selfID);
> + }
> +
> + public void testListMembers() {
> + List<Member> members = api().list();
> + assertNotNull(members);
> + assertTrue(members.size() > 0);
> + for (Member member : members) {
> + if (!member.id().equals(selfID)) {
> + this.nonSelfMember = member;
> + return;
> + }
> + }
> + throw new RuntimeException("Could not find another member in cluster with different id");
Better use `Assert.fail`
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/196/files#r37588042
Re: [jclouds-labs] JCLOUDS-985: Add members API (#196)
Posted by Zack Shoylev <no...@github.com>.
@cdancy @nacx Alright, then. We can revisit later if needed.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/196#issuecomment-140492945
Re: [jclouds-labs] JCLOUDS-985: Add members API (#196)
Posted by Christopher Dancy <no...@github.com>.
> +import org.testng.annotations.Test;
> +
> +import com.google.common.collect.ImmutableList;
> +
> +@Test(groups = "live", testName = "MembersApiLiveTest")
> +public class MembersApiLiveTest extends BaseEtcdApiLiveTest {
> +
> + private String selfID;
> + private Member nonSelfMember;
> + private Member addedMember;
> +
> + @BeforeClass
> + protected void init() {
> + String id = api.statisticsApi().self().id();
> + assertNotNull(id);
> + selfID = id;
Done
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/196/files#r37198700
Re: [jclouds-labs] JCLOUDS-985: Add members API (#196)
Posted by Ignasi Barrera <no...@github.com>.
> +import org.testng.annotations.Test;
> +
> +import com.google.common.collect.ImmutableList;
> +
> +@Test(groups = "live", testName = "MembersApiLiveTest")
> +public class MembersApiLiveTest extends BaseEtcdApiLiveTest {
> +
> + private String selfID;
> + private Member nonSelfMember;
> + private Member addedMember;
> +
> + @BeforeClass
> + protected void init() {
> + String id = api.statisticsApi().self().id();
> + assertNotNull(id);
> + selfID = id;
Avoid the intermediate variable and directly assign `selfID`.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/196/files#r36680984
Re: [jclouds-labs] JCLOUDS-985: Add members API (#196)
Posted by Ignasi Barrera <no...@github.com>.
> +import org.jclouds.json.SerializedNames;
> +
> +import com.google.auto.value.AutoValue;
> +import com.google.common.collect.ImmutableList;
> +
> +@AutoValue
> +public abstract class Member {
> +
> + public abstract String id();
> +
> + @Nullable
> + public abstract String name();
> +
> + public abstract List<String> peerURLs();
> +
> + @Nullable
Same as above.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/196/files#r36679394
Re: [jclouds-labs] JCLOUDS-985: Add members API (#196)
Posted by Ignasi Barrera <no...@github.com>.
> +
> + switch (response.getStatusCode()) {
> + case 404:
> + if (command.getCurrentRequest().getMethod().equals("DELETE")) {
> + if (command.getCurrentRequest().getEndpoint().getPath().startsWith(
> + "/" + EtcdApiMetadata.API_VERSION + "/members") && message.contains("No such member")) {
> + exception = new ResourceNotFoundException(message,
> + new HttpResponseException(command, response, message));
> + }
> + }
> + break;
> + case 409:
> + if (command.getCurrentRequest().getMethod().equals("POST")) {
> + if (command.getCurrentRequest().getEndpoint().getPath().endsWith(
> + EtcdApiMetadata.API_VERSION + "/members") && message.contains("etcdserver: ID exists"))
> + exception = new HttpResponseException(message, command, response);
I'd remove the method and path checks, and throw in general a `ResourceAlreadyExistsException ` exception if the ID exists, and the `HttpResponseException` otherwise.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/196/files#r36680821
Re: [jclouds-labs] JCLOUDS-985: Add members API (#196)
Posted by Ignasi Barrera <no...@github.com>.
> +
> + public void handleError(HttpCommand command, HttpResponse response) {
> +
> + String message = parseMessage(response);
> + Exception exception = null;
> + try {
> +
> + message = message != null ? message
> + : String.format("%s -> %s", command.getCurrentRequest().getRequestLine(), response.getStatusLine());
> +
> + switch (response.getStatusCode()) {
> + case 404:
> + if (command.getCurrentRequest().getMethod().equals("DELETE")) {
> + if (command.getCurrentRequest().getEndpoint().getPath().startsWith(
> + "/" + EtcdApiMetadata.API_VERSION + "/members") && message.contains("No such member")) {
> + exception = new ResourceNotFoundException(message,
Can't we safely throw a RNFE on every 404, without checking the method and path?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/196/files#r36680707
Re: [jclouds-labs] JCLOUDS-985: Add members API (#196)
Posted by Zack Shoylev <no...@github.com>.
> +import com.google.auto.value.AutoValue;
> +import com.google.common.collect.ImmutableList;
> +
> +@AutoValue
> +public abstract class Member {
> +
> + public abstract String id();
> +
> + @Nullable
> + public abstract String name();
> +
> + public abstract List<String> peerURLs();
> +
> + public abstract List<String> clientURLs();
> +
> + Member() {
I don't think this is needed?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/196/files#r37631196
Re: [jclouds-labs] JCLOUDS-985: Add members API (#196)
Posted by Zack Shoylev <no...@github.com>.
> +
> +@AutoValue
> +public abstract class CreateMember {
> +
> + @Nullable
> + public abstract String name();
> +
> + public abstract List<String> peerURLs();
> +
> + public abstract List<String> clientURLs();
> +
> + CreateMember() {
> + }
> +
> + @SerializedNames({ "name", "peerURLs", "clientURLs" })
> + public static CreateMember create(final String name, final List<String> peerURLs, List<String> clientURLs) {
You should probably make this method private.
Then, to be able to instantiate CreateMember, provide a builder using auto-value similar to:
https://github.com/jclouds/jclouds-labs-openstack/blob/master/openstack-heat/src/main/java/org/jclouds/openstack/heat/v1/options/CreateStack.java
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/196/files#r37631165
Re: [jclouds-labs] JCLOUDS-985: Add members API (#196)
Posted by Ignasi Barrera <no...@github.com>.
> + public abstract String id();
> +
> + @Nullable
> + public abstract String name();
> +
> + public abstract List<String> peerURLs();
> +
> + public abstract List<String> clientURLs();
> +
> + Member() {
> + }
> +
> + @SerializedNames({ "id", "name", "peerURLs", "clientURLs" })
> + private static Member create(String id, String name, List<String> peerURLs, List<String> clientURLs) {
> + if (clientURLs == null)
> + clientURLs = ImmutableList.of();
In general, treat input parameters as final. The calling code could expect the clientURLs to still be `null` after having called this method.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/196/files#r37587990
Re: [jclouds-labs] JCLOUDS-985: Add members API (#196)
Posted by Ignasi Barrera <no...@github.com>.
Thanks @cdancy! Nice cleanup. I've just added a couple final comments about treating method parameters as final and a minor style thing about making tests fail.
Could you squash the commits into a single one and rebase to the latest master so I can cleanly merge this? There is no need to another fix commit; let's fix the remaining bits while squashing.
Thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/196#issuecomment-133198427
Re: [jclouds-labs] JCLOUDS-985: Add members API (#196)
Posted by Christopher Dancy <no...@github.com>.
@nacx can we merge this? I'm going to backtrack on statements I made above about requiring builders. If it's not necessary at this point I'd rather leave it out. I was not having great success in implementing it and would like to continue moving forward and circle back at some later date to code the builders.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/196#issuecomment-139772645
Re: [jclouds-labs] JCLOUDS-985: Add members API (#196)
Posted by Christopher Dancy <no...@github.com>.
> + */
> + @Named("members:list")
> + @SelectJson("members")
> + @GET
> + List<Member> list();
> +
> + /**
> + * @param member
> + * non-existing member to add to cluster
> + * @return newly created member or null if member previously existed. Because
> + * this call returns before registration is fully complete it is not
> + * guaranteed that the Member class will have any variable, other than
> + * peerURLs, from the CreateMember class present.
> + */
> + @Named("members:add")
> + @Fallback(NullOnAddExistingMemberAnd409.class)
Agreed and done.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/196/files#r37199090
Re: [jclouds-labs] JCLOUDS-985: Add members API (#196)
Posted by Ignasi Barrera <no...@github.com>.
> +
> +@AutoValue
> +public abstract class CreateMember {
> +
> + @Nullable
> + public abstract String name();
> +
> + public abstract List<String> peerURLs();
> +
> + public abstract List<String> clientURLs();
> +
> + CreateMember() {
> + }
> +
> + @SerializedNames({ "name", "peerURLs", "clientURLs" })
> + public static CreateMember create(final String name, final List<String> peerURLs, List<String> clientURLs) {
I'd say don't use builders by default. We are talking about value objects that should be simple by nature. Using builders everywhere by default sounds like unnecessary over-engineering, but that's only my opinion :)
In this case, it has only three fields and perhaps makes more sense to provide several factory methods (if needed).
This said, I think consistency is important, so if the general case in this API is that value objects *users will build* are complex or justify the use of a builder, then use them.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/196/files#r37639213
Re: [jclouds-labs] JCLOUDS-985: Add members API (#196)
Posted by Ignasi Barrera <no...@github.com>.
> + * Handle errors and propagate exception
> + */
> +public class EtcdErrorHandler implements HttpErrorHandler {
> + @Resource
> + protected Logger logger = Logger.NULL;
> +
> + public void handleError(HttpCommand command, HttpResponse response) {
> +
> + String message = parseMessage(response);
> + Exception exception = null;
> + try {
> +
> + message = message != null ? message
> + : String.format("%s -> %s", command.getCurrentRequest().getRequestLine(), response.getStatusLine());
> +
> + switch (response.getStatusCode()) {
I've seen that requests with malformed urls return a 400. Consider catching it here and propagating an `IllegalArgumentException`, as this is the jclouds convention.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/196/files#r36681869
Re: [jclouds-labs] JCLOUDS-985: Add members API (#196)
Posted by Zack Shoylev <no...@github.com>.
> +import com.google.auto.value.AutoValue;
> +import com.google.common.collect.ImmutableList;
> +
> +@AutoValue
> +public abstract class Member {
> +
> + public abstract String id();
> +
> + @Nullable
> + public abstract String name();
> +
> + public abstract List<String> peerURLs();
> +
> + public abstract List<String> clientURLs();
> +
> + Member() {
I see. =\ I am not aware of any issues if these are removed. Unless there is a code doc explaining why these are needed, I think remove.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/196/files#r37632572
Re: [jclouds-labs] JCLOUDS-985: Add members API (#196)
Posted by Zack Shoylev <no...@github.com>.
> +import com.google.auto.value.AutoValue;
> +import com.google.common.collect.ImmutableList;
> +
> +@AutoValue
> +public abstract class Member {
> +
> + public abstract String id();
> +
> + @Nullable
> + public abstract String name();
> +
> + public abstract List<String> peerURLs();
> +
> + public abstract List<String> clientURLs();
> +
> + Member() {
A-ha! Thanks @nacx !
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/196/files#r37639680
Re: [jclouds-labs] JCLOUDS-985: Add members API (#196)
Posted by Christopher Dancy <no...@github.com>.
> + * @return list of members within cluster
> + */
> + @Named("members:list")
> + @SelectJson("members")
> + @GET
> + List<Member> list();
> +
> + /**
> + * @param member
> + * non-existing member to add to cluster
> + * @return newly created member or null if member previously existed
> + */
> + @Named("members:add")
> + @Fallback(NullOnAddExistingMemberAnd409.class)
> + @POST
> + Member add(@BinderParam(BindToJsonPayload.class) Member member);
@zack-shoylev thanks. As a result of your comments I've now made this PR just a little bit bigger by ensuring all non-user-facing value classes have private creation methods. Also separated out the CreateMember from the Member class. All mock/integration tests pass locally.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/196/files#r36545115
Re: [jclouds-labs] JCLOUDS-985: Add members API (#196)
Posted by Ignasi Barrera <no...@github.com>.
> + * peerURLs, from the CreateMember class present.
> + */
> + @Named("members:add")
> + @Fallback(NullOnAddExistingMemberAnd409.class)
> + @POST
> + Member add(@BinderParam(BindToJsonPayload.class) CreateMember memberToCreate);
> +
> + /**
> + * @param memberID
> + * id of previously existing member
> + * @return true if member was deleted or false if id did not match an
> + * existing member
> + */
> + @Named("members:delete")
> + @Path("/{id}")
> + @Fallback(BooleanOnDeleteNonExistentMemberAnd404.class)
Wouldn't it be enough to use the `FalseOnNotFoundOr404` fallback? If not, and you want to catch only responses with a concrete message... it does not look OK from a user perspective to expect a false (on certain circumstances), and an HttpResponseException on other 404.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/196/files#r36680490
Re: [jclouds-labs] JCLOUDS-985: Add members API (#196)
Posted by Zack Shoylev <no...@github.com>.
> +
> +@AutoValue
> +public abstract class CreateMember {
> +
> + @Nullable
> + public abstract String name();
> +
> + public abstract List<String> peerURLs();
> +
> + public abstract List<String> clientURLs();
> +
> + CreateMember() {
> + }
> +
> + @SerializedNames({ "name", "peerURLs", "clientURLs" })
> + public static CreateMember create(final String name, final List<String> peerURLs, List<String> clientURLs) {
Thanks @cdancy ! That sounds perfect.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/196/files#r37645426
Re: [jclouds-labs] JCLOUDS-985: Add members API (#196)
Posted by Ignasi Barrera <no...@github.com>.
> +
> + public void testListMembers() throws Exception {
> + List<Member> members = api().list();
> + assertNotNull(members);
> + assertTrue(members.size() > 0);
> + for (Member member : members) {
> + if (!member.id().equals(selfID)) {
> + this.nonSelfMember = member;
> + break;
> + }
> + }
> + }
> +
> + @Test(dependsOnMethods = "testListMembers")
> + public void testDeleteMember() {
> + assertNotNull(nonSelfMember);
Fail the test or skip it?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/196/files#r36681558
Re: [jclouds-labs] JCLOUDS-985: Add members API (#196)
Posted by Ignasi Barrera <no...@github.com>.
Closed #196.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/196#event-410336297
Re: [jclouds-labs] JCLOUDS-985: Add members API (#196)
Posted by Ignasi Barrera <no...@github.com>.
> +import org.jclouds.javax.annotation.Nullable;
> +import org.jclouds.json.SerializedNames;
> +
> +import com.google.auto.value.AutoValue;
> +import com.google.common.collect.ImmutableList;
> +
> +@AutoValue
> +public abstract class CreateMember {
> +
> + @Nullable
> + public abstract String name();
> +
> + public abstract List<String> peerURLs();
> +
> + @Nullable
> + public abstract List<String> clientURLs();
The factory method makes sure this is never null, so better remove the `@Nullable` annotation.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/196/files#r36679369
Re: [jclouds-labs] JCLOUDS-985: Add members API (#196)
Posted by Ignasi Barrera <no...@github.com>.
> +import com.google.auto.value.AutoValue;
> +import com.google.common.collect.ImmutableList;
> +
> +@AutoValue
> +public abstract class Member {
> +
> + public abstract String id();
> +
> + @Nullable
> + public abstract String name();
> +
> + public abstract List<String> peerURLs();
> +
> + public abstract List<String> clientURLs();
> +
> + Member() {
It is a best practice. See: https://github.com/google/auto/blob/master/value/README.md#best-practices
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/196/files#r37638183
Re: [jclouds-labs] JCLOUDS-985: Add members API (#196)
Posted by Zack Shoylev <no...@github.com>.
> + @Nullable
> + public abstract String id();
> +
> + @Nullable
> + public abstract String name();
> +
> + public abstract List<String> peerURLs();
> +
> + @Nullable
> + public abstract List<String> clientURLs();
> +
> + Member() {
> + }
> +
> + @SerializedNames({ "id", "name", "peerURLs", "clientURLs" })
> + public static Member create(String id, String name, List<String> peerURLs, List<String> clientURLs) {
Make this create method private, it shouldn't be visible to users.
Make another copy of this class, call it CreateMember, make sure you use AutoValue's builders to make sure a user can instantiate properly and then pass the object to:
Member add(@BinderParam(BindToJsonPayload.class) CreateMember member);
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/196/files#r36539565
Re: [jclouds-labs] JCLOUDS-985: Add members API (#196)
Posted by Ignasi Barrera <no...@github.com>.
> + */
> + @Named("members:list")
> + @SelectJson("members")
> + @GET
> + List<Member> list();
> +
> + /**
> + * @param member
> + * non-existing member to add to cluster
> + * @return newly created member or null if member previously existed. Because
> + * this call returns before registration is fully complete it is not
> + * guaranteed that the Member class will have any variable, other than
> + * peerURLs, from the CreateMember class present.
> + */
> + @Named("members:add")
> + @Fallback(NullOnAddExistingMemberAnd409.class)
Don't use 409 fallbacks in POST/PUT methods. What does a `null` value mean to users? Although documented it is not a good practice: does it mean the member was not created (and does not exist) because the operation failed? Does it mean there was one that already existed? Returning `null` is too ambiguous and we don't use fallbacks in write operations (POST/PUT).
Let the method throw an exception. jclouds already has the `ResourceAlreadyExistsException` exception, so configure the error handler to throw it in this case instead of using a fallback.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/196/files#r36680199
Re: [jclouds-labs] JCLOUDS-985: Add members API (#196)
Posted by Christopher Dancy <no...@github.com>.
@nacx should be good to go. Thanks for the reviews and helpful comments getting the members API fleshed out.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/196#issuecomment-133246567
Re: [jclouds-labs] JCLOUDS-985: Add members API (#196)
Posted by Christopher Dancy <no...@github.com>.
> +
> + public void handleError(HttpCommand command, HttpResponse response) {
> +
> + String message = parseMessage(response);
> + Exception exception = null;
> + try {
> +
> + message = message != null ? message
> + : String.format("%s -> %s", command.getCurrentRequest().getRequestLine(), response.getStatusLine());
> +
> + switch (response.getStatusCode()) {
> + case 404:
> + if (command.getCurrentRequest().getMethod().equals("DELETE")) {
> + if (command.getCurrentRequest().getEndpoint().getPath().startsWith(
> + "/" + EtcdApiMetadata.API_VERSION + "/members") && message.contains("No such member")) {
> + exception = new ResourceNotFoundException(message,
We can and will do so now. Once the API gets bigger we will have more use-cases, but as I've noted elsewhere, we can do this now for simplicities sake.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/196/files#r37198886
Re: [jclouds-labs] JCLOUDS-985: Add members API (#196)
Posted by Zack Shoylev <no...@github.com>.
> + * @return list of members within cluster
> + */
> + @Named("members:list")
> + @SelectJson("members")
> + @GET
> + List<Member> list();
> +
> + /**
> + * @param member
> + * non-existing member to add to cluster
> + * @return newly created member or null if member previously existed
> + */
> + @Named("members:add")
> + @Fallback(NullOnAddExistingMemberAnd409.class)
> + @POST
> + Member add(@BinderParam(BindToJsonPayload.class) Member member);
Some notes above. It makes more sense to have to different classes to handle what you send to the service and what you get back from the service, usually, at least for future-proofing.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/196/files#r36539649
Re: [jclouds-labs] JCLOUDS-985: Add members API (#196)
Posted by Christopher Dancy <no...@github.com>.
@nacx all comments should be addressed but it looks like the jdbc provider is having issues.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/196#issuecomment-132312032
Re: [jclouds-labs] JCLOUDS-985: Add members API (#196)
Posted by Zack Shoylev <no...@github.com>.
> +
> +@AutoValue
> +public abstract class CreateMember {
> +
> + @Nullable
> + public abstract String name();
> +
> + public abstract List<String> peerURLs();
> +
> + public abstract List<String> clientURLs();
> +
> + CreateMember() {
> + }
> +
> + @SerializedNames({ "name", "peerURLs", "clientURLs" })
> + public static CreateMember create(final String name, final List<String> peerURLs, List<String> clientURLs) {
@nacx I just think it's convenient and consistent to provide the Builder, especially because auto-value takes care of most of it.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/196/files#r37639569
Re: [jclouds-labs] JCLOUDS-985: Add members API (#196)
Posted by Christopher Dancy <no...@github.com>.
> +
> +@AutoValue
> +public abstract class CreateMember {
> +
> + @Nullable
> + public abstract String name();
> +
> + public abstract List<String> peerURLs();
> +
> + public abstract List<String> clientURLs();
> +
> + CreateMember() {
> + }
> +
> + @SerializedNames({ "name", "peerURLs", "clientURLs" })
> + public static CreateMember create(final String name, final List<String> peerURLs, List<String> clientURLs) {
@zack-shoylev I think you made the point previously, maybe here or on IRC, that I agree with in that builders, at least so far as AutoValue classes are concerned, should be used when those classes are client facing. In this case the CreateMember class would be just that and so I think it makes sense to Builderize (?!?) it. Other classes which are not client facing, and are simply handed back from a function call, need not have this functionality. Maybe this is obvious but I thought it a point worth making. I'm actually writing up the builder class now so if we are in agreement I can send in another squashed commit with the builder functionality added on top. Going forward I will follow the same model with this provider, that being: client facing value classes will get builders if only to keep things consistent.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/196/files#r37641771
Re: [jclouds-labs] JCLOUDS-985: Add members API (#196)
Posted by Christopher Dancy <no...@github.com>.
> + * peerURLs, from the CreateMember class present.
> + */
> + @Named("members:add")
> + @Fallback(NullOnAddExistingMemberAnd409.class)
> + @POST
> + Member add(@BinderParam(BindToJsonPayload.class) CreateMember memberToCreate);
> +
> + /**
> + * @param memberID
> + * id of previously existing member
> + * @return true if member was deleted or false if id did not match an
> + * existing member
> + */
> + @Named("members:delete")
> + @Path("/{id}")
> + @Fallback(BooleanOnDeleteNonExistentMemberAnd404.class)
Removed and instead will throw a ResourceNotFoundException. This seems to make the most sense IMO.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/196/files#r37199075
Re: [jclouds-labs] JCLOUDS-985: Add members API (#196)
Posted by Christopher Dancy <no...@github.com>.
> + * Handle errors and propagate exception
> + */
> +public class EtcdErrorHandler implements HttpErrorHandler {
> + @Resource
> + protected Logger logger = Logger.NULL;
> +
> + public void handleError(HttpCommand command, HttpResponse response) {
> +
> + String message = parseMessage(response);
> + Exception exception = null;
> + try {
> +
> + message = message != null ? message
> + : String.format("%s -> %s", command.getCurrentRequest().getRequestLine(), response.getStatusLine());
> +
> + switch (response.getStatusCode()) {
Agreed and done.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/196/files#r37198896
Re: [jclouds-labs] JCLOUDS-985: Add members API (#196)
Posted by Christopher Dancy <no...@github.com>.
> +import com.google.auto.value.AutoValue;
> +import com.google.common.collect.ImmutableList;
> +
> +@AutoValue
> +public abstract class Member {
> +
> + public abstract String id();
> +
> + @Nullable
> + public abstract String name();
> +
> + public abstract List<String> peerURLs();
> +
> + public abstract List<String> clientURLs();
> +
> + Member() {
@zack-shoylev just following conventions I've seen elsewhere within jclouds-labs by providing an empty constructor for AutoValue classes. If the standard going forward is to remove these, as they are not really needed anyway, then I'm more than happy to remove it ;)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/196/files#r37632279
Re: [jclouds-labs] JCLOUDS-985: Add members API (#196)
Posted by Ignasi Barrera <no...@github.com>.
Cherry-picked to master as [8b11430a](http://git-wip-us.apache.org/repos/asf/jclouds-labs/commit/8b11430a). Thx!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/196#issuecomment-140558072
Re: [jclouds-labs] JCLOUDS-985: Add members API (#196)
Posted by Christopher Dancy <no...@github.com>.
> +
> +@AutoValue
> +public abstract class CreateMember {
> +
> + @Nullable
> + public abstract String name();
> +
> + public abstract List<String> peerURLs();
> +
> + public abstract List<String> clientURLs();
> +
> + CreateMember() {
> + }
> +
> + @SerializedNames({ "name", "peerURLs", "clientURLs" })
> + public static CreateMember create(final String name, final List<String> peerURLs, List<String> clientURLs) {
Aaahhhh yes you noted this before and I missed it amongst other changes being made. Jumping on this now
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/196/files#r37632011