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/07/27 19:35:17 UTC
[jclouds-labs] JCLOUDS-971: init for etcd provider. (#194)
ADDED: version and statistics API as well as mock tests for each. Both APIS are very simple as they are all GET requests and do not require any parameters. 5 methods total between the 2. This should be a good starting point for us to break apart the API into smaller chunks for others/reviewers to digest.
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds-labs/pull/194
-- Commit Summary --
* init for etcd provider.
-- File Changes --
A etcd/pom.xml (150)
A etcd/src/main/java/org/jclouds/etcd/EtcdApi.java (33)
A etcd/src/main/java/org/jclouds/etcd/EtcdApiMetadata.java (87)
A etcd/src/main/java/org/jclouds/etcd/config/EtcdHttpApiModule.java (46)
A etcd/src/main/java/org/jclouds/etcd/domain/statistics/Counts.java (38)
A etcd/src/main/java/org/jclouds/etcd/domain/statistics/Follower.java (38)
A etcd/src/main/java/org/jclouds/etcd/domain/statistics/Latency.java (44)
A etcd/src/main/java/org/jclouds/etcd/domain/statistics/Leader.java (40)
A etcd/src/main/java/org/jclouds/etcd/domain/statistics/LeaderInfo.java (40)
A etcd/src/main/java/org/jclouds/etcd/domain/statistics/Self.java (55)
A etcd/src/main/java/org/jclouds/etcd/domain/statistics/Store.java (68)
A etcd/src/main/java/org/jclouds/etcd/features/StatisticsApi.java (48)
A etcd/src/main/java/org/jclouds/etcd/features/VersionApi.java (33)
A etcd/src/main/java/org/jclouds/etcd/handlers/EtcdErrorHandler.java (71)
A etcd/src/test/java/org/jclouds/etcd/EtcdApiMetadataTest.java (50)
A etcd/src/test/java/org/jclouds/etcd/features/StatisticsApiMockTest.java (91)
A etcd/src/test/java/org/jclouds/etcd/features/VersionApiMockTest.java (48)
A etcd/src/test/java/org/jclouds/etcd/internal/BaseEtcdMockTest.java (102)
A etcd/src/test/resources/leader.json (31)
A etcd/src/test/resources/self.json (15)
A etcd/src/test/resources/store.json (16)
A etcd/src/test/resources/version.json (1)
-- Patch Links --
https://github.com/jclouds/jclouds-labs/pull/194.patch
https://github.com/jclouds/jclouds-labs/pull/194.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/194
Re: [jclouds-labs] JCLOUDS-971: init for etcd provider. (#194)
Posted by Christopher Dancy <no...@github.com>.
Hmmm ... not exactly sure why the whitespace changes are not getting propagated to github when I commit
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/194#issuecomment-125622198
Re: [jclouds-labs] JCLOUDS-971: init for etcd provider. (#194)
Posted by Christopher Dancy <no...@github.com>.
> + org.jclouds*;version="${jclouds.version}",
> + *
> + </jclouds.osgi.import>
> + </properties>
> +
> + <dependencies>
> + <dependency>
> + <groupId>org.apache.jclouds</groupId>
> + <artifactId>jclouds-core</artifactId>
> + <version>${jclouds.version}</version>
> + </dependency>
> + <dependency>
> + <groupId>org.apache.jclouds.driver</groupId>
> + <artifactId>jclouds-okhttp</artifactId>
> + <version>${jclouds.version}</version>
> + </dependency>
Required because the API makes heavy use of the http DELETE method and java 6 has known issues with this. The OkHttp driver solves this for java6.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/194/files#r35637778
Re: [jclouds-labs] JCLOUDS-971: init for etcd provider. (#194)
Posted by Christopher Dancy <no...@github.com>.
Aaahhhh ... issued a force push and I believe it closed this PR. @nacx created another PR which references this but is more direct and to the point: https://github.com/jclouds/jclouds-labs/pull/195
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/194#issuecomment-125732519
Re: [jclouds-labs] JCLOUDS-971: init for etcd provider. (#194)
Posted by Ignasi Barrera <no...@github.com>.
> + </parent>
> +
> + <!-- TODO: when out of labs, switch to org.jclouds.api -->
> + <artifactId>etcd</artifactId>
> + <name>jclouds etcd api</name>
> + <description>jclouds components to access an implementation of etcd</description>
> + <packaging>bundle</packaging>
> +
> + <properties>
> + <test.etcd.endpoint>http://127.0.0.1:2379</test.etcd.endpoint>
> + <test.etcd.identity>FIXME</test.etcd.identity>
> + <test.etcd.credential>FIXME</test.etcd.credential>
> + <test.etcd.api-version>v2</test.etcd.api-version>
> + <jclouds.osgi.export>org.jclouds.etcd*;version="${project.version}"</jclouds.osgi.export>
> + <jclouds.osgi.import>
> + org.jclouds.labs*;version="${project.version}",
Are there any specific classes in `org.jclouds.labs` that need to be imported?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/194/files#r35618679
Re: [jclouds-labs] JCLOUDS-971: init for etcd provider. (#194)
Posted by Christopher Dancy <no...@github.com>.
> + @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());
> + exception = new HttpResponseException(message, command, response);
> +
> + } catch (Exception e) {
> + exception = new HttpResponseException(command, response, e);
> + } finally {
> + if (exception == null) {
It's just a stub for now. Once we get going this class will flesh out quite a bit more.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/194/files#r35637892
Re: [jclouds-labs] JCLOUDS-971: init for etcd provider. (#194)
Posted by Christopher Dancy <no...@github.com>.
Failure not related to my changes. Code formatting issues are now solved.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/194#issuecomment-125702730
Re: [jclouds-labs] JCLOUDS-971: init for etcd provider. (#194)
Posted by Ignasi Barrera <no...@github.com>.
> +
> +package org.jclouds.etcd.config;
> +
> +import org.jclouds.etcd.EtcdApi;
> +import org.jclouds.etcd.handlers.EtcdErrorHandler;
> +import org.jclouds.http.HttpErrorHandler;
> +import org.jclouds.http.annotation.ClientError;
> +import org.jclouds.http.annotation.Redirection;
> +import org.jclouds.http.annotation.ServerError;
> +import org.jclouds.http.config.ConfiguresHttpCommandExecutorService;
> +import org.jclouds.http.okhttp.config.OkHttpCommandExecutorServiceModule;
> +import org.jclouds.rest.ConfiguresHttpApi;
> +import org.jclouds.rest.config.HttpApiModule;
> +
> +@ConfiguresHttpApi
> +@ConfiguresHttpCommandExecutorService
Remove this annotation once the OkHttp module is not used.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/194/files#r35618889
Re: [jclouds-labs] JCLOUDS-971: init for etcd provider. (#194)
Posted by Ignasi Barrera <no...@github.com>.
> +
> +@ConfiguresHttpApi
> +@ConfiguresHttpCommandExecutorService
> +public class EtcdHttpApiModule extends HttpApiModule<EtcdApi> {
> +
> + @Override
> + protected void bindErrorHandlers() {
> + bind(HttpErrorHandler.class).annotatedWith(Redirection.class).to(EtcdErrorHandler.class);
> + bind(HttpErrorHandler.class).annotatedWith(ClientError.class).to(EtcdErrorHandler.class);
> + bind(HttpErrorHandler.class).annotatedWith(ServerError.class).to(EtcdErrorHandler.class);
> + }
> +
> + protected void configure() {
> + super.configure();
> + install(new OkHttpCommandExecutorServiceModule());
> + }
Remove this. We don't want to force users to a concrete HTTP driver unless strictly required.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/194/files#r35618910
Re: [jclouds-labs] JCLOUDS-971: init for etcd provider. (#194)
Posted by Christopher Dancy <no...@github.com>.
Closed #194.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/194#event-367539973
Re: [jclouds-labs] JCLOUDS-971: init for etcd provider. (#194)
Posted by Ignasi Barrera <no...@github.com>.
Thanks @cdancy!
Apart from the comments, the code has to be formatted according to the jclouds style guide.
>Not sure how you want to go about breaking this into chunks.
I'd say submit one *complete* API at a time, being complete: API + mock tests + live tests.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/194#issuecomment-125478475
Re: [jclouds-labs] JCLOUDS-971: init for etcd provider. (#194)
Posted by Christopher Dancy <no...@github.com>.
> + </dependency>
> + <!-- Test dependencies -->
> + <dependency>
> + <groupId>org.apache.jclouds</groupId>
> + <artifactId>jclouds-core</artifactId>
> + <version>${jclouds.version}</version>
> + <type>test-jar</type>
> + <scope>test</scope>
> + </dependency>
> + <dependency>
> + <groupId>org.apache.jclouds</groupId>
> + <artifactId>jclouds-compute</artifactId>
> + <version>${jclouds.version}</version>
> + <type>test-jar</type>
> + <scope>test</scope>
> + </dependency>
It will need the compute api for testing once I've added the relevant testers. I can remove it but I'm going to need to add it back in once I've begun to add more tests.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/194/files#r35637974
Re: [jclouds-labs] JCLOUDS-971: init for etcd provider. (#194)
Posted by Ignasi Barrera <no...@github.com>.
> + </dependency>
> + <!-- Test dependencies -->
> + <dependency>
> + <groupId>org.apache.jclouds</groupId>
> + <artifactId>jclouds-core</artifactId>
> + <version>${jclouds.version}</version>
> + <type>test-jar</type>
> + <scope>test</scope>
> + </dependency>
> + <dependency>
> + <groupId>org.apache.jclouds</groupId>
> + <artifactId>jclouds-compute</artifactId>
> + <version>${jclouds.version}</version>
> + <type>test-jar</type>
> + <scope>test</scope>
> + </dependency>
This does not seem a compute API that will require the compute service abstraction or an SSH driver. Remove the dependencies (even if they are just in test scope).
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/194/files#r35618727
Re: [jclouds-labs] JCLOUDS-971: init for etcd provider. (#194)
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());
> + exception = new HttpResponseException(message, command, response);
> +
> + } catch (Exception e) {
This catch block seems not to be reachable?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/194/files#r35619129
Re: [jclouds-labs] JCLOUDS-971: init for etcd provider. (#194)
Posted by Ignasi Barrera <no...@github.com>.
> +
> +import com.google.auto.value.AutoValue;
> +
> +@AutoValue
> +public abstract class Leader {
> +
> + public abstract String leader();
> +
> + public abstract Map<String, Follower> followers();
> +
> + Leader() {
> + }
> +
> + @SerializedNames({"leader", "followers"})
> + public static Leader create(String leader, Map<String, Follower> followers) {
> + return new AutoValue_Leader(leader, followers);
Make the map immutable with `ImmutableMap.copyOf`.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/194/files#r35618968
Re: [jclouds-labs] JCLOUDS-971: init for etcd provider. (#194)
Posted by Christopher Dancy <no...@github.com>.
@nacx that sounds fair enough and will do.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/194#issuecomment-125570575
Re: [jclouds-labs] JCLOUDS-971: init for etcd provider. (#194)
Posted by Christopher Dancy <no...@github.com>.
Not sure how you want to go about breaking this into chunks but the API as a whole can be broken down into these features (ordered from smallest to biggest): VersionApi, StatisticsApi, MembersApi, and KeysApi. This PR covers the first 2 (Version and Statistics) as both are very small and not complicated. Once this gets in I will push integration tests and figured I'd move forward in much the same way with the other API's, that being: !.) submit initial API + mock tests 2.) when merged submit integration tests. The big piece here is the init which I've sent in so all other PR's will be much smaller. Hope it's not too big and let me know what you think of my plan. I can of course change if you want me to further granularize things.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/194#issuecomment-125285877
Re: [jclouds-labs] JCLOUDS-971: init for etcd provider. (#194)
Posted by Christopher Dancy <no...@github.com>.
@nacx I've removed the ErrorHandler as it's not used with this PR and will make things less confusing and easier to digest. I've addressed the style issues (forgot to use the jclouds eclipse formatter) and added live tests. Live tests are not doing anything special as they are just GET requests with no parameters. I believe that addresses all your comments. Let me know how things look. Thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/194#issuecomment-125607028
Re: [jclouds-labs] JCLOUDS-971: init for etcd provider. (#194)
Posted by Ignasi Barrera <no...@github.com>.
> + org.jclouds*;version="${jclouds.version}",
> + *
> + </jclouds.osgi.import>
> + </properties>
> +
> + <dependencies>
> + <dependency>
> + <groupId>org.apache.jclouds</groupId>
> + <artifactId>jclouds-core</artifactId>
> + <version>${jclouds.version}</version>
> + </dependency>
> + <dependency>
> + <groupId>org.apache.jclouds.driver</groupId>
> + <artifactId>jclouds-okhttp</artifactId>
> + <version>${jclouds.version}</version>
> + </dependency>
Is there any need to force a concrete HTTP driver? Remove this dependency if possible.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/194/files#r35618836
Re: [jclouds-labs] JCLOUDS-971: init for etcd provider. (#194)
Posted by Ignasi Barrera <no...@github.com>.
> + @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());
> + exception = new HttpResponseException(message, command, response);
> +
> + } catch (Exception e) {
> + exception = new HttpResponseException(command, response, e);
> + } finally {
> + if (exception == null) {
At this point the exception will never be null.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/194/files#r35619113
Re: [jclouds-labs] JCLOUDS-971: init for etcd provider. (#194)
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());
> + exception = new HttpResponseException(message, command, response);
> +
> + } catch (Exception e) {
That is true but it's just a place holder for now. I can remove it but the code will get put back in a couple commits from now so I thought to just leave this bare minimum for now.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/194/files#r35638012