-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
[IMPROVED] Adjusted stalled producers to not be as penalizing. #6568
Conversation
a22d21f
to
191754c
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.
LGTM
From the perspective of someone not deeply familiar with NATS internals (which I imagine includes many customers), it would be helpful to include a sentence or two in the PR description explaining what is changing and why. |
@alexbozhenko linked to the kind of issues that this would fix: #5394 |
Fixed a broken test and updated warning based on @wallyqs feedback. |
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
We do have some actually, and we have lots of benchmarks that test it.. I could add more, but the msgtrace ones are pretty good. If we feel that is required, I can add it in.. |
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 hadn't looked at the message tracing tests but if you think the coverage is OK then the changes look good to me.
I will see if I can write a test.. |
Signed-off-by: Derek Collison <[email protected]>
Signed-off-by: Derek Collison <[email protected]>
Signed-off-by: Derek Collison <[email protected]>
Signed-off-by: Derek Collison <[email protected]>
8ea76c8
to
0b125e8
Compare
I added a test, which caught a minor bug, so good idea. Should be ready for final review and I will squash once approved and tests are green. |
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!
Fixes #5394 Signed-off-by: Derek Collison <[email protected]>
Includes the following: - #6574 - #6568 - #6575 - #6576 Signed-off-by: Neil Twigg <[email protected]>
Fixes #5394
Signed-off-by: Derek Collison [email protected]