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/28 21:40:46 UTC
[jclouds-labs] JCLOUDS-971: add statistics and version API (#195)
continuing from https://github.com/jclouds/jclouds-labs/pull/194#event-367539973
statistics and version API have both been fleshed out. Both are very small which is why they are being included in this PR. Mock and integration tests have been written and pass against latest etcd version: 2.1.1.
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds-labs/pull/195
-- Commit Summary --
* init for etcd provider.
* ADDED: etcd to list of projects
* REMOVED: removed EtcdErrorHandler as this PR does not depend on it.
* REFACTOR: force use of jclouds-eclipse formatting
* REALLY fixed code formatting this time around ;)
* refactor method name
* FIXED: fix version format to work against latest intended supported etcd version
-- File Changes --
A etcd/pom.xml (136)
A etcd/src/main/java/org/jclouds/etcd/EtcdApi.java (34)
A etcd/src/main/java/org/jclouds/etcd/EtcdApiMetadata.java (82)
A etcd/src/main/java/org/jclouds/etcd/config/EtcdHttpApiModule.java (40)
A etcd/src/main/java/org/jclouds/etcd/domain/statistics/Counts.java (39)
A etcd/src/main/java/org/jclouds/etcd/domain/statistics/Follower.java (39)
A etcd/src/main/java/org/jclouds/etcd/domain/statistics/Latency.java (46)
A etcd/src/main/java/org/jclouds/etcd/domain/statistics/Leader.java (42)
A etcd/src/main/java/org/jclouds/etcd/domain/statistics/LeaderInfo.java (41)
A etcd/src/main/java/org/jclouds/etcd/domain/statistics/Self.java (56)
A etcd/src/main/java/org/jclouds/etcd/domain/statistics/Store.java (69)
A etcd/src/main/java/org/jclouds/etcd/domain/version/Version.java (39)
A etcd/src/main/java/org/jclouds/etcd/features/StatisticsApi.java (49)
A etcd/src/main/java/org/jclouds/etcd/features/VersionApi.java (36)
A etcd/src/test/java/org/jclouds/etcd/BaseEtcdApiLiveTest.java (46)
A etcd/src/test/java/org/jclouds/etcd/EtcdApiMetadataTest.java (51)
A etcd/src/test/java/org/jclouds/etcd/features/StatisticsApiLiveTest.java (44)
A etcd/src/test/java/org/jclouds/etcd/features/StatisticsApiMockTest.java (92)
A etcd/src/test/java/org/jclouds/etcd/features/VersionApiLiveTest.java (41)
A etcd/src/test/java/org/jclouds/etcd/features/VersionApiMockTest.java (56)
A etcd/src/test/java/org/jclouds/etcd/internal/BaseEtcdMockTest.java (103)
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 (4)
M pom.xml (1)
-- Patch Links --
https://github.com/jclouds/jclouds-labs/pull/195.patch
https://github.com/jclouds/jclouds-labs/pull/195.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/195
Re: [jclouds-labs] JCLOUDS-971: add statistics and miscellaneous API
(#195)
Posted by Christopher Dancy <no...@github.com>.
> + }
> +
> + public static Properties defaultProperties() {
> + Properties properties = BaseHttpApiMetadata.defaultProperties();
> + return properties;
> + }
> +
> + public static class Builder extends BaseHttpApiMetadata.Builder<EtcdApi, Builder> {
> +
> + protected Builder() {
> + super(EtcdApi.class);
> + id("etcd").
> + name("Etcd API").
> + identityName("username:password").
> + defaultIdentity("Optional Username").
> + defaultCredential("Optional Password").
If I do that, and use the default* methods, then tests fail with an NPE on 'identityName'.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/195/files#r36198643
Re: [jclouds-labs] JCLOUDS-971: add statistics and miscellaneous API
(#195)
Posted by Ignasi Barrera <no...@github.com>.
Closed #195.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/195#event-374267024
Re: [jclouds-labs] JCLOUDS-971: add statistics and miscellaneous API
(#195)
Posted by Christopher Dancy <no...@github.com>.
> + }
> +
> + public static Properties defaultProperties() {
> + Properties properties = BaseHttpApiMetadata.defaultProperties();
> + return properties;
> + }
> +
> + public static class Builder extends BaseHttpApiMetadata.Builder<EtcdApi, Builder> {
> +
> + protected Builder() {
> + super(EtcdApi.class);
> + id("etcd").
> + name("Etcd API").
> + identityName("username:password").
> + defaultIdentity("Optional Username").
> + defaultCredential("Optional Password").
Perhaps I'm confused here. For optional username/password should I then ONLY set 'defaultIdentity' and 'defaultCredential' or do something else?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/195/files#r36186698
Re: [jclouds-labs] JCLOUDS-971: add statistics and miscellaneous API
(#195)
Posted by Ignasi Barrera <no...@github.com>.
> + }
> +
> + public static Properties defaultProperties() {
> + Properties properties = BaseHttpApiMetadata.defaultProperties();
> + return properties;
> + }
> +
> + public static class Builder extends BaseHttpApiMetadata.Builder<EtcdApi, Builder> {
> +
> + protected Builder() {
> + super(EtcdApi.class);
> + id("etcd").
> + name("Etcd API").
> + identityName("username:password").
> + defaultIdentity("Optional Username").
> + defaultCredential("Optional Password").
I'm confused. Why not have the identity for the username and the credential for the password?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/195/files#r36185076
Re: [jclouds-labs] JCLOUDS-971: add statistics and miscellaneous API
(#195)
Posted by Ignasi Barrera <no...@github.com>.
> + }
> +
> + public static Properties defaultProperties() {
> + Properties properties = BaseHttpApiMetadata.defaultProperties();
> + return properties;
> + }
> +
> + public static class Builder extends BaseHttpApiMetadata.Builder<EtcdApi, Builder> {
> +
> + protected Builder() {
> + super(EtcdApi.class);
> + id("etcd").
> + name("Etcd API").
> + identityName("username:password").
> + defaultIdentity("Optional Username").
> + defaultCredential("Optional Password").
That's OK. One last thing is to think if this API is intended to be used without authentication, or authentication will be mandatory.
If the common case is to use it without auth, then the api metadata should provide the default values for the identity and credential (even dummy ones) so users can create a context without having to explicitly pass dummy values. Otherwise, this PR is good to go.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/195/files#r36194568
Re: [jclouds-labs] JCLOUDS-971: add statistics and miscellaneous API
(#195)
Posted by Ignasi Barrera <no...@github.com>.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +package org.jclouds.etcd.domain.miscellaneous;
> +
> +import org.jclouds.json.SerializedNames;
> +
> +import com.google.auto.value.AutoValue;
> +
> +@AutoValue
> +public abstract class Version {
> +
> + public abstract String etcdserver();
> +
> + public abstract String etcdcluster();
Use camel case for these methods.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/195/files#r36170524
Re: [jclouds-labs] JCLOUDS-971: add statistics and miscellaneous API
(#195)
Posted by Ignasi Barrera <no...@github.com>.
> + }
> +
> + public static Properties defaultProperties() {
> + Properties properties = BaseHttpApiMetadata.defaultProperties();
> + return properties;
> + }
> +
> + public static class Builder extends BaseHttpApiMetadata.Builder<EtcdApi, Builder> {
> +
> + protected Builder() {
> + super(EtcdApi.class);
> + id("etcd").
> + name("Etcd API").
> + identityName("username:password").
> + defaultIdentity("Optional Username").
> + defaultCredential("Optional Password").
I'm not saying you have to use ones or the others. All four methods should be present:
* `identityName` and `credentialName` are mandatory for all ApiMetadata classes. They are just labels that define the format of the identity and credential so users know what to put there when creating the context. They're just labels to give a hint and allow, for example, tooling to prompt for the right thing.
* `defaultIdentity` and `defaultCredential` are default values to set for the identity and credential fields, if that makes sense, in case the user does not supply one of them. These are optional as in most apis/providers users are required both the identity and the credential.
In this case, as the credentials are not used, you have to set all four methods in your api metadata, in order to configure a default dummy credentials so the user does not have to provide them (which would be useless and confusing) when creating the context.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/195/files#r36246427
Re: [jclouds-labs] JCLOUDS-971: add statistics and miscellaneous API
(#195)
Posted by Christopher Dancy <no...@github.com>.
Failures due to jdbc provider not etcd.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/195#issuecomment-125918791
Re: [jclouds-labs] JCLOUDS-971: add statistics and miscellaneous API
(#195)
Posted by Ignasi Barrera <no...@github.com>.
> + }
> +
> + public static Properties defaultProperties() {
> + Properties properties = BaseHttpApiMetadata.defaultProperties();
> + return properties;
> + }
> +
> + public static class Builder extends BaseHttpApiMetadata.Builder<EtcdApi, Builder> {
> +
> + protected Builder() {
> + super(EtcdApi.class);
> + id("etcd").
> + name("Etcd API").
> + identityName("username:password").
> + defaultIdentity("Optional Username").
> + defaultCredential("Optional Password").
In that case you have to restore the `defaultIdentity` and `defaultCredential` methods (now we have only the ones that "describe" the authentication) so users don't have to explicitly pass dummy credentials when creating the context. You should then be able to remove the default credentials from the tests too.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/195/files#r36197766
Re: [jclouds-labs] JCLOUDS-971: add statistics and miscellaneous API
(#195)
Posted by Christopher Dancy <no...@github.com>.
> + }
> +
> + public static Properties defaultProperties() {
> + Properties properties = BaseHttpApiMetadata.defaultProperties();
> + return properties;
> + }
> +
> + public static class Builder extends BaseHttpApiMetadata.Builder<EtcdApi, Builder> {
> +
> + protected Builder() {
> + super(EtcdApi.class);
> + id("etcd").
> + name("Etcd API").
> + identityName("username:password").
> + defaultIdentity("Optional Username").
> + defaultCredential("Optional Password").
@nacx Ok. So to address I've set 'identityName' and 'credentialName' to optional username/password respectively. To make jclouds happy, and get tests to run, I've set PROPERTY_IDENTITY and PROPERTY_CREDENTIAL to the same aforementioned values within the defaultProperties method call.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/195/files#r36189488
Re: [jclouds-labs] JCLOUDS-971: add statistics and miscellaneous API
(#195)
Posted by Christopher Dancy <no...@github.com>.
@nacx done!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/195#issuecomment-127961800
Re: [jclouds-labs] JCLOUDS-971: add statistics and miscellaneous API
(#195)
Posted by Christopher Dancy <no...@github.com>.
> + }
> +
> + protected EtcdApiMetadata(Builder builder) {
> + super(builder);
> + }
> +
> + public static Properties defaultProperties() {
> + Properties properties = BaseHttpApiMetadata.defaultProperties();
> + return properties;
> + }
> +
> + public static class Builder extends BaseHttpApiMetadata.Builder<EtcdApi, Builder> {
> +
> + protected Builder() {
> + super(EtcdApi.class);
> + id("etcd").name("Etcd API").identityName("Optional Username").defaultIdentity("Optional Username and Password")
There is an auth API that was just introduced that I will tackle in a later PR. It's documentaiton can be found here:
https://github.com/coreos/etcd/blob/master/Documentation/auth_api.md
But yes by default I put these in if only to keep jclouds from complaining. Is there a better to way go about doing this?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/195/files#r36181709
Re: [jclouds-labs] JCLOUDS-971: add statistics and miscellaneous API
(#195)
Posted by Ignasi Barrera <no...@github.com>.
Pushed to master as [9cd3cfa5](http://git-wip-us.apache.org/repos/asf/jclouds-labs/commit/9cd3cfa5). Thanks @cdancy!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/195#issuecomment-128020693
Re: [jclouds-labs] JCLOUDS-971: add statistics and miscellaneous API
(#195)
Posted by Ignasi Barrera <no...@github.com>.
Looks pretty good! Just a few comments:
* Only add dependencies when needed: remove the OkHttp driver and the jclouds-compute one. Regarding the JRE and DELETE requests, IIRC they are supported if they don't have a body. Having a quick look at the etcd api docs I don't see any DELETE request with a body (I can be wrong), so the default http driver should work fine and we can let users pick their preferred driver.
* Live tests right now do not seem to have to be single-threaded. Remove that from the annotation and also the `threadCount` in the pom.xml.
* Seeing the nature of the MiscApi methods, consider removing the class and moving its methods to the EtcdApi class. Also move common method annotations to class level.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/195#issuecomment-126190860
Re: [jclouds-labs] JCLOUDS-971: add statistics and miscellaneous API
(#195)
Posted by Christopher Dancy <no...@github.com>.
Fixed last minute typo
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/195#issuecomment-127972940
Re: [jclouds-labs] JCLOUDS-971: add statistics and miscellaneous API
(#195)
Posted by Ignasi Barrera <no...@github.com>.
> + }
> +
> + public static Properties defaultProperties() {
> + Properties properties = BaseHttpApiMetadata.defaultProperties();
> + return properties;
> + }
> +
> + public static class Builder extends BaseHttpApiMetadata.Builder<EtcdApi, Builder> {
> +
> + protected Builder() {
> + super(EtcdApi.class);
> + id("etcd").
> + name("Etcd API").
> + identityName("username:password").
> + defaultIdentity("Optional Username").
> + defaultCredential("Optional Password").
The defaults are ok. The `identityName` property describes the identity (a username, an email, a token), and the `credentialName` describes the credential (a password, a private key, etc), so users know what to put there.
Once there is auth in this provider, the description of the identity would me misleading as it would encourage users to use the form "username:password" in the identity field. What about the credential field?
The defaults values are OK; it's just a matter about properly describing how the authentication is expected to be provided.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/195/files#r36187855
Re: [jclouds-labs] JCLOUDS-971: add statistics and miscellaneous API
(#195)
Posted by Ignasi Barrera <no...@github.com>.
> + }
> +
> + protected EtcdApiMetadata(Builder builder) {
> + super(builder);
> + }
> +
> + public static Properties defaultProperties() {
> + Properties properties = BaseHttpApiMetadata.defaultProperties();
> + return properties;
> + }
> +
> + public static class Builder extends BaseHttpApiMetadata.Builder<EtcdApi, Builder> {
> +
> + protected Builder() {
> + super(EtcdApi.class);
> + id("etcd").name("Etcd API").identityName("Optional Username").defaultIdentity("Optional Username and Password")
It does not seem to be authentication in this API. Should the api metadata define the `defaultCredential` too then? Also add the `credentialName`.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/195/files#r36170802
Re: [jclouds-labs] JCLOUDS-971: add statistics and miscellaneous API
(#195)
Posted by Christopher Dancy <no...@github.com>.
@nacx on your comments:
1.) All comments should be addressed. And you're right about the DELETE method, no request I could find uses a body when issuing a delete. All tests proved successful so it looks like we are good on that end.
2.) Comments have been addressed.
3.) I've added one more request to the MiscellaneousApi: metrics. Documentation on it can be found here: https://github.com/coreos/etcd/blob/master/Documentation/metrics.md. It's experimental at this point, so far as features/structure, but works as expected and returns the expected output. It's the last endpoint I want to get in before closing this PR and I think is a good reason to keep this as a separate class rather than merging with EtcdApi class. I'm not sure if the name 'Miscellaneous' fits but it's what I came up with to put all requests coming from the root URL in one place. Moving these into the EtcdApi seems it may clutter things IMO and I know there are commits for the next version to add at least 1 more endpoint to the root URL. If we can I'd like to leave them separated if only to make things easier to digest. The name is not great but I'm open to suggestions.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/195#issuecomment-126340849
Re: [jclouds-labs] JCLOUDS-971: add statistics and miscellaneous API
(#195)
Posted by Christopher Dancy <no...@github.com>.
> + }
> +
> + public static Properties defaultProperties() {
> + Properties properties = BaseHttpApiMetadata.defaultProperties();
> + return properties;
> + }
> +
> + public static class Builder extends BaseHttpApiMetadata.Builder<EtcdApi, Builder> {
> +
> + protected Builder() {
> + super(EtcdApi.class);
> + id("etcd").
> + name("Etcd API").
> + identityName("username:password").
> + defaultIdentity("Optional Username").
> + defaultCredential("Optional Password").
@nacx it's intended to be used without as OOTB etcd does not default to requiring credentials. Authentiction, as an option, was only just added in the recent stable release and is what I'm targeting for this API.
So with that said ... I think we're good to go? I've nothing else to sneak in from my end WRT to this PR.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/195/files#r36197006
Re: [jclouds-labs] JCLOUDS-971: add statistics and miscellaneous API
(#195)
Posted by Ignasi Barrera <no...@github.com>.
It looks great now. Thanks @cdancy!
Mind squashing the commits into a single one?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/195#issuecomment-127931809
Re: [jclouds-labs] JCLOUDS-971: add statistics and miscellaneous API
(#195)
Posted by Christopher Dancy <no...@github.com>.
@nacx identityName and credentialName are now set to "Optional ..." with defaultIdentity and defaultCredential set to 'N/A'. Will this suffice?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/195#issuecomment-127806414