search

goraft / raft

UNMAINTAINED: A Go implementation of the Raft distributed consensus protocol.
MIT License
2.43k stars 480 forks source link

fix(*.go): rename HeartbeatTimeout to HeartbeatInterval #166

Closed philips closed 10 years ago

philips commented 10 years ago

While re-reading the etcd tuning guide I realized that perhaps it is a bit of a misnomer to call this a "Timeout" since unlike the "Election Timeout" there is no immediate consequence of it being missed.

Perhaps naming it interval would be better?

xiang90 commented 10 years ago

@philips I think interval is better.

bcwaldon commented 10 years ago

+1

philips commented 10 years ago

@bcwaldon fixed.

philips commented 10 years ago

go fmt'd sorry.

bcwaldon commented 10 years ago

lgtm - there were a few unrelated fmt changes added to the PR, but I'm cool with it

philips commented 10 years ago

/cc @benbjohnson What do you think about this change?

benbjohnson commented 10 years ago

@philips I agree that "interval" makes more sense than "timeout". I feel like I used timeout originally because it was in the original Raft paper. I can't find a copy of the original anymore though.

The biggest issue that I have is that it breaks the API and therefore other user's integration. go-raft isn't at v1.0.0 yet so the API isn't set in stone but I'd like to hear from other users before merging.

@bketelsen @erikstmartin @pauldix @jvshahid What do you guys think? Is it a problem for you if we make this aesthetic change?

bketelsen commented 10 years ago

Fine with me!

Sent from my iPhone

On Jan 23, 2014, at 4:36 PM, Ben Johnson notifications@github.com wrote:

@philips I agree that "interval" makes more sense than "timeout". I feel like I used timeout originally because it was in the original Raft paper. I can't find a copy of the original anymore though.

The biggest issue that I have is that it breaks the API and therefore other user's integration. go-raft isn't at v1.0.0 yet so the API isn't set in stone but I'd like to hear from other users before merging.

@bketelsen @erikstmartin @pauldix @jvshahid What do you guys think? Is it a problem for you if we make this aesthetic change?

— Reply to this email directly or view it on GitHub.

jvshahid commented 10 years ago

Fine with me too

erikstmartin commented 10 years ago

+1

philips commented 10 years ago

Looks like we have quorum. ;)

The only other thing to consider is if we will need to break the API again for the event channel stuff.

benbjohnson commented 10 years ago

Thanks everybody for chiming in.

@philips What's the API change for the event channel?