eventservice: fix broker quota release and message validation#5643
eventservice: fix broker quota release and message validation#5643lidezhu wants to merge 2 commits into
Conversation
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 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
}
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