Reprioritize responses of GetReplicationMessagesResponse in frontend #6696
Conversation
arzonus
requested review from
Shaddoll,
neil-xie,
davidporter-id-au,
Groxx,
shijiesheng,
jakobht,
3vilhamster,
sankari165,
dkrotx,
taylanisikdemir and
demirkayaender
as code owners
| // if earliestCreationTime is equal, it will compare the size of the response | ||
| func cmpGetReplicationMessagesWithSize(a, b *getReplicationMessagesWithSize) int { | ||
| // a > b | ||
| if a == nil || a.earliestCreationTime == nil { |
There was a problem hiding this comment.
what's the case when earliestCreationTime is nil?
If we often have this case, then shouldn't we sort randomly instead?
There was a problem hiding this comment.
Good question! As I understand from the code, CreationTime is initially non-pointer, but internally it changed to pointer.
There was a problem hiding this comment.
So, in general, we do not expect nil-s and its more like precaution?
There was a problem hiding this comment.
Yes, we don't expect nil-s there, but want to be sure that a NPE will never happen
| if v == nil { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
This is very unusual to check for reciver-ptr == nil. Is there an expected code-path which continues if * GetReplicationMessagesResponse is nil and operates with it?
There was a problem hiding this comment.
All other methods of the structure do the same check. I assume that the structure can be nil in this case.
There was a problem hiding this comment.
Got it. Makes total sense for consistency.
common/types/replicator.go
Outdated
| if earliestTime == nil { | ||
| return nil | ||
| } | ||
|
|
||
| result := *earliestTime | ||
| return &result |
There was a problem hiding this comment.
why not just return earliestTime ?
There was a problem hiding this comment.
It's not mandatory, however it's a protection of the variable (that got non directly) from being changed wrongly.
arzonus
changed the title
| // if earliestCreationTime is equal, it will compare the size of the response | ||
| func cmpGetReplicationMessagesWithSize(a, b *getReplicationMessagesWithSize) int { | ||
| // a > b | ||
| if a == nil || a.earliestCreationTime == nil { |
There was a problem hiding this comment.
So, in general, we do not expect nil-s and its more like precaution?
| if v == nil { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Got it. Makes total sense for consistency.
What changed?
In case of a shrunk response to the
GetReplicationMessagescall, the frontend service will give priority to responses from peers with the older creation time of replication tasks in responses from history services.Why?
A replication request comes from a passive side to an active side. A request aggregates requests over multiple shardIDs. Frontend service on the active side calls GetReplicationMessages per each peer (a peer contains multiple shards) to history services. The result is aggregated into one response and sent back to the passive side.
In some cases the result response may exceed max payload size (4MB by default). In this case, frontend service is used to choose randomly some responses. A replication lag could be caused if some child responses took the whole response, but could not be selected for response due to randomized sorting of peer response.
The PR changes the sorting logic and use sorting by CreationTime of replications tasks. Instead of randomized order, frontend will put responses with the older creation time of replication tasks first to the final response, and put with newer time later. It should prevent replication lag for shards which replication tasks are quite big.
The PR will allow to avoidance of manual actions from an on-call engineer to re-run the replication for stuck shards.
How did you test it?
Potential risks
Potentially other shards without big replication tasks may experience some bigger delays due to giving prioritization to older shards.
Release notes
Documentation Changes