-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
NRG: Drop append entries when upper layer is overloaded #4735
base: main
Are you sure you want to change the base?
Conversation
d907c97
to
d361851
Compare
d361851
to
c538f38
Compare
server/raft.go
Outdated
|
||
// Updates the overloaded state. Lock must be held. | ||
func (n *raft) updateOverloadState() { | ||
n.overload.Store(n.apply.len() >= overloadThreshold || n.commit-n.applied >= uint64(overloadThreshold)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More about memory then number of elements I think, WDYT?
|
||
// Pushes to the apply queue and updates the overloaded state. Lock must be held. | ||
func (n *raft) pushToApply(ce *CommittedEntry) { | ||
n.apply.push(ce) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this function returns pending, so could short circuit some work here.
// If we are overwhelmed, i.e. the upper layer is not applying entries | ||
// fast enough and our apply queue is building up, start to drop new | ||
// append entries instead. | ||
if n.Overloaded() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just check n.apply.len()? Always present, so do not need to lock the raft group, and ipq underneath has its own which we use here anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overloaded()
doesn't take a lock but now just checks the apply queue size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It takes the ipq lock, so we are now taking it twice in that function I think.
c538f38
to
358df16
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the ipq size tracking effect performance?
// If we are overwhelmed, i.e. the upper layer is not applying entries | ||
// fast enough and our apply queue is building up, start to drop new | ||
// append entries instead. | ||
if n.Overloaded() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It takes the ipq lock, so we are now taking it twice in that function I think.
Haven't benched it yet but I will do so. |
1817cd4
to
19181b5
Compare
Signed-off-by: Neil Twigg <[email protected]>
19181b5
to
9953f1b
Compare
This adds a mechanism for the Raft layer to detect when the upper layer isn't keeping up with applies. When the overloaded condition is detected, we'll start dropping newly incoming append entries instead so that the apply queue doesn't build up endlessly.
Still need to figure out what threshold makes sense here, or how it makes sense to configure it.
Signed-off-by: Neil Twigg [email protected]