HLRC: Add Lifecycle delete to the HLRC #33142

gwbrown · 2018-08-24T23:31:32Z

Adds support for Lifecycle deletion to the Java High-Level Rest Client.
Also includes some stopgap measures to support using the new Client
package structure which should be replaced once the structure
refactoring is complete.

Relates to #33100

Adds support for Lifecycle deletion to the Java High-Level Rest Client. Also includes some stopgap measures to support using the new Client package structure which should be replaced once the structure refactoring is complete. Relates to elastic#33100

elasticmachine · 2018-08-24T23:31:33Z

Pinging @elastic/es-core-infra

dakrone · 2018-08-27T14:54:42Z

...level/src/test/java/org/elasticsearch/client/indexlifecycle/DeleteLifecycleRequestTests.java

+    }
+
+    public void testValidate() {
+        if (frequently()) {


Can you split this into two different tests rather than using the randomness here? I don't think it buys us anything

dakrone

This LGTM assuming @hub-cap is good with the MasterTimeoutRequest change

jakelandis · 2018-08-27T16:27:58Z

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

+
+import java.util.Objects;
+
+public class DeleteLifecycleRequest extends MasterTimeoutRequest<DeleteLifecycleRequest> {


The transport based APIs extend AcknowledgedRequest which has a timeout and ackTimeout in addition to the masterNodeTimeout.

(this does not) Curious if is this is intentional ?

The javadoc for AcknowledgedRequest states Facilitates consistency across different api. ... is that something we should carry forward while still divorcing ourselves from the transport request objects ?

I based MasterTimeoutRequest on MasterNodeRequest, rather than AcknowledgedRequest, which doesn't have an ackTimeout. It's possible I should be using AcknowledgedRequest instead - the difference wasn't immediately clear.

So, to answer your question: It was intentional, but it's possible I intended the wrong thing.

I talked to @hub-cap on Slack and his answer was that these timeouts should be based on the parameters that the requests can take. To determine this, there should be a Rest<whatever>Action class which defines those parameters. In this case, RestDeleteLifecycleAction (see here) defines both timeout and master_timeout, so this class should include both.

(Just waiting on some more feedback to make this change)

@gwbrown Thanks for following up. I am working a similar issue and will follow your lead here.

jakelandis · 2018-08-27T16:42:42Z

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

@@ -0,0 +1,55 @@
+/*


Should this be DeleteIndexLifecycleRequest ?

From what I saw, the classes with names including IndexLifecycle seem to relate to index<->lifecycle policy relationships (i.e. lifecycle A applies to index B), but ones with just Lifecycle relate to the management of lifecycles. If that's correct, this class is named correctly.

@dakrone, can you confirm?

I think this should be DeleteLifecyclePolicyRequest to indicate that the policy is going to be removed, and then DeleteIndexLifecycleRequest is for when we are removing a lifecycle from an index

Based on feedback in elastic#33142

colings86

I left a couple of minor comments

colings86 · 2018-08-28T08:09:08Z

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 deleteLifecycleAsync(DeleteLifecyclePolicyRequest request, RequestOptions options,


In the same spirit as @dakrone's comment about renaming the request object to DeleteLifecyclePolicyRequest I think we should rename these methods to deleteLifecyclePolicy and deleteLifecyclePolicyAsync?

colings86 · 2018-08-28T08:09:45Z

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

@@ -1174,6 +1175,17 @@ static Request xpackUsage(XPackUsageRequest usageRequest) {
        return request;
    }

+    static Request deleteLifecycle(DeleteLifecyclePolicyRequest deleteLifecyclePolicyRequest) {


Rename to deleteLifecyclePolicy?

Yep, +1 to rename this deleteLifecyclePolicy

gwbrown · 2018-08-28T16:41:57Z

Just had a brief meeting with @hub-cap - the result of that meeting is that there are some not-well-understood requirements around exactly which timeouts need to/can be shared between request classes. Baz is going to figure those requirements out when he has bandwidth to do so. In the meantime the abstract request class in this PR will change to include both timeout and master_timeout to allow moving forward with this and other ILM HLRC PR's and we'll consolidate to the final hierarchy later - it ought to be pretty straightforward.

gwbrown · 2018-08-29T15:28:32Z

I lied - @hub-cap got the base class into master very quickly (thanks!) so this PR has transitioned to using that base class.

I've made all the changes suggest so far, so any last comments on this? If not I'll merge about EOD (this comment + 7 hours) today, assuming CI passes.

dakrone

I left comments about some renames

dakrone · 2018-08-29T15:44:17Z

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

@@ -1174,6 +1175,17 @@ static Request xpackUsage(XPackUsageRequest usageRequest) {
        return request;
    }

+    static Request deleteLifecycle(DeleteLifecyclePolicyRequest deleteLifecyclePolicyRequest) {


Yep, +1 to rename this deleteLifecyclePolicy

dakrone · 2018-08-29T15:44:43Z

...evel/src/main/java/org/elasticsearch/client/indexlifecycle/DeleteLifecyclePolicyRequest.java

+        this.lifecycle = lifecycle;
+    }
+
+    public String getLifecycle() {


Can you also rename these getLifecyclePolicy?

dakrone · 2018-08-29T15:46:29Z

...src/test/java/org/elasticsearch/client/indexlifecycle/DeleteLifecyclePolicyRequestTests.java

+        try {
+            DeleteLifecyclePolicyRequest req = new DeleteLifecyclePolicyRequest(randomFrom("", null));
+            fail("should not be able to create a DeleteLifecyclePolicyRequest with null lifecycle name");
+        } catch (IllegalArgumentException exception) {


For the future, you can use

expectThrows(IllegalArgumentException.class, () -> new DeleteLifecyclePolicyRequest(randomFrom("", null)));

Instead of having to do the try/fail/catch check

Per review comments in elastic#33142

hub-cap · 2018-08-29T17:38:33Z

@gwbrown its also been backported as of about an hour ago. So you should be good to go for any 6.5 merging as well.

dakrone

LGTM

Adds support for Lifecycle Policy deletion to the Java High-Level Rest Client. Relates to #33100

gwbrown added the :Data Management/ILM+SLM Index and Snapshot lifecycle management label Aug 24, 2018

gwbrown requested review from dakrone, colings86 and hub-cap August 24, 2018 23:31

Add default to master timeout class

b3cd4c3

dakrone reviewed Aug 27, 2018

View reviewed changes

Split up test case (elastic#33142)

60eb043

dakrone approved these changes Aug 27, 2018

View reviewed changes

jakelandis reviewed Aug 27, 2018

View reviewed changes

gwbrown added 3 commits August 27, 2018 11:52

Stronger validation for Delete request

3f02774

Rename delete request class

7b14f1b

Based on feedback in elastic#33142

Merge branch 'index-lifecycle' into hlrc-delete

4c6a5c6

colings86 mentioned this pull request Aug 27, 2018

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

Closed

13 tasks

colings86 reviewed Aug 28, 2018

View reviewed changes

gwbrown added 3 commits August 28, 2018 11:01

Rename ILM client methods to match class name

24c2f6a

Make test utility more flexible

5f8e8d0

Merge branch 'index-lifecycle' into hlrc-delete

6681ccc

dakrone requested changes Aug 29, 2018

View reviewed changes

gwbrown added 2 commits August 29, 2018 10:27

Change lifecycle to lifecyclePolicy

b65b83b

Per review comments in elastic#33142

Simplify validation test

95dce78

dakrone approved these changes Aug 29, 2018

View reviewed changes

gwbrown merged commit ba8d4eb into elastic:index-lifecycle Aug 29, 2018

gwbrown added a commit that referenced this pull request Aug 29, 2018

HLRC: Add Lifecycle Policy delete to HLRC (#33142)

d34dcb2

Adds support for Lifecycle Policy deletion to the Java High-Level Rest Client. Relates to #33100

gwbrown deleted the hlrc-delete branch December 7, 2018 04:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HLRC: Add Lifecycle delete to the HLRC #33142

HLRC: Add Lifecycle delete to the HLRC #33142

gwbrown commented Aug 24, 2018

elasticmachine commented Aug 24, 2018

dakrone Aug 27, 2018

dakrone left a comment

jakelandis Aug 27, 2018 •

edited

Loading

gwbrown Aug 27, 2018

gwbrown Aug 27, 2018 •

edited

Loading

jakelandis Aug 27, 2018

jakelandis Aug 27, 2018

gwbrown Aug 27, 2018

dakrone Aug 27, 2018

colings86 left a comment

colings86 Aug 28, 2018

colings86 Aug 28, 2018

dakrone Aug 29, 2018

gwbrown commented Aug 28, 2018

gwbrown commented Aug 29, 2018 •

edited

Loading

dakrone left a comment

dakrone Aug 29, 2018

dakrone Aug 29, 2018

dakrone Aug 29, 2018

hub-cap commented Aug 29, 2018

dakrone left a comment


		import java.util.Objects;

		public class DeleteLifecycleRequest extends MasterTimeoutRequest<DeleteLifecycleRequest> {

HLRC: Add Lifecycle delete to the HLRC #33142

HLRC: Add Lifecycle delete to the HLRC #33142

Conversation

gwbrown commented Aug 24, 2018

elasticmachine commented Aug 24, 2018

Choose a reason for hiding this comment

dakrone left a comment

Choose a reason for hiding this comment

jakelandis Aug 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gwbrown Aug 27, 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

colings86 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

gwbrown commented Aug 28, 2018

gwbrown commented Aug 29, 2018 • edited Loading

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

hub-cap commented Aug 29, 2018

dakrone left a comment

Choose a reason for hiding this comment

jakelandis Aug 27, 2018 •

edited

Loading

gwbrown Aug 27, 2018 •

edited

Loading

gwbrown commented Aug 29, 2018 •

edited

Loading