deps: bump client-go for nextgen shared lock#5203
Conversation
ti-chi-bot
Bot
added
release-note-none
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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/S
![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 updates dependencies and sorts the tableNames slice in progress_table_writer.go by schema and table name before flushing. The review feedback suggests utilizing the more performant and type-safe slices and cmp packages introduced in Go 1.21 instead of sort.Slice, and recommends defensively handling potential nil elements in the slice to prevent nil-pointer dereference panics.
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.
| "context" | ||
| "database/sql" | ||
| "fmt" | ||
| "sort" |
There was a problem hiding this comment.
Since Go 1.21, the slices and cmp packages are preferred over sort.Slice for type-safe and reflection-free sorting. Replacing sort with slices and cmp.
| "sort" | |
| "cmp" | |
| "slices" |
| sort.Slice(tableNames, func(i, j int) bool { | ||
| if tableNames[i].SchemaName != tableNames[j].SchemaName { | ||
| return tableNames[i].SchemaName < tableNames[j].SchemaName | ||
| } | ||
| return tableNames[i].TableName < tableNames[j].TableName | ||
| }) |
There was a problem hiding this comment.
Using sort.Slice has performance overhead due to reflection. Since Go 1.21, slices.SortFunc is the idiomatic and more performant way to sort slices.
Additionally, since tableNames contains pointers (*event.SchemaTableName), we should defensively handle potential nil elements to avoid nil-pointer dereference panics.
slices.SortFunc(tableNames, func(a, b *event.SchemaTableName) int {
if a == nil && b == nil {
return 0
}
if a == nil {
return -1
}
if b == nil {
return 1
}
if a.SchemaName != b.SchemaName {
return cmp.Compare(a.SchemaName, b.SchemaName)
}
return cmp.Compare(a.TableName, b.TableName)
})
|
/test all |
cfzjywxk
force-pushed
the
release-nextgen-202603-client-go-shared-lock
branch
2 times, most recently
from
3378222 to
2469f4f
Compare
Bump client-go to the minimal release-nextgen-202603-ticdc pseudo-version that contains the API v2 lock-key re-encoding fix for shared lock support. Only the required kvproto version moves with client-go. Keep pd/client, grpc, and golang.org/x dependencies unchanged to limit the release branch diff. Signed-off-by: cfzjywxk <cfzjywxk@gmail.com>
Sort progress table names before batching because TableSchemaStore is map-backed and may return table names in different orders across runs. The progress writes are idempotent by table identity, but deterministic order keeps batch boundaries and SQL arguments reproducible for tests and debugging. Signed-off-by: cfzjywxk <cfzjywxk@gmail.com>
cfzjywxk
force-pushed
the
release-nextgen-202603-client-go-shared-lock
branch
from
2469f4f to
a518542
Compare
|
@tenfyzhong PTAL The ticdc |
### What changed Remove `^release-nextgen-\d+$` from the regular TiCDC presubmit branch set in `prow-jobs/pingcap/ticdc/latest-presubmits.yaml`. The next-gen presubmits in `prow-jobs/pingcap/ticdc/latest-presubmits-next-gen.yaml` still match `release-nextgen-*` branches. ### Why TiCDC PRs targeting `release-nextgen-202603` currently trigger regular jobs such as `pull-cdc-pulsar-integration-light` when `/test all` is used. Those regular jobs download TiDB, PD, and TiKV artifacts from the normal `hub` OCI registry, but next-gen artifacts are handled by the `*_next_gen` pipelines through the `tidbx` registry. This caused artifact lookup failures like: `us-docker.pkg.dev/pingcap-testing-account/hub/pingcap/tidb/package:release-nextgen-202603_linux_amd64: not found` Ref: pingcap/ticdc#5203 ### Verification Parsed the Prow YAML locally and checked branch/trigger selection for `release-nextgen-202603`: - Before this change, `/test all` selected the regular TiCDC integration jobs. - After this change, `/test all` selects no regular TiCDC jobs on `release-nextgen-202603`. - `/test next-gen` still selects the `*_next_gen` TiCDC integration jobs. - `/test pull-cdc-pulsar-integration-light-next-gen` still selects `pull_cdc_pulsar_integration_light_next_gen`. - `master`, `release-8.5`, and `feature/foo` still select the regular `/test all` jobs. Also ran `git diff --check` for the changed file. Signed-off-by: cfzjywxk <cfzjywxk@gmail.com>
| github.com/stretchr/testify v1.11.1 | ||
| github.com/thanhpk/randstr v1.0.6 | ||
| github.com/tikv/client-go/v2 v2.0.8-0.20251112113123-1264c1278595 | ||
| github.com/tikv/client-go/v2 v2.0.8-0.20260605035552-78dc334b882b |
There was a problem hiding this comment.
It looks like this is not the latest client-go commit on the release-nextgen-202603 branch?
There was a problem hiding this comment.
@wfxr It is desigend to not use the latest one to minialize the change risk for ticdc, the branch https://github.com/tikv/client-go/tree/release-nextgen-202603-ticdc is used
|
nextgen ci test passes |
@3AceShowHand @wk989898 PTAL |
|
@wfxr: adding LGTM is restricted to approvers and reviewers in OWNERS files. 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. |
| if len(tableNames) == 0 { | ||
| return nil | ||
| } | ||
| // TableSchemaStore stores table names in maps, so the returned order is not |
There was a problem hiding this comment.
We don't need a strict order here, so we don't need to make this change.
There was a problem hiding this comment.
It is used to make the flaky test TestProgressTableWriterFlush stable, should we keep the change to avoid the current flaky tests or to change the test case? What do you think?
Signed-off-by: cfzjywxk <cfzjywxk@gmail.com>
ti-chi-bot
Bot
added
needs-1-more-lgtm
|
/test next-gen |
|
/test all |
|
@3AceShowHand: No jobs can be run with 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 3AceShowHand, tenfyzhong, wfxr 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:
|
|
@cfzjywxk: The specified target(s) for 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. |
|
Need to run "/test next-gen" |
|
/test next-gen |
|
@cfzjywxk: 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. |
Signed-off-by: cfzjywxk <cfzjywxk@gmail.com>
ti-chi-bot
Bot
added
size/M
|
/test next-gen |
ti-chi-bot
Bot
merged commit 2966f35
into
pingcap:release-nextgen-202603
5 participants
What problem does this PR solve?
Issue Number: ref pingcap/tidb#66154
Related TiCDC compatibility issue: #5206
The TiCDC nextgen 202603 release branch still depends on an older
github.com/tikv/client-go/v2revision that does not contain the API v2 lock-key re-encoding fix needed by the TiDB shared-lock feature and the follow-up defensive API v2 key-bearing command coverage.Client-go defensive fix PR: tikv/client-go#1986
Client-go release-nextgen cherry-pick PR: tikv/client-go#1987
Client-go defensive coverage PR: tikv/client-go#1994
This PR consumes the minimal client-go branch for TiCDC:
github.com/tikv/client-go/v2 v2.0.8-0.20260609065146-a68b42e6aab4That pinned client-go version includes:
ec44861apicodec: prevent API v2 lock key re-encoding (#1986)78dc334go.mod: bump kvproto for shared lock protoa68b42eapicodec: complete API v2 key-bearing command coverageWhat is changed and how it works?
github.com/tikv/client-go/v2to the minimala68b42epseudo-version.github.com/pingcap/kvprotoonly as required by that client-go version.github.com/tikv/pd/client,google.golang.org/grpc, andgolang.org/x/*unchanged.slices.SortFuncandcmp.Compareper review feedback.Check List
Tests
Local checks run:
make tidyNEXT_GEN=1 make cdcNEXT_GEN=1 make integration_test_build_fastCGO_ENABLED=1 go test -v -timeout 300s -p 1 --race --tags=intest,nextgen ./pkg/sink/mysql -run 'TestProgressTableWriterFlush(Single|Multi)Batch' -count=5with failpoints enabledgo test ./internal/apicodecgo test --tags=nextgen ./logservice/txnutil/...git diff --checkFull nextgen unit CI should be judged by GitHub CI.
Questions
Will it cause performance regression or break compatibility?
No expected compatibility impact. The dependency change is limited to client-go and its required kvproto version for shared-lock support and defensive API v2 codec coverage. The progress-table ordering change sorts table names before batching progress-table writes, which only makes output deterministic.
Do you need to update user documentation, design documentation or monitoring documentation?
No.
Release note