maxUncommittedLogSize not enforced across term changes

[tbg]

See etcd-io/etcd#14627 (comment).

In becomeLeader, we call r.reset() which resets the uncommitted log size. This means that in theory the uncommitted log size isn't bounded:

• 5x replication
• peer 1 becomes leader (committed = 1)
• peer 1 amasses uncommitted log up to the limit, only replicates it to peer 2 but not 3-5 (so no quorum)
• peer 1 loses leadership
• peer 2 becomes leader (committed = 1)
• peer 2 amasses more log entries
• peer 1 becomes leader
• ...

We could either

• live with that, but uncommitted log is amassed precisely in situations where leadership is unstable, so the counter-example could happen in practice (esp. under a workload that proposes large log entries)
• not reset the uncommitted log size across term changes
• scan the uncommitted log when becoming leader to obtain its size (can be expensive and has potential to further destabilize the system).

[XiongKezhi]

What would be the downside of not resetting it across term changes, since the new leader will not remove uncommitted logs anyway?

Or c an we simply reset it to the value of (r.raftLog.lastIndex() - r.raftLog.committed) * some-estimated-size-of-an-entry to mitigate the problem?

[AngstyDuck]

Hello, there are 3 possible ways that were raised above to resolve this:

Not reset the uncommitted log size across term changes

• I worry that this might be tricky because there is no assurance that a leader and the next leader would share identical amounts of uncommitted logs. Any deviation may snowball across terms and it may result in increasingly inaccurate number of uncommitted logs down the line.

Scan the uncommitted log when becoming leader to obtain its size

• This is understandably complicated especially with possibly asynchronous removal of logs via async storage writes.(correct me if I'm wrong here)

Resetting the uncommitted log size to a rough estimate of the actual value

• I think this solution has the most potential, though its not without challenges.
• Entry sizes may vary. If we overestimate the size of an entry, new leaders may drop more entries than expected to conform to the existing config.MaxUncommittedEntriesSize.
• However if we underestimate the size of an entry, the node may theoretically exceed config.MaxUncommittedEntriesSize storing the uncommitted entries. This however is what we may already be doing by resetting the uncommitted log size anyway, and we still get to resolve the issue of potentially unbounded actual uncommitted log sizes.

I'd suggest the third option, while trying to possibly underestimate the size of the array. What do you think? If there are no additional issues, I probably can take this up.

[yashkundu]

Hi, @tbg, @jmhbnz , I would like to pick this up, can you please assign this to me?

[yashkundu]

We can maintain the uncommitted log size in every follower like it is maintained in the leader now, whenever the follower gets MsgAppend from leader, it increases the uncommitted log size, and whenever it is informed about the new committed entry, it can reduce uncommitted log size. So, whenever a follower transitions to a leader we will have the accurate uncommitted log size value, this won't be computationally expensive, and will be able to solve this issue deterministically.
@tbg , @jmhbnz, @ahrtr what do you guys think about it?

[ahrtr]

cc @nvanbenschoten