client/v3: warn when watcher stream buffer is backlogged#21704
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: xigang The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @xigang. Thanks for your PR. I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/cc @richabanker |
|
@serathius: GitHub didn't allow me to request PR reviews from the following users: richabanker. Note that only etcd-io members and repo collaborators can review this PR, and authors cannot review their own PRs. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
This looks similar to K8s logic for detecting buffer growing. Let's take learning from it kubernetes/kubernetes#138126 |
|
/cc @mborsz |
|
@serathius: GitHub didn't allow me to request PR reviews from the following users: mborsz. Note that only etcd-io members and repo collaborators can review this PR, and authors cannot review their own PRs. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/ok-to-test |
Codecov Report❌ Patch coverage is
Additional details and impacted files
... and 19 files with indirect coverage changes @@ Coverage Diff @@
## main #21704 +/- ##
==========================================
+ Coverage 70.22% 70.35% +0.12%
==========================================
Files 426 427 +1
Lines 35211 35261 +50
==========================================
+ Hits 24727 24807 +80
+ Misses 9090 9059 -31
- Partials 1394 1395 +1 Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
@richabanker PTAL. Thanks. |
| zap.String("range-end", ws.initReq.end), | ||
| zap.Int("buffered-responses", bufLen), | ||
| ) | ||
| ws.nextBufWarnLen *= 2 |
There was a problem hiding this comment.
So this would only log when the buffer hits 1024, 2048, 4096, etc. Which is great for detecting growth of the buffer, but if the consumer is slow for an extended period of time and say the buffer hovers at 1500 for an hour, it would only log once at the very beginning?
What about doing some time interval based logging instead of this size based logging (perhaps thats what @serathius meant by this comment) - basically log at most once per interval (say every 1 minute) as long as the buffer is above the threshold (keep threshold static) ?
There was a problem hiding this comment.
Thank you. The current implementation has this issue.
I checked the PR @serathius mentioned, and I’ll adjust this to use time-based logging with a static threshold instead.
There was a problem hiding this comment.
Thanks, that makes sense. I changed this to time-based rate-limited logging, so it logs at most once per interval while buffered delivery is blocked.
xigang
force-pushed
the
watchstream_buf
branch
2 times, most recently
from
38bc8e0 to
768cfb5
Compare
Signed-off-by: xigang <[email protected]>
|
/retest |
|
@mborsz could you take a look if this implementation matches same requirements as you proposed in kubernetes/kubernetes#138126 ? |
4 participants
Fixes: kubernetes/kubernetes#138217