-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Support dropping non-Raft nodes #4310
Conversation
This is still work-in-progress, and is mostly code from @corylanou The idea here is that we get incremental support out for this command, for very specific situations. cc @jwilder |
return ErrNodeNotFound | ||
} | ||
// Only dropping non-Raft nodes currently supported. | ||
peers, err := e.Store.Peers() |
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.
This doesn't compile, but it's the check I want to add. Once this check is in place, we can forget about all the Raft complexity until later.
I'd like feedback from @corylanou and @jwilder |
83b39d4
to
63185eb
Compare
Raft-check updated to take place at a higher-level, code now compiles. |
Tested:
|
35971b2
to
cc03373
Compare
Patch ready for merging, 4 green CI builds. https://circleci.com/gh/influxdb/influxdb/tree/drop_node_non_raft |
e.Store.PeersFn = func() ([]string, error) { | ||
return []string{"node1"}, nil | ||
} | ||
if res := e.ExecuteStatement(influxql.MustParseStatement(`DROP SERVER 1`)); res.Err == nil || res.Err.Error() != "node is a Raft node" { |
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.
Could use ErrNodeRaft
here to lessen the brittleness of this test.
This looks good. Limiting it to non-raft nodes takes out most of the real hard problems and allows us to move forward in another PR to do it right. +1 |
👍 for getting this out as an intermediate step. |
cc03373
to
2ac0357
Compare
Made the change suggested by @corylanou -- merging on green. |
Support dropping non-Raft nodes
With this change dropping non-Raft nodes is supported.