-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Increase health check channel buffer #17821
Increase health check channel buffer #17821
Conversation
…er of messages dropped Signed-off-by: Manan Gupta <[email protected]>
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17821 +/- ##
==========================================
- Coverage 67.44% 67.43% -0.01%
==========================================
Files 1592 1592
Lines 258076 258183 +107
==========================================
+ Hits 174051 174117 +66
- Misses 84025 84066 +41 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Manan Gupta <[email protected]>
return totalUpdates == int(totalCount.Load()) | ||
}, 5*time.Second, 100*time.Millisecond, "expected all updates to be processed") | ||
} | ||
|
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.
Is it worth adding another test like this?
// TestBroadcastChannelFull tests that some concurrent broadcasts from the healthcheck to its subscribers may be dropped.
func TestBroadcastChannelFull(t *testing.T) {
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 can add one, but it doesn't seem that useful to me
Signed-off-by: Manan Gupta <[email protected]>
Signed-off-by: Manan Gupta <[email protected]>
Description
This PR fixes the problem described in #17629. As pointed out in the issue, the problem is that the consumers aren't fast enough to ingest all the health check updates and that causes them to miss some of them.
This PR just increases the buffer size from 2 to 2048, so that the messages aren't dropped.
Related Issue(s)
Checklist
Deployment Notes