-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Integrates Consul with "stage one" of HashiCorp Raft library v2. #2222
Conversation
660adb8
to
8dd1207
Compare
The dependent changes have been pulled into the https://github.com/hashicorp/raft/tree/library-v2-stage-one branch over in Raft, which will serve as our stable branch for Consul as other Raft refactoring takes place. These changes are now vendored so this is ready for review. |
Switched to govendor so rebase incoming to fix up the metadata. |
This isn't safe because it would implicitly commit all outstanding log entries. The new Raft library already has logic to not start a vote if the current node isn't in the configuration, so this shoudn't be needed.
if err := s.raft.SetPeers(addrs).Error(); err != nil { | ||
s.logger.Printf("[ERR] consul: failed to bootstrap peers: %v", err) | ||
// Attempt a live bootstrap! | ||
s.logger.Printf("[INFO] consul: found expected number of peers, attempting to bootstrap cluster...") |
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 its useful to have it log the IPs it is attempting to bootstrap with as a sanity check
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.
Good call - I'll add them to this log message.
LGTM |
This isn't ready to merge until we do the proper vendoring but the integration part is ready for review. All unit tests are passing locally and I've booted a test cluster with this :-)