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

Add voting config exclusion add and clear API spec and integration test cases #55760

Merged
merged 4 commits into from Apr 29, 2020

Conversation

zacharymorn
Copy link
Contributor

Closes #48131

- contains : { metadata.cluster_coordination.voting_config_exclusions: {node_id: "_absent_", node_name: "nodeName2"} }

#---
# THIS TEST CASE WILL CAUSE TIMEOUT FOR CLEAR VOTING CONFIG OPERATION, HENCE BREAKING ALL TESTS
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For this test and the next, as there's only 1 master node in the cluster, I tried to post a voting config exclusion with that master node id / name. Doing so caused timeout exceptions for these tests themselves as well as others when executing clearing voting config exclusion in teardown. Not sure if there's a good workaround for this issue? I searched around to see if I can set up more nodes in the cluster for testing, but haven't found anything yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, waiting and then timing out is what I'd expect here. For the clear operation you can avoid the wait with wait_for_removal: true but there's no way to avoid the wait during the add operation. I think it'd be best to use a bogus node name or ID instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. For the clear operation I do need it to complete successfully to reset cluster voting config exclusion state for the next test, so I guess there's no obvious work around then.

Since I already have other tests to use bogus node name or id, I'll remove these two tests then.

@DaveCTurner DaveCTurner self-assigned this Apr 25, 2020
@DaveCTurner
Copy link
Contributor

@elasticmachine ok to test

Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

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

Looks good. There's a small typo. Also please merge master and fix up a test that I think will fail thanks to #55673.

- length: { metadata.cluster_coordination.voting_config_exclusions: 0 }

---
"Add voting config exclusion by unknonw node Id":
Copy link
Contributor

Choose a reason for hiding this comment

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

Unknown spelling ;)

Suggested change
"Add voting config exclusion by unknonw node Id":
"Add voting config exclusion by unknown node Id":

(typo is duplicated elsewhere too)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ops. Fixed.

---
"Throw exception when adding voting config exclusion without specifying nodes":
- do:
catch: /Please set node identifiers correctly. One and only one of \[node_name\], \[node_names\] and \[node_ids\] has to be set/
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this message no longer applies in v8 since the node_name parameter is now removed. I must remember to use this message in the backport, however.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

@DaveCTurner DaveCTurner dismissed their stale review April 29, 2020 07:32

comments addressed

Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

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

LGTM thanks @zacharymorn

@DaveCTurner DaveCTurner merged commit 498fc66 into elastic:master Apr 29, 2020
@DaveCTurner DaveCTurner added :Distributed/Cluster Coordination Cluster formation and cluster state publication, including cluster membership and fault detection. >enhancement v7.8.0 v8.0.0 labels Apr 29, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed (:Distributed/Cluster Coordination)

@elasticmachine elasticmachine added the Team:Distributed Meta label for distributed team label Apr 29, 2020
DaveCTurner pushed a commit to DaveCTurner/elasticsearch that referenced this pull request Apr 29, 2020
DaveCTurner added a commit that referenced this pull request Apr 29, 2020
Closes #48131
Backport of #55760

Co-authored-by: zacharymorn <zacharymorn@gmail.com>
russcam added a commit to elastic/elasticsearch-net that referenced this pull request Jun 4, 2020
Relates: #4718, elastic/elasticsearch#55760

This commit adds the voting config exclusions APIs to the high level client.
Custom response builder types are required because the APIs return
\n as the response, which the client will attempt to deserialize to a
response object.
russcam added a commit to elastic/elasticsearch-net that referenced this pull request Jun 10, 2020
Relates: #4718, elastic/elasticsearch#55760

This commit adds the voting config exclusions APIs to the high level client.
Custom response builder types are required because the APIs return
\n as the response, which the client will attempt to deserialize to a
response object.
github-actions bot pushed a commit to elastic/elasticsearch-net that referenced this pull request Jun 10, 2020
Relates: #4718, elastic/elasticsearch#55760

This commit adds the voting config exclusions APIs to the high level client.
Custom response builder types are required because the APIs return
\n as the response, which the client will attempt to deserialize to a
response object.
github-actions bot pushed a commit to elastic/elasticsearch-net that referenced this pull request Jun 10, 2020
Relates: #4718, elastic/elasticsearch#55760

This commit adds the voting config exclusions APIs to the high level client.
Custom response builder types are required because the APIs return
\n as the response, which the client will attempt to deserialize to a
response object.
russcam added a commit to elastic/elasticsearch-net that referenced this pull request Jun 10, 2020
Relates: #4718, elastic/elasticsearch#55760

This commit adds the voting config exclusions APIs to the high level client.
Custom response builder types are required because the APIs return
\n as the response, which the client will attempt to deserialize to a
response object.

Co-authored-by: Russ Cam <russ.cam@elastic.co>
russcam added a commit to elastic/elasticsearch-net that referenced this pull request Jun 10, 2020
Relates: #4718, elastic/elasticsearch#55760

This commit adds the voting config exclusions APIs to the high level client.
Custom response builder types are required because the APIs return
\n as the response, which the client will attempt to deserialize to a
response object.

Co-authored-by: Russ Cam <russ.cam@elastic.co>
@jakelandis jakelandis removed the v8.0.0 label Jul 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed/Cluster Coordination Cluster formation and cluster state publication, including cluster membership and fault detection. >enhancement Team:Distributed Meta label for distributed team v7.8.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Elasticsearch rest-api-spec is missing the Voting Config Exclusions API
4 participants