-
Notifications
You must be signed in to change notification settings - Fork 9.8k
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
etcdserver, embed: stricter reconfig checking #6106
etcdserver, embed: stricter reconfig checking #6106
Conversation
do we have an e2e test for this feature yet? |
nope, I'll see about adding some |
f08210f
to
8c8e4f4
Compare
Added an integration test; e2e would've been kind of messy. /cc @xiang90 |
8c8e4f4
to
bf3d704
Compare
|
||
// all attempts to add member should fail | ||
for i := 1; i < len(c.Members); i++ { | ||
if err := c.addMemberByURL(t, c.URL(i), "unix://foo:12345"); err == nil { |
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.
check the exact error? if err != xxx
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.
the error's kind of misleading; it gets turned into a no leader error because the api gives a 500 error and the client converts it...
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.
:(. ok...
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.
can do add comment and a todo to fix this?
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'll just fix it now instead of reusing the not enough started error message...
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.
punting on this, using the comment+TODO; change would be too involved for this patch
lgtm |
bf3d704
to
9063ce5
Compare
Make --strict-reconfig-check a default and check if cluster is healthy when adding a member.
Make --strict-reconfig-check a default and check if cluster is healthy when adding a member.