HLRC: Add remove index lifecycle policy by jdconrad · Pull Request #34204 · elastic/elasticsearch

jdconrad · 2018-10-01T22:21:54Z

This change adds the command RemoveIndexLifecyclePolicy to the HLRC. This uses the new TimeRequest as a base class for RemoveIndexLifecyclePolicyRequest on the client side.

elasticmachine · 2018-10-01T22:21:56Z

Pinging @elastic/es-core-infra

AthenaEryma · 2018-10-01T22:34:33Z

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

+    }
+
+    @Override
+    public RemoveIndexLifecyclePolicyRequest indices(String... indices) {


This is a question, not a change request: What is our philosophy regarding having setters vs. immutable request objects going forward for the HLRC? I've been under the impression we preferred immutable objects, but it doesn't seem to be consistent.

@gwbrown Good question that I would like to know the answer to myself :). I was mostly going off what I saw within the set index lifecycle policy classes since that's the analog to this request.

I think the most important thing is for the API to be built in a way where you cannot create invalid requests. It would be good if any mandatory fields in the request were passed into the constructor and don't have setters. The reason this was done for the server side was because the request extends IndicesRequest.Replaceable but I think in order to achieve the client separation we want we actually can't extend that here?

yea Im all for not exetnding that class. And Im also all for putting things that are primitive and easily validatable into the constructors. Optionals, i think im ok with setters but i think this also deserves a wider audience to discuss.

Removed the extension. However, IndicesOptions as a member variable still seems necessary. There is no way to specify how to handle wild cards appropriately without it, and the SetIndexLifecyclePolicyRequest also uses them. I'm open to suggestions here if this needs changes still.

colings86 · 2018-10-02T06:46:34Z

...est/java/org/elasticsearch/client/indexlifecycle/RemoveIndexLifecyclePolicyRequestTests.java

+            request.indices(generateRandomStringArray(20, 20, false));
+        }
+        return request;
+    }


I think we should implement mutateInstance so the hash code and equals tests can test non-equal instances too?

The framework changed with the removal the XContent methods. Added tests for equals and hashcode separately since this now extends from ESTestCase.

colings86 · 2018-10-02T06:46:56Z

...st/java/org/elasticsearch/client/indexlifecycle/RemoveIndexLifecyclePolicyResponseTests.java

+    protected RemoveIndexLifecyclePolicyResponse createTestInstance() {
+        List<String> failedIndexes = Arrays.asList(generateRandomStringArray(20, 20, false));
+        return new RemoveIndexLifecyclePolicyResponse(failedIndexes);
+    }


I think we should implement mutateInstance so the hash code and equals tests can test non-equal instances too?

Same as above. The framework changed with the removal the XContent methods. Added tests for equals and hashcode separately.

colings86 · 2018-10-02T06:50:50Z

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

+    }
+
+    @Override
+    public RemoveIndexLifecyclePolicyRequest indices(String... indices) {


I think the most important thing is for the API to be built in a way where you cannot create invalid requests. It would be good if any mandatory fields in the request were passed into the constructor and don't have setters. The reason this was done for the server side was because the request extends IndicesRequest.Replaceable but I think in order to achieve the client separation we want we actually can't extend that here?

hub-cap · 2018-10-02T16:00:08Z

...rc/main/java/org/elasticsearch/client/indexlifecycle/RemoveIndexLifecyclePolicyResponse.java

+    public XContentBuilder toXContent(XContentBuilder builder, ToXContent.Params params) throws IOException {
+        builder.startObject();
+        builder.field(HAS_FAILURES_FIELD.getPreferredName(), hasFailures());
+        builder.field(FAILED_INDEXES_FIELD.getPreferredName(), failedIndexes);


so this just returns emptyness if there are no failed indexes?

disregard, this should be removed.

hub-cap · 2018-10-02T16:12:10Z

...rc/main/java/org/elasticsearch/client/indexlifecycle/RemoveIndexLifecyclePolicyResponse.java

+    }
+
+    @Override
+    public XContentBuilder toXContent(XContentBuilder builder, ToXContent.Params params) throws IOException {


this should not be in the responses. @nik9000 has recently merged some test stuff that will alter your test cases below as well, once this is removed. Pls update to use that.

jdconrad · 2018-10-03T23:54:08Z

@colings86 @hub-cap Thanks for the first reviews. I think I addressed all the PR comments, and this is ready for another round.

colings86 · 2018-10-04T08:20:40Z

...est/java/org/elasticsearch/client/indexlifecycle/RemoveIndexLifecyclePolicyRequestTests.java

        assertFalse(request.validate().isPresent());
    }
+
+    public void testEqualsAndHashCode() {


Maybe we should use EqualsHashCodeTestUtils.checkEqualsAndHashCode() in this test so we have consistent testing with the rest of the codebase for hashcode and equals?

Done. Thanks for pointing out this utility method!

jdconrad · 2018-10-04T18:59:53Z

@colings86 Updated the equals and hashcode tests to use the utility method you pointed me at. Thanks for pointing that out. Would you please take another look when you get the chance?

jdconrad · 2018-10-04T20:44:51Z

@elasticmachine test this please

colings86

@jdconrad I left a nit comment but this LGTM. I would like @hub-cap or @gwbrown to take a look before we merge it as they are more familiar with the HLRC itself.

colings86 · 2018-10-05T06:33:55Z

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

        return request;
    }

+    static Request removeIndexLifecyclePolicy(RemoveIndexLifecyclePolicyRequest setPolicyRequest) {


nit: can we named the variable removeLifecycleRequest?

Oops! Good catch, thanks.

hub-cap

a minor constructor nit, and then one CYA on my part WRT a rename.

hub-cap · 2018-10-15T18:20:51Z

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

+    private final IndicesOptions indicesOptions;
+
+    public RemoveIndexLifecyclePolicyRequest(List<String> indices) {
+        if (indices == null) {


Objects.requireNotNull

This method also does not need to exist, as you can use this(indices, IndicesOptions.strictExpandOpen()), and fix the validation in the other constructor.

A++ new constructor code

hub-cap · 2018-10-15T18:30:30Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/XPackClientPlugin.java

                ExplainLifecycleAction.INSTANCE,
                SetIndexLifecyclePolicyAction.INSTANCE,
-                RemovePolicyForIndexAction.INSTANCE,
+                RemoveIndexLifecyclePolicyAction.INSTANCE,


Im going to assume all of these changes are approved by someone else :)

Name changes match the changes to SetIndexLifecyclePolicyAction.

jdconrad · 2018-10-16T15:12:26Z

@colings86 @gwbrown @hub-cap Thanks for the reviews!

colings86 · 2018-10-16T15:17:27Z

@jdconrad Thanks for doing this one 😄

This change adds the command RemoveIndexLifecyclePolicy to the HLRC. This uses the new TimeRequest as a base class for RemoveIndexLifecyclePolicyRequest on the client side.

jdconrad added 2 commits October 1, 2018 14:27

Add remove lifecycle index policy API to the ilm client.

dfb6805

Add more tests.

6444c77

jdconrad added v7.0.0 :Core/Java High Level REST Client :Data Management/ILM+SLM DO NOT USE. Use ":StorageEngine/ILM" or ":Distributed Coordination/SLM" instead. v6.5.0 labels Oct 1, 2018

jdconrad requested review from colings86 and hub-cap October 1, 2018 22:21

colings86 mentioned this pull request Oct 1, 2018

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

Closed

13 tasks

AthenaEryma reviewed Oct 1, 2018

View reviewed changes

colings86 reviewed Oct 2, 2018

View reviewed changes

hub-cap suggested changes Oct 2, 2018

View reviewed changes

jdconrad added 4 commits October 3, 2018 08:41

Merge branch 'index-lifecycle' into ilmc2

6734718

Response to PR comments.

86d3a6d

Merge branch 'index-lifecycle' into ilmc2

9a0df97

More response to PR comments.

5c505ed

colings86 reviewed Oct 4, 2018

View reviewed changes

jdconrad added 2 commits October 4, 2018 07:40

Merge branch 'index-lifecycle' into ilmc2

f741233

Update equals and hashcode tests.

2a427cd

colings86 reviewed Oct 5, 2018

View reviewed changes

jdconrad added 3 commits October 10, 2018 09:57

Merge branch 'index-lifecycle' into ilmc2

b70a690

Fix variable name.

007a9b9

Merge branch 'index-lifecycle' into ilmc2

ee002b8

hub-cap approved these changes Oct 15, 2018

View reviewed changes

Response to PR comments.

168999c

jdconrad merged commit 80474e1 into elastic:index-lifecycle Oct 16, 2018

jdconrad added the backport pending label Oct 16, 2018

jdconrad removed the backport pending label Oct 16, 2018

colings86 added the >non-issue label Oct 25, 2018

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

Conversation

jdconrad commented Oct 1, 2018

Uh oh!

elasticmachine commented Oct 1, 2018

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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

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!

jdconrad commented Oct 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdconrad commented Oct 4, 2018

Uh oh!

jdconrad commented Oct 4, 2018

Uh oh!

colings86 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!

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!

jdconrad commented Oct 16, 2018

Uh oh!

colings86 commented Oct 16, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

jdconrad Oct 3, 2018 •

edited

Loading

jdconrad Oct 3, 2018 •

edited

Loading