HLRC: Add support for XPack Post Start Basic Licence API by vladimirdolzhenko · Pull Request #33606 · elastic/elasticsearch

vladimirdolzhenko · 2018-09-11T15:29:26Z

HLRC: Add support for XPack Post Start Basic Licence API

Relates to #29827

Relates to elastic#29827

elasticmachine · 2018-09-11T15:29:27Z

Pinging @elastic/es-core-infra

imotov

LGTM, assuming that you will fix the formatting issue and the test pass. Maybe @hub-cap should give it a look as well.

vladimirdolzhenko · 2018-09-11T17:18:07Z

@elasticmachine test this please

vladimirdolzhenko · 2018-09-13T10:41:24Z

@hub-cap could you please have a look as well ?

hub-cap

Lets drop the word post from all this, as we want to keep it to just the name of the action. Of course, in the request converters we will know its a POST, but the HL client should be agnostic of this. Also please move the stuff you put back in protocol back. Sorry about any confusion, the rest client has been in lots of flux.

x-pack/plugin/core/src/main/java/org/elasticsearch/license/RestPostStartBasicLicense.java

...ugin/core/src/main/java/org/elasticsearch/protocol/xpack/license/PostStartBasicResponse.java

…tart_basic

hub-cap · 2018-09-19T16:13:27Z

as mentioned in #33406 (review), we need to add a LicensingIT test class for testing randomized values in these calls, and negative tests. Please add that. I wanted to just drop a comment before I started reviewing so you know of this quicker.

hub-cap

Great work so far. Some stuff to address below, nothing huge.

hub-cap · 2018-09-19T16:16:22Z

...nt/rest-high-level/src/main/java/org/elasticsearch/client/license/PostStartBasicRequest.java

+import org.elasticsearch.action.support.master.AcknowledgedRequest;
+
+public class PostStartBasicRequest extends AcknowledgedRequest<PostStartBasicRequest> {
+    private boolean acknowledge = false;


pls make this boolean final and remove the setter.

hub-cap · 2018-09-19T16:21:13Z

...t/rest-high-level/src/main/java/org/elasticsearch/client/license/PostStartBasicResponse.java

+import static org.elasticsearch.common.xcontent.ConstructingObjectParser.optionalConstructorArg;
+import static org.elasticsearch.common.xcontent.XContentParserUtils.ensureExpectedToken;
+
+public class PostStartBasicResponse extends AcknowledgedResponse {


This class looks like it has a lot of extra stuff that is not needed. In general, we do not need from/to transport serialization. Do we need addCustomFields, as its also toXContent related.

hub-cap · 2018-09-19T16:21:37Z

...t/rest-high-level/src/main/java/org/elasticsearch/client/license/PostStartBasicResponse.java

+        return new PostStartBasicResponse(status, acknowledgements.v2(), acknowledgements.v1());
+    });
+
+    private static final ParseField BASIC_WAS_STARTED = new ParseField("basic_was_started");


see https://github.com/elastic/elasticsearch/pull/33406/files#r218858319

hub-cap · 2018-09-19T16:26:50Z

...t/rest-high-level/src/main/java/org/elasticsearch/client/license/PostStartBasicResponse.java

+    private Map<String, String[]> acknowledgeMessages;
+    private String acknowledgeMessage;
+
+    public enum Status {


is this a more common class that can be used elsewhere? if so, lets break it out to its own class, and put it in the same package.

hub-cap · 2018-09-19T16:28:21Z

...t-high-level/src/test/java/org/elasticsearch/client/license/PostStartBasicResponseTests.java

+import java.util.Map;
+import java.util.function.Predicate;
+
+public class PostStartBasicResponseTests extends AbstractStreamableXContentTestCase<PostStartBasicResponse> {


These will have to change as well, once you remove the ToXContent portion of the response. You will have to manually construct the parser and then pass that in to the fromXContent of the response.

hub-cap · 2018-09-19T16:31:59Z

docs/java-rest/high-level/licensing/post-start-basic.asciidoc

@@ -0,0 +1,55 @@
+[[java-rest-high-post-start-basic]]


You may want to lift some of the wording from the very similar PR ive linked here, https://github.com/elastic/elasticsearch/blob/07554dda3b4bb2e2871d913f00e3cead0cedeeb4/docs/java-rest/high-level/licensing/start-trial.asciidoc .

hub-cap · 2018-09-19T16:50:47Z

Pls also add a LicenseRequestConvertersTests to test your request converter.

…tart_basic

vladimirdolzhenko · 2018-09-27T11:12:08Z

@elasticmachine test this please

…tart_basic

hub-cap

LGTM, one nit then you can merge

hub-cap · 2018-10-02T14:31:13Z

client/rest-high-level/src/main/java/org/elasticsearch/client/license/StartBasicRequest.java

+    }
+
+    @Override
+    public ActionRequestValidationException validate() {


You can leave this out if you are not using it

with ccbc485 StartBasicRequest extends TimedRequest, added couple test routines for that

…tart_basic

…t trial license

vladimirdolzhenko · 2018-10-04T17:50:15Z

there is a dependency for tests on #33406 - I'd rather wait for start trial license to be merged

hub-cap · 2018-10-15T16:41:20Z

if you want to merge, you can always just stub in some Low level rest calls and then @andyb-elastic can clean them up in his PR ;)

vladimirdolzhenko · 2018-10-16T11:32:28Z

@hub-cap thanks for the tip: in fact start trial license did not help as it had been already started.

there are several ways (at least 2) on how to solve the problem:

put a hard coded trial license (as it is not possible to bring license-tools to hlrc due to jar hell) - it could works for snapshot build only
split license specific tests into a separated gradle task
any other

for now I rely on a hard coded trial license approach

hub-cap · 2018-10-16T13:03:29Z

Ahh right it was this issue, and I discussed with @andyb-elastic and he is going to make a new PR with the changes to make a new cluster with the different license test requirements. So it might make sense to merge this and backport it and await his change, so you can also make your change. Your call.

vladimirdolzhenko · 2018-10-16T13:23:23Z

thanks @hub-cap for comments 👍

Relates to #29827 (cherry picked from commit 230ad53)

andyb-elastic · 2018-10-16T17:39:26Z

Right, looks like you got the case where it starts a basic license working by manually setting a trial license. I'm working on a PR to run the license IT in a separate gradle project (we ran into some issues getting it to work with multiple clusters in the same project)

Relates to #29827

HLRC: Add support for XPack Post Start Basic Licence API

71d6de1

Relates to elastic#29827

vladimirdolzhenko added >enhancement :Core/Java High Level REST Client labels Sep 11, 2018

vladimirdolzhenko requested a review from imotov September 11, 2018 15:29

vladimirdolzhenko force-pushed the hlrc_post_start_basic branch from 3ac4964 to 71d6de1 Compare September 11, 2018 15:42

imotov approved these changes Sep 11, 2018

View reviewed changes

hub-cap suggested changes Sep 17, 2018

View reviewed changes

x-pack/plugin/core/src/main/java/org/elasticsearch/license/RestPostStartBasicLicense.java Outdated Show resolved Hide resolved

...ugin/core/src/main/java/org/elasticsearch/protocol/xpack/license/PostStartBasicResponse.java Outdated Show resolved Hide resolved

Vladimir Dolzhenko added 3 commits September 17, 2018 23:17

drop post verb

5f00b7f

Merge remote-tracking branch 'remotes/origin/master' into hlrc_post_s…

625bd66

…tart_basic

don't touch xpack / server

8dfb03d

vladimirdolzhenko requested a review from hub-cap September 18, 2018 10:51

hub-cap suggested changes Sep 19, 2018

View reviewed changes

Vladimir Dolzhenko added 4 commits September 19, 2018 21:31

drop post verb; drop unused functionality; polished doc

9430892

added LicenseRequestConvertersTests

7fde9be

added LicensingIT

c4ab5de

Merge remote-tracking branch 'remotes/origin/master' into hlrc_post_s…

2320bd8

…tart_basic

Merge remote-tracking branch 'remotes/origin/master' into hlrc_post_s…

4015b25

…tart_basic

hub-cap approved these changes Oct 2, 2018

View reviewed changes

Vladimir Dolzhenko added 6 commits October 3, 2018 11:23

Merge remote-tracking branch 'remotes/origin/master' into hlrc_post_s…

31ec2b8

…tart_basic

TimedRequest rocks!

ccbc485

Merge remote-tracking branch 'remotes/origin/master' into hlrc_post_s…

9519f21

…tart_basic

Merge remote-tracking branch 'remotes/origin/master' into hlrc_post_s…

9c64a65

…tart_basic

added default value for timeout

8539641

muted tests those change licence to basic - has to wait for HLRC star…

34ed1f3

…t trial license

elasticmachine mentioned this pull request Oct 4, 2018

Java high-level REST client completeness for xpack APIs #29827

Closed

98 tasks

vladimirdolzhenko requested a review from hub-cap October 4, 2018 17:49

add roll-back license

a6adade

vladimirdolzhenko mentioned this pull request Oct 16, 2018

HLRC XPack Protocol clean up: Licence, Misc #34469

Merged

vladimirdolzhenko merged commit 230ad53 into elastic:master Oct 16, 2018

vladimirdolzhenko added a commit that referenced this pull request Oct 16, 2018

HLRC: Add support for XPack Post Start Basic Licence API (#33606)

714aed6

Relates to #29827 (cherry picked from commit 230ad53)

vladimirdolzhenko added v7.0.0 v6.5.0 labels Oct 16, 2018

vladimirdolzhenko deleted the hlrc_post_start_basic branch October 16, 2018 14:06

kcm pushed a commit that referenced this pull request Oct 30, 2018

HLRC: Add support for XPack Post Start Basic Licence API (#33606)

a62a646

Relates to #29827

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Conversation

vladimirdolzhenko commented Sep 11, 2018

Uh oh!

elasticmachine commented Sep 11, 2018

Uh oh!

imotov left a comment

Choose a reason for hiding this comment

Uh oh!

vladimirdolzhenko commented Sep 11, 2018

Uh oh!

vladimirdolzhenko commented Sep 13, 2018

Uh oh!

hub-cap left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hub-cap commented Sep 19, 2018

Uh oh!

hub-cap left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hub-cap commented Sep 19, 2018

Uh oh!

vladimirdolzhenko commented Sep 27, 2018

Uh oh!

hub-cap left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vladimirdolzhenko commented Oct 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hub-cap commented Oct 15, 2018

Uh oh!

vladimirdolzhenko commented Oct 16, 2018

Uh oh!

hub-cap commented Oct 16, 2018

Uh oh!

vladimirdolzhenko commented Oct 16, 2018

Uh oh!

andyb-elastic commented Oct 16, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

vladimirdolzhenko commented Oct 4, 2018 •

edited

Loading