-
Notifications
You must be signed in to change notification settings - Fork 25.2k
HLRC: Add remove index lifecycle policy #34204
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
dfb6805
6444c77
6734718
86d3a6d
9a0df97
5c505ed
f741233
2a427cd
b70a690
007a9b9
ee002b8
168999c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,80 @@ | ||
/* | ||
* Licensed to Elasticsearch under one or more contributor | ||
* license agreements. See the NOTICE file distributed with | ||
* this work for additional information regarding copyright | ||
* ownership. Elasticsearch licenses this file to you under | ||
* the Apache License, Version 2.0 (the "License"); you may | ||
* not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, | ||
* software distributed under the License is distributed on an | ||
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
* KIND, either express or implied. See the License for the | ||
* specific language governing permissions and limitations | ||
* under the License. | ||
*/ | ||
|
||
package org.elasticsearch.client.indexlifecycle; | ||
|
||
import org.elasticsearch.action.IndicesRequest; | ||
import org.elasticsearch.action.support.IndicesOptions; | ||
import org.elasticsearch.client.TimedRequest; | ||
|
||
import java.util.Arrays; | ||
import java.util.Objects; | ||
|
||
public class RemoveIndexLifecyclePolicyRequest extends TimedRequest implements IndicesRequest.Replaceable { | ||
|
||
private String[] indices; | ||
private IndicesOptions indicesOptions = IndicesOptions.strictExpandOpen(); | ||
|
||
public RemoveIndexLifecyclePolicyRequest() { | ||
} | ||
|
||
public RemoveIndexLifecyclePolicyRequest(String... indices) { | ||
if (indices == null) { | ||
throw new IllegalArgumentException("indices cannot be null"); | ||
} | ||
this.indices = indices; | ||
} | ||
|
||
@Override | ||
public RemoveIndexLifecyclePolicyRequest indices(String... indices) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
this.indices = indices; | ||
return this; | ||
} | ||
|
||
@Override | ||
public String[] indices() { | ||
return indices; | ||
} | ||
|
||
public void indicesOptions(IndicesOptions indicesOptions) { | ||
this.indicesOptions = indicesOptions; | ||
} | ||
|
||
public IndicesOptions indicesOptions() { | ||
return indicesOptions; | ||
} | ||
|
||
@Override | ||
public int hashCode() { | ||
return Objects.hash(Arrays.hashCode(indices), indicesOptions); | ||
} | ||
|
||
@Override | ||
public boolean equals(Object obj) { | ||
if (obj == null) { | ||
return false; | ||
} | ||
if (getClass() != obj.getClass()) { | ||
return false; | ||
} | ||
RemoveIndexLifecyclePolicyRequest other = (RemoveIndexLifecyclePolicyRequest) obj; | ||
return Objects.deepEquals(indices, other.indices) && | ||
Objects.equals(indicesOptions, other.indicesOptions); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,95 @@ | ||
/* | ||
* Licensed to Elasticsearch under one or more contributor | ||
* license agreements. See the NOTICE file distributed with | ||
* this work for additional information regarding copyright | ||
* ownership. Elasticsearch licenses this file to you under | ||
* the Apache License, Version 2.0 (the "License"); you may | ||
* not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, | ||
* software distributed under the License is distributed on an | ||
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
* KIND, either express or implied. See the License for the | ||
* specific language governing permissions and limitations | ||
* under the License. | ||
*/ | ||
|
||
package org.elasticsearch.client.indexlifecycle; | ||
|
||
import org.elasticsearch.common.ParseField; | ||
import org.elasticsearch.common.xcontent.ConstructingObjectParser; | ||
import org.elasticsearch.common.xcontent.ToXContent; | ||
import org.elasticsearch.common.xcontent.ToXContentObject; | ||
import org.elasticsearch.common.xcontent.XContentBuilder; | ||
import org.elasticsearch.common.xcontent.XContentParser; | ||
|
||
import java.io.IOException; | ||
import java.util.List; | ||
import java.util.Objects; | ||
|
||
public class RemoveIndexLifecyclePolicyResponse implements ToXContentObject { | ||
|
||
public static final ParseField HAS_FAILURES_FIELD = new ParseField("has_failures"); | ||
public static final ParseField FAILED_INDEXES_FIELD = new ParseField("failed_indexes"); | ||
@SuppressWarnings("unchecked") | ||
public static final ConstructingObjectParser<RemoveIndexLifecyclePolicyResponse, Void> PARSER = new ConstructingObjectParser<>( | ||
"change_policy_for_index_response", a -> new RemoveIndexLifecyclePolicyResponse((List<String>) a[0])); | ||
static { | ||
PARSER.declareStringArray(ConstructingObjectParser.constructorArg(), FAILED_INDEXES_FIELD); | ||
// Needs to be declared but not used in constructing the response object | ||
PARSER.declareBoolean(ConstructingObjectParser.constructorArg(), HAS_FAILURES_FIELD); | ||
} | ||
|
||
private List<String> failedIndexes; | ||
|
||
public RemoveIndexLifecyclePolicyResponse() { | ||
} | ||
|
||
public RemoveIndexLifecyclePolicyResponse(List<String> failedIndexes) { | ||
if (failedIndexes == null) { | ||
throw new IllegalArgumentException(FAILED_INDEXES_FIELD.getPreferredName() + " cannot be null"); | ||
} | ||
this.failedIndexes = failedIndexes; | ||
} | ||
|
||
public List<String> getFailedIndexes() { | ||
return failedIndexes; | ||
} | ||
|
||
public boolean hasFailures() { | ||
return failedIndexes.isEmpty() == false; | ||
} | ||
|
||
@Override | ||
public XContentBuilder toXContent(XContentBuilder builder, ToXContent.Params params) throws IOException { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
builder.startObject(); | ||
builder.field(HAS_FAILURES_FIELD.getPreferredName(), hasFailures()); | ||
builder.field(FAILED_INDEXES_FIELD.getPreferredName(), failedIndexes); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. so this just returns emptyness if there are no failed indexes? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. disregard, this should be removed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed. |
||
builder.endObject(); | ||
return builder; | ||
} | ||
|
||
public static RemoveIndexLifecyclePolicyResponse fromXContent(XContentParser parser) { | ||
return PARSER.apply(parser, null); | ||
} | ||
|
||
@Override | ||
public int hashCode() { | ||
return Objects.hash(failedIndexes); | ||
} | ||
|
||
@Override | ||
public boolean equals(Object obj) { | ||
if (obj == null) { | ||
return false; | ||
} | ||
if (getClass() != obj.getClass()) { | ||
return false; | ||
} | ||
RemoveIndexLifecyclePolicyResponse other = (RemoveIndexLifecyclePolicyResponse) obj; | ||
return Objects.equals(failedIndexes, other.failedIndexes); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
/* | ||
* Licensed to Elasticsearch under one or more contributor | ||
* license agreements. See the NOTICE file distributed with | ||
* this work for additional information regarding copyright | ||
* ownership. Elasticsearch licenses this file to you under | ||
* the Apache License, Version 2.0 (the "License"); you may | ||
* not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, | ||
* software distributed under the License is distributed on an | ||
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
* KIND, either express or implied. See the License for the | ||
* specific language governing permissions and limitations | ||
* under the License. | ||
*/ | ||
|
||
package org.elasticsearch.client.indexlifecycle; | ||
|
||
import org.elasticsearch.action.support.IndicesOptions; | ||
import org.elasticsearch.test.ESTestCase; | ||
|
||
public class RemoveIndexLifecyclePolicyRequestTests extends ESTestCase { | ||
|
||
private RemoveIndexLifecyclePolicyRequest createTestInstance() { | ||
RemoveIndexLifecyclePolicyRequest request = new RemoveIndexLifecyclePolicyRequest(generateRandomStringArray(20, 20, false)); | ||
if (randomBoolean()) { | ||
IndicesOptions indicesOptions = IndicesOptions.fromOptions(randomBoolean(), randomBoolean(), randomBoolean(), randomBoolean(), | ||
randomBoolean(), randomBoolean(), randomBoolean()); | ||
request.indicesOptions(indicesOptions); | ||
} | ||
if (randomBoolean()) { | ||
request.indices(generateRandomStringArray(20, 20, false)); | ||
} | ||
return request; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should implement There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The framework changed with the removal the XContent methods. Added tests for equals and hashcode separately since this now extends from ESTestCase. |
||
|
||
public void testNullIndices() { | ||
IllegalArgumentException exception = expectThrows(IllegalArgumentException.class, | ||
() -> new RemoveIndexLifecyclePolicyRequest((String[]) null)); | ||
assertEquals("indices cannot be null", exception.getMessage()); | ||
} | ||
|
||
public void testValidate() { | ||
RemoveIndexLifecyclePolicyRequest request = createTestInstance(); | ||
assertFalse(request.validate().isPresent()); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can we named the variable
removeLifecycleRequest
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops! Good catch, thanks.