-
-
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
Changes to prevent fan-in scenarios from slow consumer state #876
Conversation
Signed-off-by: Derek Collison <derek@nats.io>
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.
Do we need lws
?
Otherwise, LGTM and love the simplicity of this and it does indeed solve the issue on N to 1 bench run.
server/client.go
Outdated
wdl time.Duration // Snapshot fo write deadline. | ||
mp int64 // snapshot of max pending. | ||
fsp int // Flush signals that are pending from readLoop's pcd. | ||
lws int64 // Last write 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.
Not sure if this was during dev because I see it being set but not used anywhere?
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 was using it to determine releaseLock state but got more consistent results with what is there. I can remove.
// Do NOT hold lock during actual IO unless we are behind | ||
if releaseLock { | ||
c.mu.Unlock() | ||
} |
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.
Can't reproduce the slow consumer issue with this change now (tried with nats-bench -np 40 -ns 1 -n 100000000 -ms 1024 foo
)
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.
Yes I was using the new fan in tests but glad you tried this one too. Thanks!
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.
LGTM, this improves a lot slow consumer issues and memory usage of the server.
Signed-off-by: Derek Collison <derek@nats.io>
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.
LGTM
Signed-off-by: Derek Collison <derek@nats.io>
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.
LGTM
This tries to balance the new decoupled producer consumer architecture for fanout with the ability to utilized coupled feedback in fan in scenarios where slow consumers where becoming the norm.
Signed-off-by: Derek Collison derek@nats.io
/cc @nats-io/core