Modernize cdc#12720
Conversation
ti-chi-bot
Bot
added
the
release-note-none
|
[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 |
ti-chi-bot
Bot
added
the
size/XXL
![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 refactors and modernizes the codebase by replacing interface{} with any, adopting for range loops for index-only iterations, utilizing Go 1.21+ slices and built-in min/max functions, and simplifying goroutine spawning in tests with wg.Go. The review feedback recommends using fmt.Fprintf with strings.Builder in cdc/model/owner.go to avoid allocations and prevent nil pointer dereferences, simplifying the allowed check in cdc/api/middleware/middleware.go with slices.Contains, and adding a defensive check in placeHolder within cdc/sink/dmlsink/txn/mysql/dml.go to prevent a runtime panic when n <= 0.
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.
| for id, sinfo := range p { | ||
| s += fmt.Sprintf("%s: %+v,", id, *sinfo) | ||
| s.WriteString(fmt.Sprintf("%s: %+v,", id, *sinfo)) | ||
| } |
There was a problem hiding this comment.
Using fmt.Fprintf directly with the strings.Builder avoids the intermediate string allocation caused by fmt.Sprintf. Additionally, we should defensively check if sinfo is nil before dereferencing it to prevent potential runtime panics.
| for id, sinfo := range p { | |
| s += fmt.Sprintf("%s: %+v,", id, *sinfo) | |
| s.WriteString(fmt.Sprintf("%s: %+v,", id, *sinfo)) | |
| } | |
| for id, sinfo := range p { | |
| if sinfo == nil { | |
| fmt.Fprintf(&s, "%s: nil,", id) | |
| } else { | |
| fmt.Fprintf(&s, "%s: %+v,", id, *sinfo) | |
| } | |
| } |
| allowed := false | ||
| serverCfg := config.GetGlobalServerConfig() | ||
| for _, user := range serverCfg.Security.ClientAllowedUser { | ||
| if user == username { | ||
| allowed = true | ||
| break | ||
| } | ||
| if slices.Contains(serverCfg.Security.ClientAllowedUser, username) { | ||
| allowed = true | ||
| } |
There was a problem hiding this comment.
The allowed boolean variable can be directly initialized with the result of slices.Contains, making the code cleaner and more concise.
| allowed := false | |
| serverCfg := config.GetGlobalServerConfig() | |
| for _, user := range serverCfg.Security.ClientAllowedUser { | |
| if user == username { | |
| allowed = true | |
| break | |
| } | |
| if slices.Contains(serverCfg.Security.ClientAllowedUser, username) { | |
| allowed = true | |
| } | |
| serverCfg := config.GetGlobalServerConfig() | |
| allowed := slices.Contains(serverCfg.Security.ClientAllowedUser, username) |
| func placeHolder(n int) string { | ||
| var builder strings.Builder | ||
| builder.Grow((n-1)*2 + 1) | ||
| for i := 0; i < n; i++ { | ||
| for i := range n { | ||
| if i > 0 { | ||
| builder.WriteString(",") | ||
| } |
There was a problem hiding this comment.
If n is 0 (e.g., when there are no columns to update or insert), (n-1)*2 + 1 evaluates to -1. Passing a negative value to builder.Grow will cause a runtime panic. Adding a defensive check for n <= 0 prevents this panic.
func placeHolder(n int) string {
if n <= 0 {
return ""
}
var builder strings.Builder
builder.Grow((n-1)*2 + 1)
for i := range n {
if i > 0 {
builder.WriteString(",")
}
|
/test pull-cdc-integration-kafka-test |
- owner.go: write ProcessorsInfos.String() via fmt.Fprintf and guard against a nil *TaskStatus to avoid a panic on dereference - middleware.go: initialize allowed directly from slices.Contains - dml.go: guard placeHolder against n <= 0 so builder.Grow never gets a negative value Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
The error seems legit |
The modernization added a defensive `if n <= 0 { return "" }` guard to
placeHolder, changing its long-standing contract (panic on n <= 0). The
pre-existing TestHolderString still asserts that placeHolder(0) and
placeHolder(-1) panic, so unit tests failed. The only production caller
passes len(columnNames), which is guaranteed >= 1, so the guard added no
value while breaking the test. Revert to the original fail-fast behavior.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
/test pull-verify |
|
@dveeden: The specified target(s) for Use 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. |
|
/retest |
2 participants
What problem does this PR solve?
Issue Number: ref #12691
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