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

Support dropping non-Raft nodes #4310

Merged
merged 1 commit into from
Oct 4, 2015
Merged

Support dropping non-Raft nodes #4310

merged 1 commit into from
Oct 4, 2015

Conversation

otoolep
Copy link
Contributor

@otoolep otoolep commented Oct 3, 2015

With this change dropping non-Raft nodes is supported.

@otoolep
Copy link
Contributor Author

otoolep commented Oct 3, 2015

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()
Copy link
Contributor Author

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.

@otoolep
Copy link
Contributor Author

otoolep commented Oct 3, 2015

Started with #4233.

This a minimum viable fix for the "extra" nodes reporting in #4212.

@otoolep
Copy link
Contributor Author

otoolep commented Oct 3, 2015

I'd like feedback from @corylanou and @jwilder

@otoolep otoolep force-pushed the drop_node_non_raft branch 2 times, most recently from 83b39d4 to 63185eb Compare October 3, 2015 04:25
@otoolep
Copy link
Contributor Author

otoolep commented Oct 3, 2015

Raft-check updated to take place at a higher-level, code now compiles.

@otoolep
Copy link
Contributor Author

otoolep commented Oct 3, 2015

Tested:

> show servers
id      cluster_addr    raft    raft-leader
1       localhost:8088  true    true
2       localhost:8188  true    false
3       localhost:8288  true    false

> drop server 3
ERR: node is a Raft node
Warning: It is possible this error is due to not setting a database.
Please set a database with the command "use <database>".
> 

@otoolep
Copy link
Contributor Author

otoolep commented Oct 3, 2015

Patch ready for merging, 4 green CI builds.

https://circleci.com/gh/influxdb/influxdb/tree/drop_node_non_raft

@otoolep otoolep closed this Oct 3, 2015
@otoolep otoolep reopened this Oct 3, 2015
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" {
Copy link
Contributor

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.

@corylanou
Copy link
Contributor

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

@toddboom
Copy link
Contributor

toddboom commented Oct 4, 2015

👍 for getting this out as an intermediate step.

@otoolep
Copy link
Contributor Author

otoolep commented Oct 4, 2015

Made the change suggested by @corylanou -- merging on green.

otoolep added a commit that referenced this pull request Oct 4, 2015
@otoolep otoolep merged commit 129d042 into master Oct 4, 2015
@otoolep otoolep deleted the drop_node_non_raft branch October 4, 2015 07:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants