eventbroker: fix broker quota release and message validation#5643
eventbroker: fix broker quota release and message validation#5643lidezhu wants to merge 2 commits into
Conversation
|
Skipping CI for Draft Pull Request. |
📝 WalkthroughWalkthroughEventservice scan quota cleanup now covers early exits and scan failures, table-trigger dispatcher metrics remain balanced, and malformed heartbeat or congestion-control messages are ignored safely after logging. ChangesEventservice corrections
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Code Review
This pull request introduces fixes to the event service, including releasing memory quota when skipping scans due to insufficient quota, tracking table trigger dispatchers in metrics, and handling invalid message payloads by returning early. Corresponding unit tests have been added. The review feedback points out a potential quota leak if scanning fails later in the process, and warns about unsafe type assertions on message payloads that could lead to panics, suggesting safer 'comma-ok' type assertions instead.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| } | ||
|
|
||
| if uint64(sl.maxDMLBytes) > task.availableMemoryQuota.Load() { | ||
| releaseQuota(available, uint64(sl.maxDMLBytes)) |
There was a problem hiding this comment.
While releasing the allocated quota here is necessary, a similar quota leak can occur if scanner.scan fails further down in this function (around line 728). When scanner.scan returns an error, the remaining scannedBytes of the allocated quota is not fully released, which can lead to temporary quota exhaustion until the next congestion control update.
Consider fully releasing the remaining quota in the error handling block of scanner.scan as well:
if err != nil {
if scannedBytes > 0 {
releaseQuota(available, uint64(min(sl.maxDMLBytes, scannedBytes)))
}
log.Error("scan events failed", ...)
return
}|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: asddongmen, hongyunyan The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[LGTM Timeline notifier]Timeline:
|
|
@lidezhu: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
What problem does this PR solve?
Issue Number: close #5642
What is changed and how it works?
Check List
Tests
Questions
Will it cause performance regression or break compatibility?
Do you need to update user documentation, design documentation or monitoring documentation?
Release note
Summary by CodeRabbit
Bug Fixes
Tests