config: make minSyncPointInterval configurable via debug config#5271
config: make minSyncPointInterval configurable via debug config#5271pingyu wants to merge 5 commits into
Conversation
Signed-off-by: Ping Yu <yuping@pingcap.com>
Signed-off-by: Ping Yu <yuping@pingcap.com>
ti-chi-bot
Bot
added
release-note-none
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a debug-configurable minimum sync-point interval, splits replica validation for CLI usage, exposes --sync-point/--sync-interval CLI flags wired into replica config completion, and updates tests and integration scripts to use the new settings. ChangesConfigurable Sync Point Interval
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
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 |
ti-chi-bot
Bot
added
the
size/L
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces --sync-point and --sync-interval CLI flags for creating changefeeds, allowing users to configure sync points directly from the command line. It also adds a new debug configuration option min-sync-point-interval to allow overriding the minimum sync point interval (defaulting to 30s, but configurable down to 5s for testing/debugging). Feedback suggests automatically enabling the sync point feature if a non-zero sync interval is provided to prevent silent misconfigurations.
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.
Signed-off-by: Ping Yu <yuping@pingcap.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
ti-chi-bot
Bot
added
release-note
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@tests/integration_tests/syncpoint/run.sh`:
- Line 162: The run_cdc_server invocation uses unquoted variable expansions
which can cause word-splitting/globbing; update the command that calls
run_cdc_server to quote all expanded variables (e.g., change run_cdc_server
--workdir $WORK_DIR --binary $CDC_BINARY --config $CUR/conf/server.toml to use
"$WORK_DIR", "$CDC_BINARY", and "$CUR/conf/server.toml") and apply the same
quoting fix to the other run_cdc_server invocation referenced in the same
script.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b12c8655-6c62-422e-82a0-9382aac8a9c2
📒 Files selected for processing (10)
cmd/cdc/cli/cli_changefeed_create.gocmd/cdc/cli/cli_changefeed_create_test.gopkg/config/debug.gopkg/config/replica_config.gopkg/config/replica_config_test.gopkg/config/server.gopkg/config/server_config_test.gotests/integration_tests/syncpoint/conf/changefeed.tomltests/integration_tests/syncpoint/conf/server.tomltests/integration_tests/syncpoint/run.sh
💤 Files with no reviewable changes (1)
- tests/integration_tests/syncpoint/conf/changefeed.toml
| export GO_FAILPOINTS='github.com/pingcap/ticdc/utils/dynstream/InjectDeadlock=10%return(true)' | ||
| start_ts=$(run_cdc_cli_tso_query ${UP_PD_HOST_1} ${UP_PD_PORT_1}) | ||
| run_cdc_server --workdir $WORK_DIR --binary $CDC_BINARY | ||
| run_cdc_server --workdir $WORK_DIR --binary $CDC_BINARY --config $CUR/conf/server.toml |
There was a problem hiding this comment.
Quote variable expansions in command arguments to avoid word-splitting edge cases.
Use quotes for expanded shell vars in these commands to prevent accidental splitting/globbing in CI paths or env-specific workdirs.
Suggested patch
-run_cdc_server --workdir $WORK_DIR --binary $CDC_BINARY --config $CUR/conf/server.toml
+run_cdc_server --workdir "$WORK_DIR" --binary "$CDC_BINARY" --config "$CUR/conf/server.toml"
-cdc_cli_changefeed create --start-ts=$start_ts --sink-uri="$SINK_URI" --sync-point --sync-interval 10s
+cdc_cli_changefeed create --start-ts="$start_ts" --sink-uri="$SINK_URI" --sync-point --sync-interval "10s"Also applies to: 167-167
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 162-162: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 162-162: Double quote to prevent globbing and word splitting.
(SC2086)
🤖 Prompt for 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.
In `@tests/integration_tests/syncpoint/run.sh` at line 162, The run_cdc_server
invocation uses unquoted variable expansions which can cause
word-splitting/globbing; update the command that calls run_cdc_server to quote
all expanded variables (e.g., change run_cdc_server --workdir $WORK_DIR --binary
$CDC_BINARY --config $CUR/conf/server.toml to use "$WORK_DIR", "$CDC_BINARY",
and "$CUR/conf/server.toml") and apply the same quoting fix to the other
run_cdc_server invocation referenced in the same script.
Source: Linters/SAST tools
…nto debug-min-sync-point-interval Signed-off-by: Ping Yu <yuping@pingcap.com>
|
/cc @tenfyzhong |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: tenfyzhong 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 |
[LGTM Timeline notifier]Timeline:
|
|
/cc @lidezhu |
|
/check-issue-triage-complete |
|
I think adding this configuration just for testing is not a good idea—it introduces unnecessary complexity. You might as well just hack the value directly. |
3 participants
What problem does this PR solve?
Issue Number: close #5270
What is changed and how it works?
Make
minSyncPointIntervalconfigurable through the existingDebugConfiginpkg/config/debug.go, instead of being hardcoded to 30 seconds.Changes:
pkg/config/debug.go: AddDebugReplicaConfigwithMinSyncPointIntervalfield underdebug.replica-config. The configurable range is [5s, 30s]. AddgetMinSyncPointInterval()to read the value from global server config with fallback to 30s default.pkg/config/replica_config.go: Replace hardcodedminSyncPointIntervalconstant withminSyncPointIntervalDefault(30s) andminConfigurableSyncPointInterval(5s). SplitValidateAndAdjustintoValidateAndAdjust(server-side, uses debug config minimum) andValidateAndAdjustForCLI(client-side, uses absolute minimum 5s).pkg/config/server.go: AddReplicaConfig: NewDefaultDebugReplicaConfig()to default server config.cmd/cdc/cli/cli_changefeed_create.go: Add--sync-pointand--sync-intervalCLI flags. UseValidateAndAdjustForCLIfor CLI-side validation.tests/integration_tests/syncpoint/: Replace changefeed.toml (which set sync-point-interval=30s) with server.toml debug config (min-sync-point-interval=5s), and use--sync-point --sync-interval 10sflags instead.This allows integration tests to set a smaller minimum (e.g., 5s) via the debug config, significantly reducing test execution time while keeping the 30s safety guard for production.
Check List
Tests
Questions
Will it cause performance regression or break compatibility?
No. The default minimum remains 30s. Only when explicitly configured via debug config (unexposed/internal) does the minimum change.
Do you need to update user documentation, design documentation or monitoring documentation?
No. This is purely a debug/internal configuration with no user-facing changes.
Release note
Summary by CodeRabbit
New Features
Validation
Tests