Correct handling of term updates during the election for Candidate/Follower #55

dmitraver · 2015-08-02T18:21:47Z

According to the raft paper
"Current terms are exchanged whenever servers communicate; if one server’s current term is smaller than the other’s, then it updates its current term to the larger value. If a candidate or leader discovers that its term is out of date, it immediately reverts to follower state. If a server receives a request with a stale term number, it rejects the request."

This pull request implements this rules for election phase of Follower/Candidate states. It also fixes the #46 and #47 issues.

…higher term number is received from Candidate

…lection

dmitraver · 2015-08-02T18:54:20Z

After running whole tests few times I've discovered that the multi jvm tests are failing from time to time and for now I have no idea why. I need to investigate this test cases deeper and update my pull request accordingly.

colin-scott · 2015-08-02T21:50:21Z

src/main/scala/pl/project13/scala/akka/raft/Candidate.scala

-    case Event(msg: RequestVote, m: ElectionMeta) if m.canVoteIn(msg.term) =>
-      sender ! VoteCandidate(m.currentTerm)
-      stay() using m.withVoteFor(msg.term, candidate())
+		case Event(msg: RequestVote, m: ElectionMeta) if msg.term < m.currentTerm =>


Minor nit: it looks like the rest of the source code uses two spaces for indentation, whereas this pull request has tabs

Thx for pointing this out, I will fix that.

colin-scott · 2015-08-02T22:06:30Z

Random guess: I bet that the multi-jvm tests are inherently somewhat non-deterministic.

dmitraver · 2015-08-03T08:22:21Z

@colin-scott Hi, Colin. I guess you're right but it's not good anyway. The problem is that previously term updates that are handled by this PR weren't reflected in the code and this new changes can cascade and affect other parts of code that depends on it because everything is linked to some extend.

colin-scott · 2015-08-04T02:07:29Z

Just realized: I think you should also have the leader step down if term > currentTerm.

It currently stay()s:

https://github.com/ktoso/akka-raft/blob/master/src/main/scala/pl/project13/scala/akka/raft/Leader.scala#L28

colin-scott · 2015-08-04T02:28:22Z

src/main/scala/pl/project13/scala/akka/raft/protocol/StateMetadata.scala

-    def forFollower: Meta            = Meta(clusterSelf, currentTerm, config, Map.empty)
-    def forNewElection: ElectionMeta = this.forFollower.forNewElection
+    def forLeader: LeaderMeta = LeaderMeta(clusterSelf, currentTerm, config)
+    def forFollower(term: Term = currentTerm): Meta = Meta(clusterSelf, term, config, Map.empty)


Rather than having a default argument here, maybe it would be cleaner to chain the methods?

e.g.

goto(Follower) with m.forFollower.withTerm(term)

Nice catch! Also thought about that. There is a trade off between efficiency and readability and I just wanted to remove unnecessary copying of objects if its possible. Surely, I know about premature optimizations :) I think you're right there is also one more place where same "side effect" is hidden.

In this particular piece I'm ok with the default param, though the explicit version would be nice as well. No need to change just yet.

dmitraver · 2015-08-04T06:20:00Z

Can be an issue though. Leader should also have the same term update rules as candidate. Will check that later.

dmitraver · 2015-08-07T14:43:53Z

After investigating test failures for some time I've found that election tests are failing from time to time on one of three JVM nodes with the following output:

[JVM-1] [WARN] [08/07/2015 15:50:22.389] [RaftClusterSpec-akka.remote.default-remote-dispatcher-5] [akka.tcp://RaftClusterSpec@127.0.0.1:33394/system/endpointManager/reliableEndpointWriter-akka.tcp%3A%2F%2FRaftClusterSpec%40127.0.0.1%3A58897-4] Association with remote system [akka.tcp://RaftClusterSpec@127.0.0.1:58897] has failed, address is now gated for [5000] ms. Reason is: [Disassociated].
[JVM-1] [INFO] [08/07/2015 15:50:22.389] [RaftClusterSpec-akka.actor.default-dispatcher-19] [akka://RaftClusterSpec/system/transports/akkaprotocolmanager.tcp0/akkaProtocol-tcp%3A%2F%2FRaftClusterSpec%40127.0.0.1%3A51937-4] Message [akka.remote.transport.AssociationHandle$Disassociated] from Actor[akka://RaftClusterSpec/deadLetters] to Actor[akka://RaftClusterSpec/system/transports/akkaprotocolmanager.tcp0/akkaProtocol-tcp%3A%2F%2FRaftClusterSpec%40127.0.0.1%3A51937-4#-372792519] was not delivered. [4] dead letters encountered. This logging can be turned off or adjusted with configuration settings 'akka.log-dead-letters' and 'akka.log-dead-letters-during-shutdown'.
[JVM-1] [WARN] [08/07/2015 15:50:38.813] [RaftClusterSpec-akka.remote.default-remote-dispatcher-5] [Remoting] Association to [akka.tcp://RaftClusterSpec@127.0.0.1:58897] having UID [1443814610] is irrecoverably failed. UID is now quarantined and all messages to this UID will be delivered to dead letters. Remote actorsystem must be restarted to recover from this situation.

Same error messages can be observed in the failed build log of travis. Don't actually know what can cause this problem but probably it's not related to the bugs in code itself.

ktoso · 2015-08-07T14:45:43Z

Gating happens when a connection to another node fails (i.e. in multi node tests when a node is killed) as a node notices it cannot talk to the other node and "gates it" for a while, trying to re-establish the connection later on.

dmitraver · 2015-08-07T15:01:37Z

@ktoso Is there any way to prevent it from failing or its an expected behavior? I can retrigger the build and it will eventually succeed but it's not the best solution I guess.

ktoso · 2015-08-07T15:05:15Z

I'll need to look at the tests where exactly it happens, some tests could be purposefully killing nodes – then it's totally expected. Sadly now prepping and delivering a webinar so will only get to it rather later.

Thanks for all the work btw! I hope to get to reviewing soon.

colin-scott · 2015-08-08T01:25:18Z

src/main/scala/pl/project13/scala/akka/raft/Candidate.scala

+			stay()
+
+		case Event(msg: RequestVote, m: ElectionMeta) if msg.term > m.currentTerm =>
+			log.info("Received newer {}. Current term is {}. Revert to follower state.", msg.term, m.currentTerm)


perhaps you should forward the RequestVote message instead of dropping it?

e.g.

m.clusterSelf forward msg goto(Follower) using m.forFollower(msg.term)

This sounds like an optimisation (i.e. not needed for correctness), right?
If so let's skip it for now - let's aim for pure correctness in the upcoming work and PRs :)

Yeah, it's an optimization

Made it an issue so we can re-visit it later #61

ktoso · 2015-08-08T22:06:28Z

src/main/scala/pl/project13/scala/akka/raft/Candidate.scala

@@ -64,7 +91,7 @@ private[raft] trait Candidate {
      if (leaderIsAhead) {
        log.info("Reverting to Follower, because got AppendEntries from Leader in {}, but am in {}", append.term, m.currentTerm)
        m.clusterSelf forward append
-        goto(Follower) using m.forFollower
+        goto(Follower) using m.forFollower()


Minor: I actually like to use forFollower methods as it indicated the method has no side effects. Imagine it's a getter / field.
That style refers to the http://docs.scala-lang.org/glossary/#uniform-access-principle

FWIW, with a default parameter (as @dmitraver added), it doesn't compile without parens

ktoso · 2015-08-08T22:27:41Z

Changes make sense, thanks a lot @dmitraver!
I looked at the failures and would say they're timing problems due to too small await timeouts for the travis box (it's pretty slow).

I'll merge the changes and work a bit in the infra / timeouts to make tests not as fleaky.

ktoso · 2015-08-08T22:27:59Z

For the next contrib @dmitraver please use 2-spaces though... 😉

Correct handling of term updates during the election for Candidate/Follower

dmitraver · 2015-09-02T12:32:48Z

@ktoso Thx for the hint ;) Will do.

dmitraver added 2 commits August 2, 2015 20:14

Updates current term number in the Follower state during election if …

d716db6

…higher term number is received from Candidate

Correct handling of term updates for the Candidate state during the e…

d835ef8

…lection

colin-scott reviewed Aug 2, 2015
View reviewed changes

colin-scott reviewed Aug 4, 2015
View reviewed changes

colin-scott mentioned this pull request Aug 4, 2015

Nodes should not forget who they voted for #56

Open

colin-scott reviewed Aug 8, 2015
View reviewed changes

colin-scott mentioned this pull request Aug 8, 2015

Follower should be able to vote more than once in a term #29

Open

ktoso reviewed Aug 8, 2015
View reviewed changes

ktoso added a commit that referenced this pull request Aug 8, 2015

Merge pull request #55 from dmitraver/master

e656196

Correct handling of term updates during the election for Candidate/Follower

ktoso merged commit e656196 into ktoso:master Aug 8, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correct handling of term updates during the election for Candidate/Follower #55

Correct handling of term updates during the election for Candidate/Follower #55

dmitraver commented Aug 2, 2015

dmitraver commented Aug 2, 2015

colin-scott Aug 2, 2015

dmitraver Aug 3, 2015

colin-scott commented Aug 2, 2015

dmitraver commented Aug 3, 2015

colin-scott commented Aug 4, 2015

colin-scott Aug 4, 2015

dmitraver Aug 4, 2015

ktoso Aug 8, 2015

dmitraver commented Aug 4, 2015

dmitraver commented Aug 7, 2015

ktoso commented Aug 7, 2015

dmitraver commented Aug 7, 2015

ktoso commented Aug 7, 2015

colin-scott Aug 8, 2015

ktoso Aug 8, 2015

colin-scott Aug 8, 2015

ktoso Aug 8, 2015

ktoso Aug 8, 2015

colin-scott Aug 14, 2015

ktoso commented Aug 8, 2015

ktoso commented Aug 8, 2015

dmitraver commented Sep 2, 2015

Correct handling of term updates during the election for Candidate/Follower #55

Correct handling of term updates during the election for Candidate/Follower #55

Conversation

dmitraver commented Aug 2, 2015

dmitraver commented Aug 2, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colin-scott commented Aug 2, 2015

dmitraver commented Aug 3, 2015

colin-scott commented Aug 4, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitraver commented Aug 4, 2015

dmitraver commented Aug 7, 2015

ktoso commented Aug 7, 2015

dmitraver commented Aug 7, 2015

ktoso commented Aug 7, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktoso commented Aug 8, 2015

ktoso commented Aug 8, 2015

dmitraver commented Sep 2, 2015