*: clean up low-risk lint issues#5049
Conversation
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
release-note
|
[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 |
📝 WalkthroughWalkthroughThis PR systematically replaces the external PingCAP error package ( ChangesError Package Standardization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a 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 refactors error handling by replacing external and standard library error imports with a local package, alongside minor cleanups like removing unused variables and simplifying mock functions. The reviewer identified several critical issues where the local error package lacks required functions (e.g., New, Trace, Cause, Is) or shadows standard library imports, which will cause compilation errors across multiple files.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/sink/mysql/mysql_writer_ddl_index_rewrite_test.go (1)
473-480:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore assignment to
sourceTableInfoin this call.Discarding this return value leaves
sourceTableInfonil, which makes the laterrequire.NotNil(t, sourceTableInfo)fail deterministically.Proposed fix
- _ = assertRestoreAnonymousIndexToNamedIndex( + sourceTableInfo = assertRestoreAnonymousIndexToNamedIndex( t, helper, "alter table t_anon_idx add column e int, add index (a)", "ALTER TABLE `t_anon_idx` ADD COLUMN `e` INT, ADD INDEX (`a`)", true, timodel.ActionMultiSchemaChange, )🤖 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 `@pkg/sink/mysql/mysql_writer_ddl_index_rewrite_test.go` around lines 473 - 480, The test call to assertRestoreAnonymousIndexToNamedIndex currently discards its return so sourceTableInfo remains nil; change the invocation to capture and assign its return to sourceTableInfo (e.g., sourceTableInfo := assertRestoreAnonymousIndexToNamedIndex(...)) so that the subsequent require.NotNil(t, sourceTableInfo) has a valid value; update the call site where assertRestoreAnonymousIndexToNamedIndex is invoked in mysql_writer_ddl_index_rewrite_test.go to store its returned table info.pkg/etcd/client.go (1)
607-615:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCall
Cause()before checking witherrors.Is()to unwrap the error properly.
client.Get()returns gRPC status errors that require unwrapping withCause()beforeerrors.Is()can reliably match againstv3rpc.ErrPermissionDenied. The rest of the codebase follows this pattern correctly—seeIsRetryableEtcdError()inpkg/errors/utils.go, which explicitly unwraps withCause(err)before checkingerrors.Is(etcdErr, v3rpc.*). Without unwrapping, permission-denied responses may not match and the health check could incorrectly report unhealthy status.Change line 614 to:
etcdErr := errors.Cause(err) return err == nil || errors.Is(etcdErr, v3rpc.ErrPermissionDenied)🤖 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 `@pkg/etcd/client.go` around lines 607 - 615, IsHealthy currently compares the raw error from client.Get against v3rpc.ErrPermissionDenied which fails for gRPC status-wrapped errors; unwrap the returned error using errors.Cause before calling errors.Is. In function IsHealthy, capture the result of errors.Cause(err) (e.g., etcdErr := errors.Cause(err)) and use errors.Is(etcdErr, v3rpc.ErrPermissionDenied) while still returning true when err == nil, so permission-denied responses are recognized correctly.
🤖 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 `@coordinator/changefeed/backoff.go`:
- Line 184: The error string is built in the return statement in backoff.go
using e.Message + e.Code which yields reversed and unseparated values; update
the construction inside the call to errors.ShouldFailChangefeed to use the error
code first and a separator before the message (i.e., build the string as e.Code
+ ": " + e.Message) so the resulting error reads like "ErrXYZ: description"
instead of concatenated words.
In `@pkg/sink/mysql/mysql_writer_dml_test.go`:
- Around line 364-367: The test is asserting len(rowTypes) after calling
writer.generateBatchSQLInUnSafeMode but discards the returned rowTypes, causing
stale-data checks; update each unsafe-mode call to capture all three return
values (e.g. sql, args, rowTypes := writer.generateBatchSQLInUnSafeMode(...))
and then assert len(rowTypes) immediately for that invocation (apply the same
change to the other occurrences around lines indicated — the calls at 364-367,
415-418, and 457-460) so each test uses the fresh rowTypes returned by
generateBatchSQLInUnSafeMode.
- Around line 655-658: The test calls writer.generateBatchSQLInSafeMode but
ignores its returned rowTypes then asserts len(rowTypes) against stale data;
update the call to rebind rowTypes (e.g., sql, args, rowTypes, _ =
writer.generateBatchSQLInSafeMode(...)) so the variable rowTypes reflects the
invocation, then keep the require.Equal(t, 2, len(rowTypes)) assertion; locate
the call to generateBatchSQLInSafeMode and adjust the multiple-assignment to
include rowTypes.
---
Outside diff comments:
In `@pkg/etcd/client.go`:
- Around line 607-615: IsHealthy currently compares the raw error from
client.Get against v3rpc.ErrPermissionDenied which fails for gRPC status-wrapped
errors; unwrap the returned error using errors.Cause before calling errors.Is.
In function IsHealthy, capture the result of errors.Cause(err) (e.g., etcdErr :=
errors.Cause(err)) and use errors.Is(etcdErr, v3rpc.ErrPermissionDenied) while
still returning true when err == nil, so permission-denied responses are
recognized correctly.
In `@pkg/sink/mysql/mysql_writer_ddl_index_rewrite_test.go`:
- Around line 473-480: The test call to assertRestoreAnonymousIndexToNamedIndex
currently discards its return so sourceTableInfo remains nil; change the
invocation to capture and assign its return to sourceTableInfo (e.g.,
sourceTableInfo := assertRestoreAnonymousIndexToNamedIndex(...)) so that the
subsequent require.NotNil(t, sourceTableInfo) has a valid value; update the call
site where assertRestoreAnonymousIndexToNamedIndex is invoked in
mysql_writer_ddl_index_rewrite_test.go to store its returned table info.
🪄 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: d7458179-064b-4cbf-bd03-ee2c8564a29c
📒 Files selected for processing (18)
coordinator/changefeed/backoff.gocoordinator/changefeed/etcd_backend.gologservice/logpuller/priority_queue.gologservice/logpuller/region_request_worker_test.gomaintainer/maintainer_controller_test.gopkg/api/util.gopkg/common/event/dml_event_test.gopkg/etcd/client.gopkg/migrate/migrate.gopkg/orchestrator/reactor_state.gopkg/orchestrator/reactor_state_tester.gopkg/redo/writer/blackhole/writer.gopkg/sink/kafka/mock_factory.gopkg/sink/mysql/mysql_writer_ddl_index_rewrite_test.gopkg/sink/mysql/mysql_writer_dml_test.gopkg/txnutil/gc/gc_manager_test.gopkg/util/external_storage_test.gopkg/util/test_helper_test.go
| // ShouldFailChangefeed return true if a running error contains a changefeed not retry error. | ||
| func ShouldFailChangefeed(e *heartbeatpb.RunningError) bool { | ||
| return cerrors.ShouldFailChangefeed(errors.New(e.Message + e.Code)) | ||
| return errors.ShouldFailChangefeed(errors.New(e.Message + e.Code)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for RunningError construction sites to verify Message/Code format.
# Find where RunningError structs are created
rg -nP --type=go -C3 'RunningError\s*\{' -g '!vendor/'
# Find where RunningError.Message or RunningError.Code are assigned
rg -nP --type=go -C3 '\.(Message|Code)\s*=' -g '!vendor/' -g '*heartbeatpb*'Repository: pingcap/ticdc
Length of output: 20200
🏁 Script executed:
# Read the backoff.go file around line 184 to see the full function context
cat -n coordinator/changefeed/backoff.go | sed -n '170,200p'Repository: pingcap/ticdc
Length of output: 1186
🏁 Script executed:
# Check the RunningError struct definition in protobuf
rg -n --type=go 'type RunningError struct' -A 10Repository: pingcap/ticdc
Length of output: 2413
🏁 Script executed:
# Look for other usages of ShouldFailChangefeed to understand expected error message format
rg -n --type=go 'ShouldFailChangefeed' -B 3 -A 3Repository: pingcap/ticdc
Length of output: 4109
🏁 Script executed:
# Check how e.Message is populated in the backoff code
rg -n --type=go 'e\.Message' coordinator/changefeed/backoff.go -B 2 -A 2Repository: pingcap/ticdc
Length of output: 304
🏁 Script executed:
# Check how ShouldFailChangefeed in errors package actually works
cat -n pkg/errors/helper.go | sed -n '129,160p'Repository: pingcap/ticdc
Length of output: 1053
🏁 Script executed:
# Check how ErrorCode function extracts the code from errors
rg -n --type=go 'func ErrorCode' -A 8Repository: pingcap/ticdc
Length of output: 563
🏁 Script executed:
# Look for RFC or error code related helper functions
rg -n --type=go 'RFCErrorCode' -B 2 -A 5Repository: pingcap/ticdc
Length of output: 3918
🏁 Script executed:
# Check what the ShouldFailChangefeed function at line 130 in helper.go actually checks
rg -n --type=go 'strings.Contains' pkg/errors/helper.go -B 2 -A 2Repository: pingcap/ticdc
Length of output: 446
🏁 Script executed:
# Look for test cases that show how ShouldFailChangefeed is expected to work
cat -n pkg/errors/error_test.go | sed -n '23,100p'Repository: pingcap/ticdc
Length of output: 3023
🏁 Script executed:
# Check the changefeedUnRetryableErrors list to understand what it contains
rg -n --type=go 'changefeedUnRetryableErrors' pkg/errors/helper.go -B 5 -A 5Repository: pingcap/ticdc
Length of output: 1177
🏁 Script executed:
# Check if there are any other examples of how Message and Code are used together
rg -n --type=go 'Message.*Code' coordinator/changefeed/ -A 2 -B 2Repository: pingcap/ticdc
Length of output: 474
🏁 Script executed:
# Look at how the error is supposed to be formatted by checking the comment or intent
rg -n --type=go 'RunningError' coordinator/changefeed/backoff.go -B 5 -A 5Repository: pingcap/ticdc
Length of output: 1780
🏁 Script executed:
# Check if there's any documentation about how the Code field is structured
rg -n --type=go 'RFC.*Code\|error.*code' pkg/errors/helper.go | head -20Repository: pingcap/ticdc
Length of output: 39
🏁 Script executed:
# Look at actual test cases to see what format is expected for e.Code and e.Message
cat -n coordinator/changefeed/backoff_test.go | sed -n '35,70p'Repository: pingcap/ticdc
Length of output: 1308
🏁 Script executed:
# Check if there are any examples showing both Code and Message populated together
rg -n --type=go 'Code:.*Message:' coordinator/changefeed/ -B 1 -A 1Repository: pingcap/ticdc
Length of output: 280
🏁 Script executed:
# Look for any formatter or helper that should be used for RunningError
rg -n --type=go 'func.*RunningError.*String\|func.*Format' -A 5Repository: pingcap/ticdc
Length of output: 39
Fix error message formatting by adding separator and correcting order.
The concatenation e.Message + e.Code produces malformed error strings like "snapshot lost by goCDC:ErrSnapshotLostByGC" with no separator and reversed order. Use e.Code + ": " + e.Message to follow standard error formatting conventions (error code followed by message).
🤖 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 `@coordinator/changefeed/backoff.go` at line 184, The error string is built in
the return statement in backoff.go using e.Message + e.Code which yields
reversed and unseparated values; update the construction inside the call to
errors.ShouldFailChangefeed to use the error code first and a separator before
the message (i.e., build the string as e.Code + ": " + e.Message) so the
resulting error reads like "ErrXYZ: description" instead of concatenated words.
| sql, args, _ = writer.generateBatchSQLInUnSafeMode([]*commonEvent.DMLEvent{dmlDeleteEvent, dmlUpdateEvent}) | ||
| require.Equal(t, 2, len(sql)) | ||
| require.Equal(t, 2, len(args)) | ||
| require.Equal(t, 2, len(rowTypes)) |
There was a problem hiding this comment.
rowTypes assertions are now reading stale data in unsafe-mode test cases.
These calls discard the returned rowTypes, but subsequent assertions still check len(rowTypes). Capture rowTypes for each case that asserts it.
Proposed fix pattern
-sql, args, _ = writer.generateBatchSQLInUnSafeMode(...)
+sql, args, rowTypes = writer.generateBatchSQLInUnSafeMode(...)
require.Equal(t, 2, len(rowTypes))Also applies to: 415-418, 457-460
🤖 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 `@pkg/sink/mysql/mysql_writer_dml_test.go` around lines 364 - 367, The test is
asserting len(rowTypes) after calling writer.generateBatchSQLInUnSafeMode but
discards the returned rowTypes, causing stale-data checks; update each
unsafe-mode call to capture all three return values (e.g. sql, args, rowTypes :=
writer.generateBatchSQLInUnSafeMode(...)) and then assert len(rowTypes)
immediately for that invocation (apply the same change to the other occurrences
around lines indicated — the calls at 364-367, 415-418, and 457-460) so each
test uses the fresh rowTypes returned by generateBatchSQLInUnSafeMode.
| sql, args, _ = writer.generateBatchSQLInSafeMode([]*commonEvent.DMLEvent{dmlInsertEvent, dmlDeleteEvent, dmlUpdateEvent}) | ||
| require.Equal(t, 2, len(sql)) | ||
| require.Equal(t, 2, len(args)) | ||
| require.Equal(t, 2, len(rowTypes)) |
There was a problem hiding this comment.
rowTypes length check in safe-mode test also uses stale value.
This call discards rowTypes but still asserts len(rowTypes) immediately after. Rebind rowTypes from this invocation.
Proposed fix
-sql, args, _ = writer.generateBatchSQLInSafeMode([]*commonEvent.DMLEvent{dmlInsertEvent, dmlDeleteEvent, dmlUpdateEvent})
+sql, args, rowTypes = writer.generateBatchSQLInSafeMode([]*commonEvent.DMLEvent{dmlInsertEvent, dmlDeleteEvent, dmlUpdateEvent})
require.Equal(t, 2, len(sql))
require.Equal(t, 2, len(args))
require.Equal(t, 2, len(rowTypes))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sql, args, _ = writer.generateBatchSQLInSafeMode([]*commonEvent.DMLEvent{dmlInsertEvent, dmlDeleteEvent, dmlUpdateEvent}) | |
| require.Equal(t, 2, len(sql)) | |
| require.Equal(t, 2, len(args)) | |
| require.Equal(t, 2, len(rowTypes)) | |
| sql, args, rowTypes = writer.generateBatchSQLInSafeMode([]*commonEvent.DMLEvent{dmlInsertEvent, dmlDeleteEvent, dmlUpdateEvent}) | |
| require.Equal(t, 2, len(sql)) | |
| require.Equal(t, 2, len(args)) | |
| require.Equal(t, 2, len(rowTypes)) |
🤖 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 `@pkg/sink/mysql/mysql_writer_dml_test.go` around lines 655 - 658, The test
calls writer.generateBatchSQLInSafeMode but ignores its returned rowTypes then
asserts len(rowTypes) against stale data; update the call to rebind rowTypes
(e.g., sql, args, rowTypes, _ = writer.generateBatchSQLInSafeMode(...)) so the
variable rowTypes reflects the invocation, then keep the require.Equal(t, 2,
len(rowTypes)) assertion; locate the call to generateBatchSQLInSafeMode and
adjust the multiple-assignment to include rowTypes.
|
@3AceShowHand: 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. |
ti-chi-bot
Bot
added
the
release-note-none
|
[FORMAT CHECKER NOTIFICATION] Notice: To remove the 📖 For more info, you can check the "Contribute Code" section in the development guide. |
ti-chi-bot
Bot
removed
the
release-note
1 participant
What problem does this PR solve?
Issue Number: close #xxx
make check-staticreports a batch of small lint issues. This PR intentionally keeps the scope to low-risk lint cleanup so the diff is easy to review and does not mix in broader refactors.What is changed and how it works?
github.com/pingcap/ticdc/pkg/errorsto follow the repository depguard rule.errors.Iswhere touched by the lint cleanup, so wrapped errors are handled correctly.unconvert, including redundant timestamp, byte slice, and integer conversions.Review notes:
errors.Ismatching wrapped sentinel errors, which matches Go error-handling expectations.Check List
Tests
go test --tags=intest $(git diff upstream/master --name-only -- '*.go' | xargs -n1 dirname | sort -u | grep -v '^tests/integration_tests/' | sed 's#^#./#')tools/bin/golangci-lint run --new-from-rev upstream/master --timeout 10m0stools/bin/golangci-lint run --enable-only unconvert --max-issues-per-linter 0 --max-same-issues 0 --timeout 10m0sgit diff --checkNote: full
make check-staticstill reports existing full-repo baseline lint issues, so--new-from-rev upstream/masteris used to validate that this PR does not add new lint issues.Questions
Will it cause performance regression or break compatibility?
No. The changes are lint cleanup only. No performance or compatibility impact is expected.
Do you need to update user documentation, design documentation or monitoring documentation?
No.
Release note