search

goraft / raft

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

Command Context #142

Closed benbjohnson closed 10 years ago

benbjohnson commented 10 years ago

I spent the weekend trying to figure out some etcd issues after upgrading to the current Raft revision. One of the biggest issues was trying to access things like CommitIndex() from within a Command.Apply() function. The commands are executed within a server lock so it causes the app to hang when trying to retrieve the commit index from within the Apply() function.

The Raft version hasn't been sync'd up on etcd for a while so we should probably try to stay more in sync and hopefully these issues will pop up sooner and be easier to debug.

The change included in this PR is to pass a Context object to the Apply() function instead of the Server reference. I kept the old Apply(Server) for backwards compatibility but it's not advertised in the API anymore and it's marked as "deprecated" in the code.

The new Context object has CurrentIndex() and CurrentTerm() methods attached to it so the command can use that information without requiring a lock.

/cc: @philips @xiangli-cmu

xiang90 commented 10 years ago

@benbjohnson This is reasonable. LGTM.