search

upsight / dinghy

Dinghy implements leader election using the raft protocol
MIT License
15 stars 4 forks source link

OnFollower and OnLeader errors ignored? #6

Closed ryanschneider closed 6 years ago

ryanschneider commented 6 years ago

It looks like the errors returned by OnFollower and OnLeader are just logged, is that the intended behavior?

I feel like if OnLeader returns an error, the node should StepDown, not sure what a OnFollower error should do though.

If you agree w/ that change, for symmetry how do you feel about adding an OnCandidate that if it returns a non-nil error, just goes back to being a Follower instead of trying to initiate an election for itself?

pkar commented 6 years ago

Yes it's intended. You could implement the Logger interface error to get custom behavior like sending to error reporting services https://github.com/upsight/dinghy/blob/master/log.go#L9

An alternative is have custom code in OnLeader that deals with forced stepping down or anything else that might be needed.

OnCandidate seems more like internal bookeeping so didn't get any customization. It certainly could be added if there is a use case, it just never came up for us internally.

noonat commented 6 years ago

@pkar and I were talking about this some more. The current intent was for OnLeader to be just an event callback to notify that the instance has become a leader, so just logging the returned error made sense from that perspective.

But it's also true that OnLeader is currently the only way to extend dinghy with application specific logic, so it might make sense to instead treat an error as an indication that it failed to become a leader. In that case, stepping down would make sense.

We're going to look into it more for our internal use cases.

ryanschneider commented 6 years ago

Cool thanks. Also hey @noonat, I just realized we worked together in the past, and that this is an Upsight.com repo. :)

noonat commented 6 years ago

@ryanschneider Haha, small world, I didn't realize this was the same Ryan Schneider!

ryanschneider commented 6 years ago

Awesome, looks very similar to how I was doing it locally before I got pulled into other stuff. Glad to see the changes in the example main.go as well, I was going to mention that as well (I made more or less the same edits in my test app).

pkar commented 6 years ago

Thanks for reporting, hope it works out!