Skip to content
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.

Sign up for GitHub

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

[TEST] Fix unit test failure in RestHighLevelClientTests #36

Merged
merged 3 commits into from
Feb 4, 2021
Merged

[TEST] Fix unit test failure in RestHighLevelClientTests #36

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dcf00b6
Add 'ilm_pilicy' field definition back to DataStream
Feb 3, 2021
e404c65
fix testProvidedNamedXContents by modifying the assertion
Feb 3, 2021
99dfc74
Revert "Add 'ilm_pilicy' field definition back to DataStream"
Feb 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions client/rest-high-level/src/main/java/org/elasticsearch/client/indices/DataStream.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ public String getIlmPolicyName() {
public static final ParseField GENERATION_FIELD = new ParseField("generation");
public static final ParseField STATUS_FIELD = new ParseField("status");
public static final ParseField INDEX_TEMPLATE_FIELD = new ParseField("template");
public static final ParseField ILM_POLICY_FIELD = new ParseField("ilm_policy");
Copy link
Collaborator

@nknize nknize Feb 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

which test is failing because of this? the concern here is that ILM is an xpack feature, so we're not going to want to keep this ParseField around. Do you want to open a separate PR to cleanly remove ILM from DataStreams or do you want to remove it in this PR (I see this PR is a WIP)?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh I see:

org.elasticsearch.client.indices.GetDataStreamResponseTests > testFromXContent FAILED
org.elasticsearch.common.xcontent.XContentParseException: [1:8288] [data_stream] unknown field [ilm_policy]

I think we should remove ilm_policy altogether from GetDataStreamAction.java instead of adding it back in. Looks like this was a side effect of reverting the DataStream commit.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. I will hold the ILM part on for now.
The reason I added back was I saw the other part of codes in the original PR was already reverted into this repo, and the license header was Apache 2.0.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know ODFE Index Mangement plugin is doing the job for Index Lifecycle, so seems there is something needs to be coordinated.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're safe to remove the ILM stuff altogether for now and we can revert those PRs if the Index Management team wants them back in?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to remove the change related to "ilm_policy" in this PR, and actually it's not a part of "RestHighLevelClientTests" class 😉


@SuppressWarnings("unchecked")
private static final ConstructingObjectParser<DataStream, Void> PARSER = new ConstructingObjectParser<>("data_stream",
Expand All @@ -110,6 +111,7 @@ public String getIlmPolicyName() {
PARSER.declareLong(ConstructingObjectParser.constructorArg(), GENERATION_FIELD);
PARSER.declareString(ConstructingObjectParser.constructorArg(), STATUS_FIELD);
PARSER.declareString(ConstructingObjectParser.optionalConstructorArg(), INDEX_TEMPLATE_FIELD);
PARSER.declareString(ConstructingObjectParser.optionalConstructorArg(), ILM_POLICY_FIELD);
}

public static DataStream fromXContent(XContentParser parser) throws IOException {
Expand Down
4 changes: 2 additions & 2 deletions client/rest-high-level/src/test/java/org/elasticsearch/client/RestHighLevelClientTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -660,7 +660,7 @@ public void testDefaultNamedXContents() {

public void testProvidedNamedXContents() {
List<NamedXContentRegistry.Entry> namedXContents = RestHighLevelClient.getProvidedNamedXContents();
assertEquals(75, namedXContents.size());
assertEquals(13, namedXContents.size());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aha, is this because of the xpack content removal?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Nick, as far as I find, it's due to the removal of x-pack "ml", but just a rough investigation.
Btw, I think I'd better move some of my comments from the meta issue(# 23) and describe more in the PR.

Map<Class<?>, Integer> categories = new HashMap<>();
List<String> names = new ArrayList<>();
for (NamedXContentRegistry.Entry namedXContent : namedXContents) {
Expand All @@ -670,7 +670,7 @@ public void testProvidedNamedXContents() {
categories.put(namedXContent.categoryClass, counter + 1);
}
}
assertEquals("Had: " + categories, 14, categories.size());
assertEquals("Had: " + categories, 3, categories.size());
assertEquals(Integer.valueOf(3), categories.get(Aggregation.class));
assertTrue(names.contains(ChildrenAggregationBuilder.NAME));
assertTrue(names.contains(MatrixStatsAggregationBuilder.NAME));
Expand Down