More comments and logging fixes. #2362
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2362 +/- ##
==========================================
- Coverage 60.49% 60.28% -0.22%
==========================================
Files 128 128
Lines 26275 26277 +2
==========================================
- Hits 15895 15841 -54
- Misses 8976 9035 +59
+ Partials 1404 1401 -3 |
manager/state/raft/raft.go
Outdated
| @@ -542,6 +542,9 @@ func (n *Node) Run(ctx context.Context) error { | |||
| n.done() | |||
| }() | |||
|
|
|||
| // Flag to indicate if this manager node was the raft leader in the previous iteration of the loop. | |||
| // It is mainly used to cancel out proposals that this node might have made when it was the leader | |||
| // in the previous iteration. It not only needed to avoid deadlocking processCommitted(). | |||
There was a problem hiding this comment.
I don't quite understand the last sentence - seems like it's missing a word.
There was a problem hiding this comment.
I can't understand it either. Don't remember what I was trying to say. I'll fix/remove it.
manager/state/raft/raft.go
Outdated
| @@ -630,6 +633,8 @@ func (n *Node) Run(ctx context.Context) error { | |||
| // cancelAll, or by its own check of signalledLeadership. | |||
| n.wait.cancelAll() | |||
| } else if !wasLeader && rd.SoftState.RaftState == raft.StateLeader { | |||
| // If this node was the leader in the previous iteration, | |||
| // set wasLeader to make sure all outstanding proposals are cancelled. | |||
There was a problem hiding this comment.
This case is actually when the node becomes the leader.
!wasLeader - wasn't the leader before
rd.SoftState.RaftState == raft.StateLeader - is the leader now
There was a problem hiding this comment.
What is the meaning of the variable 'wasLeader' ? Would 'isLeader' be more appropriate ? Currently this is confusing because 'wasLeader', to me, sound like the node was the leader in the past, and from your explanation it sounds like that's not what it means. @aaronlehmann
There was a problem hiding this comment.
The name might be confusing. I think renaming it to isLeader would be okay. The idea was that it's a long-lived state variable, not an instantaneous value.
I think the reason I named it that way was that if it as isLeader, this line would be } else if !isLeader && rd.SoftState.RaftState == raft.StateLeader {, which seems kind of nonsensical - we're the leader and we're not the leader. wasLeader tried to convey that it was the past state.
manager/state/raft/raft.go
Outdated
| // processInternalRaftRequest proposes a value to be appended to the raft log. | ||
| // It calls Propose() on etcd/raft, which calls back into the raft FSM, | ||
| // which then sends a message to each of the participating nodes | ||
| // in the raft groupp to apply a log entry and then waits for it to be applied |
manager/state/raft/raft.go
Outdated
| @@ -630,6 +633,8 @@ func (n *Node) Run(ctx context.Context) error { | |||
| // cancelAll, or by its own check of signalledLeadership. | |||
| n.wait.cancelAll() | |||
| } else if !wasLeader && rd.SoftState.RaftState == raft.StateLeader { | |||
| // If this node was the leader in the previous iteration, | |||
| // set wasLeader to make sure all outstanding proposals are cancelled. | |||
| wasLeader = true | |||
There was a problem hiding this comment.
Why do we let the FSM proceed here and not cancel the outstanding proposals right away here ? @aaronlehmann
manager/state/raft/raft.go
Outdated
| @@ -542,6 +542,9 @@ func (n *Node) Run(ctx context.Context) error { | |||
| n.done() | |||
| }() | |||
|
|
|||
| // Flag to indicate if this manager node was the raft leader in the previous iteration of the loop. | |||
| // It is mainly used to cancel out proposals that this node might have made when it was the leader | |||
| // in the previous iteration. It not only needed to avoid deadlocking processCommitted(). | |||
There was a problem hiding this comment.
I can't understand it either. Don't remember what I was trying to say. I'll fix/remove it.
manager/state/raft/raft.go
Outdated
| @@ -630,6 +633,8 @@ func (n *Node) Run(ctx context.Context) error { | |||
| // cancelAll, or by its own check of signalledLeadership. | |||
| n.wait.cancelAll() | |||
| } else if !wasLeader && rd.SoftState.RaftState == raft.StateLeader { | |||
| // If this node was the leader in the previous iteration, | |||
| // set wasLeader to make sure all outstanding proposals are cancelled. | |||
There was a problem hiding this comment.
What is the meaning of the variable 'wasLeader' ? Would 'isLeader' be more appropriate ? Currently this is confusing because 'wasLeader', to me, sound like the node was the leader in the past, and from your explanation it sounds like that's not what it means. @aaronlehmann
anshulpundir
force-pushed
the
commentary
branch
from
bddf91e to
fd751f5
Compare
Signed-off-by: Anshul Pundir <[email protected]>
anshulpundir
force-pushed
the
commentary
branch
from
fd751f5 to
5a7c9bb
Compare
anshulpundir
changed the title
| @@ -1681,7 +1689,7 @@ func (n *Node) processInternalRaftRequest(ctx context.Context, r *api.InternalRa | |||
|
|
|||
| // Do this check after calling register to avoid a race. | |||
| if atomic.LoadUint32(&n.signalledLeadership) != 1 { | |||
| log.G(ctx).Errorf("node %x is no longer leader, aborting propose", n.opts.ID) | |||
| log.G(ctx).Error("node is no longer leader, aborting propose") | |||
There was a problem hiding this comment.
@anshulpundir why remove node IDs from these log messages?
There was a problem hiding this comment.
node id is a field already added as a field to the contest logger https://github.com/docker/swarmkit/blob/master/node/node.go#L286 @nishanttotla
Signed-off-by: Anshul Pundir <[email protected]>
Signed-off-by: Anshul Pundir [email protected]