-
Notifications
You must be signed in to change notification settings - Fork 25k
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
Conversation
- 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 |
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.
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.
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.
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.
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.
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.
… which caused timeout exception
@elasticmachine ok to test |
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.
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": |
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.
Unknown spelling ;)
"Add voting config exclusion by unknonw node Id": | |
"Add voting config exclusion by unknown node Id": |
(typo is duplicated elsewhere too)
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.
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/ |
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.
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.
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.
Updated.
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.
LGTM thanks @zacharymorn
Pinging @elastic/es-distributed (:Distributed/Cluster Coordination) |
Closes #48131 Backport of #55760 Co-authored-by: zacharymorn <[email protected]>
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.
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.
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.
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.
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 <[email protected]>
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 <[email protected]>
Closes #48131