HLRC: Add ILM Status to HLRC #33283

jakelandis · 2018-08-30T19:09:53Z

Adds support for the Index Lifecycle Management Status to the Java
High-Level Rest Client.

Relates to #33100

Notes to reviewer(s):

I don't think there should be anything controversial here, and probably don't need 5 reviewers ... just wasn't sure who to tag.
the naming conventions were chosen to match the start/stopILM.
Using a TimedRequest instead of a StatusILMRequest... not sure the preference here, but it kinda matches the AcknowledgedResponse in terms of not creating typed request/response when not needed.

Adds support for the Index Lifecycle Management Status to the Java High-Level Rest Client. Relates to elastic#33100

elasticmachine · 2018-08-30T19:09:55Z

Pinging @elastic/es-core-infra

dakrone

I left comments renaming these methods/classes, I'm in favor of:

LifecycleManagementStatusResponse
lifecycleManagementStatus(...)

dakrone · 2018-08-30T19:13:43Z

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

+     * @param request the request with user defined timeouts.
+     * @param options the request options (e.g. headers), use {@link RequestOptions#DEFAULT} if nothing needs to be customized
+     */
+    public StatusILMResponse StatusILM(TimedRequest request, RequestOptions options) throws IOException {


I think these should be LifecycleManagementStatusResponse, rather than using the ILM in method/class names

Also, the method name is StatusILM and we don't capitalize any of our method names

Also, the method name is StatusILM and we don't capitalize any of our method names

oops didn't meant to capitalize the method name. (kinda surprised that check style didn't catch that). will fix.

I think these should be LifecycleManagementStatusResponse, rather than using the ILM in method/class names

I tend to agree, but was matching the start/stopILM here: https://github.com/elastic/elasticsearch/tree/index-lifecycle/x-pack/protocol/src/main/java/org/elasticsearch/protocol/xpack/indexlifecycle

Should I rename those too ?

dakrone · 2018-08-30T19:14:00Z

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

+     * @param options  the request options (e.g. headers), use {@link RequestOptions#DEFAULT} if nothing needs to be customized
+     * @param listener the listener to be notified upon request completion
+     */
+    public void StatusILMAsync(TimedRequest request, RequestOptions options,


Same here about naming and capitalization

oops ... IDE find and replace gone wild. will fix.

dakrone · 2018-08-30T19:15:29Z

...rest-high-level/src/main/java/org/elasticsearch/client/indexlifecycle/StatusILMResponse.java

+        }
+    }
+
+    // generated


I don't think we need these generated comments

I can remove them. FWIW, I generally put these in here as a way to signal nothing special here, so no tests, and if changes are made need to re-generate.

gwbrown

Left some comments.

gwbrown · 2018-08-30T19:13:04Z

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

+     * @param request the request with user defined timeouts.
+     * @param options the request options (e.g. headers), use {@link RequestOptions#DEFAULT} if nothing needs to be customized
+     */
+    public StatusILMResponse StatusILM(TimedRequest request, RequestOptions options) throws IOException {


~~Method names should start with a lowercase letter, so this should be statusILM.~~

~~Or maybe getILMStatus? statusILM doesn't make much sense seeing it for the first time - there's no verb there.~~

Lee beat me to it and left better feedback, see his comment instead.

gwbrown · 2018-08-30T19:13:40Z

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

+     * @param options  the request options (e.g. headers), use {@link RequestOptions#DEFAULT} if nothing needs to be customized
+     * @param listener the listener to be notified upon request completion
+     */
+    public void StatusILMAsync(TimedRequest request, RequestOptions options,


As above re: method names.

gwbrown · 2018-08-30T19:18:04Z

...rest-high-level/src/main/java/org/elasticsearch/client/indexlifecycle/StatusILMResponse.java

+    /**
+     * Enum representing the different modes that Index Lifecycle Service can operate in.
+     */
+    public enum OperationMode {


#33250 moves the existing OperationMode class into o.e.client.indexlifecycle - you may want to wait until that gets merged (should be later today) and use that one instead of having this as an inner class.

thanks! ... will wait till that gets merged and update accordingly.

gwbrown · 2018-08-30T19:22:18Z

client/rest-high-level/src/test/java/org/elasticsearch/client/IndexLifecycleIT.java

-        Response statusResponse = client().performRequest(statusReq);
-        String statusResponseString = EntityUtils.toString(statusResponse.getEntity());
-        assertEquals("{\"operation_mode\":\"RUNNING\"}", statusResponseString);
+        TimedRequest statusRequest = new TimedRequest();


While I get the logic behind why you did it this way, I think generic classes make more sense for responses than requests. As a user of the HLRC, this feels really strange - even if GetILMStatusRequest would be an extra class that doesn't really add anything, I think having separate classes for each request type is the way to go. I could be wrong though, I'd like to hear others' input on this.

I don't have a strong opinion beyond consistency in the API.

Perhaps we should allow a no-arg method in the client that creates the request for them. However, that also creates inconsistency (unless you create a bunch of new methods).

I'd like to hear others' input on this.

..same

@hub-cap Could you weigh in on this?

gwbrown · 2018-08-30T19:28:25Z

...high-level/src/test/java/org/elasticsearch/client/indexlifecycle/StatusILMResponseTests.java

+
+public class StatusILMResponseTests extends ESTestCase {
+
+    public void testClientServerStatuses() {


Using the shared class would let you get rid of this test, too.

will wait for #33250 to use shared class.

* Fixed incorrect method uppercase * Renamed statsILM -> lifecycleManagementStatus * removed "generated" comments

jakelandis · 2018-08-30T21:11:03Z

Changes pushed:

Fixed incorrect method uppercase
Renamed statsILM -> lifecycleManagementStatus
removed "generated" comments

TODO (this PR):

~~Integrate with HLRC: Add Put Lifecycle Policy API to HLRC #33250 once merged~~ (done)
~~Decision on named Request classes vs. generic TimedRequest class for empty requests [1]~~

TODO (not in this PR):

rename stopILM and startILM ? [2]
~~enforce basic method naming via checkstyle~~ (edit: this is a rabbit hole not worth going down)

@hub-cap [1]
Is there a preferred design for how to model empty requests (no parameters) for the API ?
a) Use a generic TimedRequest (like this PR currently)
b) create an empty typed Request (e.g. public class MyThingRequest extends TimedRequest{ //empty } ) (as @gwbrown advocates)
c) introduce no arg methods in the *Client's that under the covers uses a TimedRequest (e.g. MyThingResponse getMyThing(){ //doit })
d) other ?

@colings86 [2]
Any objections if I move the start/stopILM HLRC API's to the newer format and change the name to match the @dakrone's naming suggestion ? (different PR from this one)

gwbrown · 2018-08-30T22:09:17Z

@jakelandis #33250 has been merged into index-lifecycle and backported to index-lifecycle-6.x, so you should be good to go for switching over to the existing OperationMode class.

colings86 · 2018-08-31T08:22:30Z

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

@@ -1241,6 +1241,18 @@ static Request stopILM(StopILMRequest stopILMRequest) {
        return request;
    }

+    static Request statusILM(TimedRequest ilmStatusRequest){


Can we make this lifecycleManagementStatus to match the method in the client?

thanks missed this one. done.

gwbrown · 2018-09-04T15:00:24Z

LGTM, pending @hub-cap's input on specialized vs generic requests.

hub-cap · 2018-09-04T15:46:58Z

If we create a request and we ever want to add things to it, then it will not require a breaking contract change. It also gives us a chance to rename TimedRequest if its scope changes one day. It might even be worth making it abstract to ensure we dont use it this way. what do yall think?

jakelandis · 2018-09-04T15:49:46Z

@hub-cap - good point. I will add the specific the request to this PR and change TimedRequest to abstract.

…equest

jakelandis · 2018-09-04T21:00:35Z

@dakrone @gwbrown - mind taking another look all suggestions are implemented.

dakrone

I left 3 comments but they are very minor, otherwise LGTM

dakrone · 2018-09-04T21:26:23Z

...src/main/java/org/elasticsearch/client/indexlifecycle/LifecycleManagementStatusResponse.java

+        "operation_mode", a -> new LifecycleManagementStatusResponse((String) a[0]));
+
+    static {
+        PARSER.declareString(ConstructingObjectParser.constructorArg(), new ParseField("operation_mode"));


Can you put the ParseField into a class private var and then use it in the parser (so we don't accidentally typo/change it in the future)

moved "operation_mode" to a static final string (not exactly the ask, but i believe same outcome)

dakrone · 2018-09-04T21:27:28Z

...src/main/java/org/elasticsearch/client/indexlifecycle/LifecycleManagementStatusResponse.java

+    }
+
+    //package private for testing
+    LifecycleManagementStatusResponse(String operationMode) {


I think this should take an OperationMode to push the conversion into the caller, rather than doing it in the constructor (it seems cleaner to me that way).

This is just my personal preference though, so up to you if you want to change it.

leaving as-is

dakrone · 2018-09-04T21:28:44Z

client/rest-high-level/src/main/java/org/elasticsearch/client/indexlifecycle/OperationMode.java

+    static OperationMode fromString(String string) {
+        return EnumSet.allOf(OperationMode.class).stream()
+            .filter(e -> string.equalsIgnoreCase(e.name())).findFirst()
+            .orElseThrow(() -> new IllegalArgumentException(String.format(Locale.ENGLISH, "%s is not a valid operation_mode", string)));


We use Locale.ROOT for as many things as we can, rather than the English locale here

jakelandis · 2018-09-05T18:10:36Z

Thanks for the reviews! will merge once CI turns green.

jakelandis · 2018-09-05T18:36:16Z

jenkins test this please

Adds support for the Index Lifecycle Management Status to the Java High-Level Rest Client. Relates to elastic#33100

Adds support for the Index Lifecycle Management Status to the Java High-Level Rest Client. Relates to #33100

HLRC: Add ILM Status to HLRC

f5918db

Adds support for the Index Lifecycle Management Status to the Java High-Level Rest Client. Relates to elastic#33100

jakelandis added >enhancement :Data Management/ILM+SLM Index and Snapshot lifecycle management labels Aug 30, 2018

jakelandis requested review from dakrone, colings86, talevy, hub-cap and gwbrown August 30, 2018 19:09

colings86 mentioned this pull request Aug 30, 2018

Java high-level REST client completeness for ILM APIs #33100

Closed

13 tasks

dakrone requested changes Aug 30, 2018

View reviewed changes

gwbrown suggested changes Aug 30, 2018

View reviewed changes

review changes

916b535

* Fixed incorrect method uppercase * Renamed statsILM -> lifecycleManagementStatus * removed "generated" comments

colings86 reviewed Aug 31, 2018

View reviewed changes

jakelandis added 3 commits August 31, 2018 09:17

Merge branch 'index-lifecycle' into hlrc_ilm_status

3bcaee4

fix imports from merge

8b06ccd

use o.e.c.i.OperationMode and remove inner class of the same name

4f77a09

gwbrown approved these changes Sep 4, 2018

View reviewed changes

jakelandis added 2 commits September 4, 2018 11:33

Merge branch 'index-lifecycle' into hlrc_ilm_status

12f7a15

added LifecycleManagementStatusRequest removed direct usage of TimedR…

d755bd0

…equest

dakrone approved these changes Sep 4, 2018

View reviewed changes

use Locale.ROOT and string constant

80462d5

jakelandis merged commit 8b8ff2b into elastic:index-lifecycle Sep 5, 2018

jakelandis added the backport pending label Sep 5, 2018

jakelandis added a commit to jakelandis/elasticsearch that referenced this pull request Sep 5, 2018

HLRC: Add ILM Status to HLRC (elastic#33283)

0f80730

Adds support for the Index Lifecycle Management Status to the Java High-Level Rest Client. Relates to elastic#33100

jakelandis mentioned this pull request Sep 5, 2018

6.x - HLRC: Add ILM Status to HLRC (#33283) #33448

Merged

jakelandis added a commit that referenced this pull request Sep 6, 2018

HLRC: Add ILM Status to HLRC (#33283) (#33448)

801cde0

Adds support for the Index Lifecycle Management Status to the Java High-Level Rest Client. Relates to #33100

jakelandis removed the backport pending label Sep 6, 2018


		public class StatusILMResponseTests extends ESTestCase {

		public void testClientServerStatuses() {

HLRC: Add ILM Status to HLRC #33283

HLRC: Add ILM Status to HLRC #33283

Conversation

jakelandis commented Aug 30, 2018

elasticmachine commented Aug 30, 2018

dakrone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gwbrown left a comment

Choose a reason for hiding this comment

gwbrown Aug 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakelandis commented Aug 30, 2018 • edited Loading

gwbrown commented Aug 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gwbrown commented Sep 4, 2018

hub-cap commented Sep 4, 2018

jakelandis commented Sep 4, 2018

jakelandis commented Sep 4, 2018

dakrone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakelandis commented Sep 5, 2018

jakelandis commented Sep 5, 2018

gwbrown Aug 30, 2018 •

edited

Loading

jakelandis commented Aug 30, 2018 •

edited

Loading