syncer: scale history buffer with visible memory#10696
Conversation
|
[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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe syncer now sizes its history buffer from visible memory (clamped between 10000 and 100000 slots), uses that size when creating history buffers, centralizes history index metric updates and adds a historyBufferSize metric, updates Grafana panels, and adds unit tests for sizing. ChangesDynamic History Buffer Sizing and Metrics
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Signed-off-by: okjiang <819421878@qq.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/syncer/server_test.go`:
- Around line 54-56: The subtests use the outer-scope require (re) which calls
FailNow on the outer *testing.T and aborts the entire table when one case fails;
update the subtest to accept its own *testing.T (change func(_ *testing.T) to
func(t *testing.T)) and use a subtest-local require (e.g., re := require.New(t))
or call require.Equal(t, ...) directly so each subtest (testing
historyBufferSizeFromMemory) fails independently and doesn't stop other cases
from running.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Signed-off-by: okjiang <819421878@qq.com>
Signed-off-by: okjiang <819421878@qq.com>
|
Checked the I pushed 1a70cf8 for the new dashboard review comments, so CI should rerun on the latest commit. |
|
Checked the latest Requesting a rerun for the required failed checks. /retest-required |
|
Checked the latest required test failures on 1a70cf8.
Both failures are outside the files touched by this PR ( /retest-required |
|
Checked the latest This does not require a code change in this PR: the PR only changes syncer history buffer sizing/metrics and the Grafana panel, and the same two narrow router tests passed locally with failpoints enabled:
Requesting a rerun for the required failed check. /retest-required |
|
Checked the latest
These failures are outside this PR changes (
Requesting another rerun for the required failed check. /retest-required |
Signed-off-by: okjiang <819421878@qq.com>
| "targets": [ | ||
| { | ||
| "expr": "pd_region_syncer_status{k8s_cluster=\"$k8s_cluster\", tidb_cluster=\"$tidb_cluster\", type=\"sync_index\"}", | ||
| "expr": "pd_region_syncer_status{k8s_cluster=\"$k8s_cluster\", tidb_cluster=\"$tidb_cluster\", job=~\".*pd.*\", type=\"sync_index\"}", |
There was a problem hiding this comment.
Should we just delete the Sync index and History index directly? They don't seem to have much of a reason to exist.
What do you think? @rleungx
|
@okJiang: The following test 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. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #10696 +/- ##
==========================================
+ Coverage 79.03% 79.07% +0.04%
==========================================
Files 536 536
Lines 73103 73225 +122
==========================================
+ Hits 57777 57904 +127
+ Misses 11234 11224 -10
- Partials 4092 4097 +5
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
What problem does this PR solve?
Issue Number: Close #10692, ref #10666, ref #10668
What is changed and how does it work?
Check List
Release note
Summary by CodeRabbit
Improvements
Observability
Tests