Tweak sync permit return condition

[litt3]

Background

• With the health monitor, nodes under high stress have sync permits revoked if any queue is too full
• Testing has shown that nodes with many revoked permits have a duplicate event rate decrease (~30% instead of ~60%)

Problem

• During periods of stress, we should ideally reduce the duplicate event rate even more, down close to 0%
• The reason I think that we still see something like 30% duplicate rate, even for nodes that have only 1 sync permit, is that sync x completes, the permit is returned, and then sync x+1 begins before the events from sync x have been handled.

Proposal

• We could reduce the duplicate event rate for nodes under heavy stress if we simply wait to return sync permits until all events from that sync have made it through the intake pipeline.
  • The mechanism to do this already exists, since we keep track of how many events from a given peer are in the intake pipeline
• In the happy case, where there is no stress on a node, waiting to return the permit won't have any effect
  • Since there is 1 permit per peer, and we already wait to perform a sync with a given peer until all events from the previous sync are through the intake pipeline, nothing changes
  • Therefore, the change in sync return logic will only come into effect for stressed nodes, which is exactly what we want
• There is a small complication from the orphan buffer, since it is possible for events to remain in the intake pipeline for a while
  • Therefore, we would need to start keeping an additional counter (similar to the counter for number of events from a given peer in the intake pipeline), which would represent the number of events from a given peer in the orphan buffer
  • The logic for syncing another time with the same peer would remain as currently implemented (don't sync with the same peer again if events from that peer are still in the orphan buffer)
  • The logic for returning sync permits during times of stress would consider number of events from the peer in the orphan buffer (i.e., the permit would be released when events_from_peer_in_intake - events_from_peer_in_orphan_buffer == 0

[lpetrovic05]

One issue I see is that if our permit count is 0, and we wait for all events to pass through the pipeline, we create a gap where we are getting new events. Between the time all events pass through, and we start getting new ones, we have to wait for a few round trips of sync to go by, and this is wasted time I think.

[litt3]

One issue I see is that if our permit count is 0, and we wait for all events to pass through the pipeline, we create a gap where we are getting new events. Between the time all events pass through, and we start getting new ones, we have to wait for a few round trips of sync to go by, and this is wasted time I think.

With the current bottleneck being transaction handling (by a huge margin), I don't think there would be any wasted time. The counters only keep track of events in the intake pipeline, and don't impose any limitations on round handling. The only way that time would be wasted is if the delay you mention causes the transaction handler to operate at less than 100% utilization (which we'd certainly want to keep an eye on while evaluating performance of this change).