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.

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

Make XContentBuilder in AliasActions build is_write_index field #35071

Merged
merged 3 commits into from
Oct 31, 2018

Conversation

upgle
Copy link
Contributor

@upgle upgle commented Oct 30, 2018

Alias 'is_write_index' feature was introduced in 6.4 version. but, toXContent method of AliasActions does not build for is_write_index field.

so is_write_index can not be set with IndicesAliasesRequest (RestHighLevelClient)

You can see it in the code below.

public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {

Sample of RestHighLevelClient

It does not set is_write_index despite specifying options with writeIndex chaining method.

IndicesAliasesRequest request = new IndicesAliasesRequest();
IndicesAliasesRequest.AliasActions aliasAction =
        new IndicesAliasesRequest.AliasActions(IndicesAliasesRequest.AliasActions.Type.ADD)
                .index(index)
                .alias(alias)
                .filter(filter)
                .writeIndex(isWriteIndex); // this chaining method does not working.

request.addAliasAction(aliasAction);
return client.indices().updateAliases(request, RequestOptions.DEFAULT);

Notes

  • I added test for this PR

@colings86 colings86 added >enhancement :Data Management/Indices APIs APIs to create and manage indices and templates labels Oct 30, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@talevy talevy self-requested a review October 30, 2018 16:16
Copy link
Contributor

@talevy talevy left a comment

Choose a reason for hiding this comment

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

thank you @upgle for contributing this fix and bringing it to our attention!

Rest High Level Client was missed here.

Your changes look good, but I think they are missing three things

  • the IndicesClientIT changes would benefit from randomization
  • The IndicesAliasesRequest's hashCode/toString/equals are also missing writeIndex
  • Add test on the server-side for IndicesAliasesRequestTests, where toXContent is tested. This means updating RandomAliasActionsGenerator.java to randomly include writeIndex.

I've addressed these concerns in a commit to make it clearer: talevy@b72608d

thanks!

@upgle
Copy link
Contributor Author

upgle commented Oct 30, 2018

Hi @talevy, Thank you for your kind comments.
I've modified this PR with reference to the code you shared. Can I get a review again, please?

@talevy
Copy link
Contributor

talevy commented Oct 30, 2018

thank you for for the update, I will run our continuous integration test suite against this to double check we didn't miss anything

@talevy
Copy link
Contributor

talevy commented Oct 30, 2018

@elasticmachine, test this please

@upgle upgle force-pushed the patch-write-index branch from c167803 to 245b473 Compare October 31, 2018 01:37
@upgle
Copy link
Contributor Author

upgle commented Oct 31, 2018

@talevy
Build is failed here https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+pull-request/1418/
I found version confilict engine exceptoin.. so I rebased it on top of master.

[2018-10-30T19:30:22,495][DEBUG][o.e.a.g.TransportGetAction] [node-3] null: failed to execute [get [test_1][test][1]: routing [null]]
org.elasticsearch.transport.RemoteTransportException: [node-1][127.0.0.1:40018][indices:data/read/get[s]]
Caused by: org.elasticsearch.index.engine.VersionConflictEngineException: [test][1]: version conflict, current version [2] is different than the one provided [1]
at org.elasticsearch.index.engine.Engine.getFromSearcher(Engine.java:618) ~[elasticsearch-7.0.0-alpha1-SNAPSHOT.jar:7.0.0-alpha1-SNAPSHOT]
at org.elasticsearch.index.engine.InternalEngine.get(InternalEngine.java:659) ~[elasticsearch-7.0.0-alpha1-SNAPSHOT.jar:7.0.0-alpha1-SNAPSHOT]

@upgle
Copy link
Contributor Author

upgle commented Oct 31, 2018

@talevy
And also found.. MixedClusterClientYamlTestSuiteIT.test failures

Tests with failures:
  - org.elasticsearch.backwards.MixedClusterClientYamlTestSuiteIT.test {p0=search/10_source_filtering/_source_include}
  - org.elasticsearch.backwards.MixedClusterClientYamlTestSuiteIT.test {p0=search/10_source_filtering/_source include and _source in body}
  - org.elasticsearch.backwards.MixedClusterClientYamlTestSuiteIT.test {p0=get_source/70_source_filtering/Source filtering}
  - org.elasticsearch.backwards.MixedClusterClientYamlTestSuiteIT.test {p0=get/71_source_filtering_with_types/Source filtering}
  - org.elasticsearch.backwards.MixedClusterClientYamlTestSuiteIT.test {p0=explain/20_source_filtering/Source filtering}

I think we need to retry integration test again. This seems to be caused by this issue. 6256c33

@talevy
Copy link
Contributor

talevy commented Oct 31, 2018

thanks for applying the rebase. I will rerun the tests

@talevy
Copy link
Contributor

talevy commented Oct 31, 2018

@elasticmachine, test this please

@upgle
Copy link
Contributor Author

upgle commented Oct 31, 2018

Hi @talevy, test is just passed 🎉

Copy link
Contributor

@talevy talevy left a comment

Choose a reason for hiding this comment

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

thank you @upgle! I'll go ahead and merge this into the appropriate branches

@talevy talevy merged commit 9ef4788 into elastic:master Oct 31, 2018
talevy pushed a commit that referenced this pull request Oct 31, 2018
…5071)

Make XContentBuilder in AliasesActions build `is_write_index` field
talevy pushed a commit that referenced this pull request Oct 31, 2018
…5071)

Make XContentBuilder in AliasesActions build `is_write_index` field
talevy pushed a commit that referenced this pull request Oct 31, 2018
…5071)

Make XContentBuilder in AliasesActions build `is_write_index` field
@colings86 colings86 added v6.5.0 and removed v6.5.1 labels Nov 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants