*: fix golangci-lint issues and delete unused code#5037
Open
3AceShowHand wants to merge 18 commits into
Open
Conversation
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
release-note
Contributor
|
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:
📝 WalkthroughWalkthroughStandardizes wrapped-error handling (Is/As/Cause), renames unused params to ChangesRepository-wide consistency changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
ti-chi-bot
Bot
added
do-not-merge/work-in-progress
![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 across several modules, replacing direct error comparisons and type assertions with more robust errors.Is and errors.As patterns, typically using the cerror wrapper. It also cleans up a duplicate line in expr_filter.go and adds a lint suppression in subscription_client.go. The review feedback identifies several instances where the use of errors.Cause(err) is redundant because cerror.Is and cerror.As already handle error unwrapping, and also points out an indentation issue in api_client.go. Addressing these suggestions will improve code idiomaticity and maintainability.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (5)
tests/integration_tests/bank/case.go (1)
564-564: ⚡ Quick winRemove redundant
errors.Cause()unwrapping beforecerror.Is()check.The pattern
cerror.Is(errors.Cause(err), context.Canceled)is redundant becausecerror.Is()already traverses the error chain to detect cancellation in wrapped errors. Usecerror.Is(err, context.Canceled)directly.♻️ Simplify error check
err := workload() - if err != nil && !cerror.Is(errors.Cause(err), context.Canceled) { + if err != nil && !cerror.Is(err, context.Canceled) { log.Warn("workload failed", zap.Error(err)) }🤖 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/bank/case.go` at line 564, The error check is using redundant unwrapping with errors.Cause(err) before calling cerror.Is; update the condition to call cerror.Is(err, context.Canceled) directly (leave the surrounding if logic intact) so the wrapped error chain is correctly inspected without the extra errors.Cause() call; change the expression involving cerror.Is and errors.Cause to use cerror.Is(err, context.Canceled) referencing the existing err variable and context.Canceled symbol.pkg/pdutil/api_client.go (2)
366-366: ⚡ Quick winRemove redundant
errors.Cause()unwrapping beforecerror.Is()check.Same issue as line 190:
cerror.Is()already traverses the error chain, so unwrapping witherrors.Cause()first is redundant.♻️ Simplify error check
}, retry.WithMaxTries(defaultMaxRetry), retry.WithIsRetryableErr(func(err error) bool { - if cerror.Is(errors.Cause(err), context.Canceled) { + if cerror.Is(err, context.Canceled) { return false } return true🤖 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/pdutil/api_client.go` at line 366, The check at cerror.Is(errors.Cause(err), context.Canceled) is redundant because cerror.Is already walks the error chain; change the call to use the original error (cerror.Is(err, context.Canceled)) instead of wrapping with errors.Cause(err); apply the same simplification used earlier (see the similar pattern around the code that referenced line 190) to remove unnecessary unwrapping and keep error-chain-aware checks in functions in api_client.go.
190-193: ⚡ Quick winRemove redundant
errors.Cause()unwrapping beforecerror.Is()check.The pattern
cerror.Is(errors.Cause(err), context.Canceled)is redundant becausecerror.Is()already traverses the error chain to match wrapped errors. Usecerror.Is(err, context.Canceled)directly.♻️ Simplify error check
retry.WithIsRetryableErr(func(err error) bool { - if cerror.Is(errors.Cause(err), context.Canceled) { + if cerror.Is(err, context.Canceled) { return false } return true }))🤖 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/pdutil/api_client.go` around lines 190 - 193, The error check currently calls cerror.Is(errors.Cause(err), context.Canceled) which redundantly unwraps the error; change it to call cerror.Is(err, context.Canceled) directly (keeping the same behavior for the variable err and the context.Canceled sentinel) and remove any now-unused errors.Cause import if it becomes unused.pkg/sink/codec/csv/csv_decoder.go (1)
113-113: ⚡ Quick winRemove redundant
errors.Cause()unwrapping beforeerrors.Is()check.The pattern
errors.Is(errors.Cause(err), io.EOF)is redundant becauseerrors.Is()already traverses the error chain to match EOF in wrapped errors. Useerrors.Is(err, io.EOF)directly.♻️ Simplify EOF check
- if errors.Is(errors.Cause(err), io.EOF) { + if errors.Is(err, io.EOF) { return common.MessageTypeUnknown, false }🤖 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/codec/csv/csv_decoder.go` at line 113, Replace the redundant errors.Is(errors.Cause(err), io.EOF) check with errors.Is(err, io.EOF) in the CSV decoder code: locate the EOF handling in pkg/sink/codec/csv/csv_decoder.go where the variable err is being checked and change the call to use errors.Is(err, io.EOF) directly (no errors.Cause unwrap) so the error chain is properly inspected without redundant unwrapping.pkg/orchestrator/etcd_worker_test.go (1)
275-276: ⚡ Quick winRemove redundant
errors.Cause()unwrapping beforecerrors.Is()check.The pattern
cerrors.Is(errors.Cause(err), target)is redundant becausecerrors.Is()already traverses the error chain to match wrapped errors. Unwrapping witherrors.Cause()first defeats the purpose of using the wrapped-error-awareIs()check.♻️ Simplify error checks
- if err != nil && (cerrors.Is(errors.Cause(err), context.DeadlineExceeded) || - cerrors.Is(errors.Cause(err), context.Canceled) || + if err != nil && (cerrors.Is(err, context.DeadlineExceeded) || + cerrors.Is(err, context.Canceled) || strings.Contains(err.Error(), "etcdserver: request timeout")) { return }🤖 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/orchestrator/etcd_worker_test.go` around lines 275 - 276, The error checks unnecessarily call errors.Cause(err) before cerrors.Is, which is redundant because cerrors.Is already walks wrapped errors; update the conditional in the test to call cerrors.Is(err, context.DeadlineExceeded) and cerrors.Is(err, context.Canceled) (and any other cerrors.Is checks) directly instead of cerrors.Is(errors.Cause(err), ...), leaving the rest of the logic in the function (the conditional around err) unchanged.
🤖 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.
Nitpick comments:
In `@pkg/orchestrator/etcd_worker_test.go`:
- Around line 275-276: The error checks unnecessarily call errors.Cause(err)
before cerrors.Is, which is redundant because cerrors.Is already walks wrapped
errors; update the conditional in the test to call cerrors.Is(err,
context.DeadlineExceeded) and cerrors.Is(err, context.Canceled) (and any other
cerrors.Is checks) directly instead of cerrors.Is(errors.Cause(err), ...),
leaving the rest of the logic in the function (the conditional around err)
unchanged.
In `@pkg/pdutil/api_client.go`:
- Line 366: The check at cerror.Is(errors.Cause(err), context.Canceled) is
redundant because cerror.Is already walks the error chain; change the call to
use the original error (cerror.Is(err, context.Canceled)) instead of wrapping
with errors.Cause(err); apply the same simplification used earlier (see the
similar pattern around the code that referenced line 190) to remove unnecessary
unwrapping and keep error-chain-aware checks in functions in api_client.go.
- Around line 190-193: The error check currently calls
cerror.Is(errors.Cause(err), context.Canceled) which redundantly unwraps the
error; change it to call cerror.Is(err, context.Canceled) directly (keeping the
same behavior for the variable err and the context.Canceled sentinel) and remove
any now-unused errors.Cause import if it becomes unused.
In `@pkg/sink/codec/csv/csv_decoder.go`:
- Line 113: Replace the redundant errors.Is(errors.Cause(err), io.EOF) check
with errors.Is(err, io.EOF) in the CSV decoder code: locate the EOF handling in
pkg/sink/codec/csv/csv_decoder.go where the variable err is being checked and
change the call to use errors.Is(err, io.EOF) directly (no errors.Cause unwrap)
so the error chain is properly inspected without redundant unwrapping.
In `@tests/integration_tests/bank/case.go`:
- Line 564: The error check is using redundant unwrapping with errors.Cause(err)
before calling cerror.Is; update the condition to call cerror.Is(err,
context.Canceled) directly (leave the surrounding if logic intact) so the
wrapped error chain is correctly inspected without the extra errors.Cause()
call; change the expression involving cerror.Is and errors.Cause to use
cerror.Is(err, context.Canceled) referencing the existing err variable and
context.Canceled symbol.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 041c685d-b320-4a15-a94c-26c6c1f6e39e
📒 Files selected for processing (11)
logservice/logpuller/subscription_client.gopkg/filter/expr_filter.gopkg/messaging/message_center.gopkg/orchestrator/etcd_worker_test.gopkg/pdutil/api_client.gopkg/redo/reader/file.gopkg/sink/codec/common/helper.gopkg/sink/codec/csv/csv_decoder.gopkg/sink/kafka/options_test.gotests/integration_tests/bank/case.gotests/integration_tests/many_pk_or_uk/main.go
💤 Files with no reviewable changes (1)
- pkg/filter/expr_filter.go
ti-chi-bot
Bot
added
size/L
…ineffassign-nilerr-errorlint
ti-chi-bot
Bot
added
size/XL
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
ti-chi-bot
Bot
removed
the
do-not-merge/work-in-progress
Collaborator
Author
|
/test all |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/sink/codec/common/helper.go (1)
328-329: ⚡ Quick winSimplify error type checking for consistency.
Line 329 calls
errors.Cause(err)beforeerrors.As, buterrors.Asalready unwraps errors internally per Go 1.13+ semantics. This is redundant and inconsistent with line 272 inqueryRowChecksumAux, which correctly useserrors.As(err, &mysqlErr)directly.♻️ Proposed fix to remove redundant unwrapping
var mysqlErr *mysqlDriver.MySQLError - ok := errors.As(errors.Cause(err), &mysqlErr) + ok := errors.As(err, &mysqlErr)🤖 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/codec/common/helper.go` around lines 328 - 329, Replace the redundant unwrapping call in the MySQL error type check: instead of calling errors.As(errors.Cause(err), &mysqlErr) (where mysqlErr is a *mysqlDriver.MySQLError), call errors.As(err, &mysqlErr) directly so the built-in unwrapping semantics are used consistently (as done in queryRowChecksumAux); update the check in helper.go accordingly.pkg/sink/kafka/options_test.go (1)
834-835: ⚡ Quick winSimplify error type checking.
Line 835 calls
errors.Cause(err)beforeerrors.As, buterrors.Asalready unwraps errors internally per Go 1.13+ semantics. The redundanterrors.Causecall is unnecessary.♻️ Proposed fix to remove redundant unwrapping
var numError *strconv.NumError - ok := errors.As(errors.Cause(err), &numError) + ok := errors.As(err, &numError)🤖 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/kafka/options_test.go` around lines 834 - 835, The test currently does redundant unwrapping by calling errors.Cause(err) before errors.As; update the check that declares var numError *strconv.NumError and the call ok := errors.As(errors.Cause(err), &numError) to call errors.As directly on err (i.e., ok := errors.As(err, &numError)), relying on Go's built-in unwrapping semantics so strconv.NumError is detected correctly without errors.Cause.
🤖 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.
Nitpick comments:
In `@pkg/sink/codec/common/helper.go`:
- Around line 328-329: Replace the redundant unwrapping call in the MySQL error
type check: instead of calling errors.As(errors.Cause(err), &mysqlErr) (where
mysqlErr is a *mysqlDriver.MySQLError), call errors.As(err, &mysqlErr) directly
so the built-in unwrapping semantics are used consistently (as done in
queryRowChecksumAux); update the check in helper.go accordingly.
In `@pkg/sink/kafka/options_test.go`:
- Around line 834-835: The test currently does redundant unwrapping by calling
errors.Cause(err) before errors.As; update the check that declares var numError
*strconv.NumError and the call ok := errors.As(errors.Cause(err), &numError) to
call errors.As directly on err (i.e., ok := errors.As(err, &numError)), relying
on Go's built-in unwrapping semantics so strconv.NumError is detected correctly
without errors.Cause.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8976b863-9641-49b9-9978-ad75436a7423
📒 Files selected for processing (13)
Makefiledownstreamadapter/sink/pulsar/helper.godownstreamadapter/sink/topicmanager/kafka_topic_manager.godownstreamadapter/sink/topicmanager/pulsar_topic_manager.godownstreamadapter/sink/topicmanager/pulsar_topic_manager_mock.godownstreamadapter/sink/topicmanager/pulsar_topic_manager_test.gopkg/errors/reexport.gopkg/messaging/message_center.gopkg/orchestrator/etcd_worker_test.gopkg/pdutil/api_client.gopkg/redo/reader/file.gopkg/sink/codec/common/helper.gopkg/sink/kafka/options_test.go
💤 Files with no reviewable changes (1)
- downstreamadapter/sink/topicmanager/pulsar_topic_manager.go
Collaborator
Author
|
/test all |
3AceShowHand
changed the title
ti-chi-bot
Bot
added
release-note-none
3AceShowHand
changed the title
ti-chi-bot
Bot
added
size/XXL
Collaborator
Author
|
/test all |
tenfyzhong
approved these changes
May 13, 2026
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tenfyzhong 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:
|
Collaborator
Author
|
/test all |
Collaborator
Author
|
/test all |
Collaborator
Author
|
@CodeRabbit review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/check/active_active_tso_indexes.go (1)
173-173: ⚡ Quick winConsider using predefined errors for better categorization.
These validation errors currently use generic
errors.Neworerrors.Errorf. For consistency and better error handling, consider defining predefined errors for:
- Inconsistent TSO index values across TiDB instances (lines 173, 186)
- Missing required TSO config keys (lines 196, 199)
This would enable callers to distinguish between different failure modes using
errors.Is().Also applies to: 186-186, 196-196, 199-199
🤖 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/check/active_active_tso_indexes.go` at line 173, Define package-level sentinel errors (e.g., var ErrInconsistentTSOIndex = errors.New("inconsistent tso-unique-index across TiDB instances") and var ErrMissingTSOConfigKey = errors.New("missing required tso config key")) in pkg/check/active_active_tso_indexes.go and replace the ad-hoc errors.New/Errorsf returns that currently use the literal messages (for example the return using "downstream TiDB reports inconsistent tso-unique-index across instances" and the returns using "missing required tso config key") to return or wrap those sentinel errors; where additional context (like which key) is helpful, wrap the sentinel with fmt.Errorf("%w: key=%s", ErrMissingTSOConfigKey, key) so callers can use errors.Is() to detect ErrInconsistentTSOIndex and ErrMissingTSOConfigKey.
🤖 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/sink/mysql/helper.go`:
- Around line 206-209: Replace the direct sentinel comparison against
sql.ErrNoRows with an errors.Is check: in the block where
db.QueryRowContext(...).Scan(&name, &value) is called (the session variable
lookup using variableName), change the condition `err != nil && err !=
sql.ErrNoRows` to use `errors.Is(err, sql.ErrNoRows)` so wrapped ErrNoRows is
handled correctly; update the conditional to `if err != nil && !errors.Is(err,
sql.ErrNoRows) { ... }` (same error wrapping via ErrMySQLQueryError stays
unchanged).
---
Nitpick comments:
In `@pkg/check/active_active_tso_indexes.go`:
- Line 173: Define package-level sentinel errors (e.g., var
ErrInconsistentTSOIndex = errors.New("inconsistent tso-unique-index across TiDB
instances") and var ErrMissingTSOConfigKey = errors.New("missing required tso
config key")) in pkg/check/active_active_tso_indexes.go and replace the ad-hoc
errors.New/Errorsf returns that currently use the literal messages (for example
the return using "downstream TiDB reports inconsistent tso-unique-index across
instances" and the returns using "missing required tso config key") to return or
wrap those sentinel errors; where additional context (like which key) is
helpful, wrap the sentinel with fmt.Errorf("%w: key=%s", ErrMissingTSOConfigKey,
key) so callers can use errors.Is() to detect ErrInconsistentTSOIndex and
ErrMissingTSOConfigKey.
🪄 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: 011d0a1e-d9c7-4cda-9b54-13191f56564e
📒 Files selected for processing (55)
.golangci.ymlapi/status/status.gocmd/cdc/cli/cli_changefeed_helper.gocmd/storage-consumer/consumer.gocmd/storage-consumer/main.gocoordinator/controller.gocoordinator/coordinator_test.godownstreamadapter/dispatchermanager/dispatcher_manager_redo.godownstreamadapter/dispatchermanager/helper.godownstreamadapter/sink/blackhole/sink.godownstreamadapter/sink/pulsar/ddl_producer.godownstreamadapter/sink/pulsar/dml_producer.godownstreamadapter/sink/pulsar/mock_producer.godownstreamadapter/sink/pulsar/sink.godownstreamadapter/sink/redo/sink.godownstreamadapter/sink/topicmanager/kafka_topic_manager.godownstreamadapter/sink/topicmanager/pulsar_topic_manager.gologservice/coordinator/coordinator.gologservice/eventstore/event_store.gologservice/logpuller/subscription_client.gomaintainer/testutil/test_util.gopkg/check/active_active_tso_indexes.gopkg/common/log_coordinator.gopkg/diff/checkpoint.gopkg/diff/diff.gopkg/encryption/tikv_http_client.gopkg/errors/helper.gopkg/errors/utils.gopkg/etcd/client.gopkg/eventservice/event_scanner.gopkg/filter/expr_filter.gopkg/filter/utils.gopkg/httputil/httputil.gopkg/logger/log.gopkg/messaging/remote_target.gopkg/redo/reader/file.gopkg/redo/reader/reader.gopkg/retry/retry_test.gopkg/sink/codec/bootstraper.gopkg/sink/kafka/claimcheck/claim_check.gopkg/sink/mysql/config.gopkg/sink/mysql/helper.gopkg/sink/mysql/mysql_writer.gopkg/sink/mysql/mysql_writer_ddl.gopkg/sink/mysql/mysql_writer_dml_exec.gopkg/sink/mysql/mysql_writer_for_active_active_sync_stats.gopkg/sink/mysql/mysql_writer_for_ddl_ts.gopkg/sink/mysql/progress_table_writer.gopkg/sink/mysql/progress_table_writer_test.gopkg/sink/mysql/sql_builder_test.gopkg/tcpserver/tcp_server.gopkg/upstream/manager.gopkg/util/json_writer.gopkg/version/check.gotests/integration_tests/bank/case.go
✅ Files skipped from review due to trivial changes (14)
- downstreamadapter/sink/pulsar/sink.go
- downstreamadapter/sink/pulsar/dml_producer.go
- api/status/status.go
- pkg/sink/codec/bootstraper.go
- downstreamadapter/sink/pulsar/ddl_producer.go
- maintainer/testutil/test_util.go
- pkg/common/log_coordinator.go
- downstreamadapter/sink/blackhole/sink.go
- pkg/sink/mysql/mysql_writer_for_ddl_ts.go
- downstreamadapter/sink/pulsar/mock_producer.go
- logservice/eventstore/event_store.go
- pkg/util/json_writer.go
- pkg/sink/kafka/claimcheck/claim_check.go
- downstreamadapter/dispatchermanager/helper.go
🚧 Files skipped from review as they are similar to previous changes (21)
- downstreamadapter/sink/topicmanager/pulsar_topic_manager.go
- cmd/storage-consumer/main.go
- pkg/httputil/httputil.go
- pkg/diff/checkpoint.go
- pkg/sink/mysql/mysql_writer_ddl.go
- coordinator/controller.go
- logservice/coordinator/coordinator.go
- pkg/diff/diff.go
- pkg/sink/mysql/mysql_writer_for_active_active_sync_stats.go
- pkg/redo/reader/reader.go
- downstreamadapter/sink/topicmanager/kafka_topic_manager.go
- pkg/logger/log.go
- cmd/storage-consumer/consumer.go
- downstreamadapter/sink/redo/sink.go
- coordinator/coordinator_test.go
- pkg/tcpserver/tcp_server.go
- pkg/filter/utils.go
- pkg/retry/retry_test.go
- pkg/errors/utils.go
- pkg/sink/mysql/mysql_writer.go
- pkg/redo/reader/file.go
Comment on lines
206
to
209
| err := db.QueryRowContext(context.Background(), querySQL).Scan(&name, &value) | ||
| if err != nil && err != sql.ErrNoRows { | ||
| errMsg := "fail to query session variable " + variableName | ||
| return "", cerror.ErrMySQLQueryError.Wrap(err).GenWithStack(errMsg) | ||
| return "", errors.ErrMySQLQueryError.Wrap(err).GenWithStack("fail to query session variable %s", variableName) | ||
| } |
Contributor
There was a problem hiding this comment.
Missed errorlint fix: replace direct err != sql.ErrNoRows comparison with errors.Is.
While the adjacent wrap on line 208 was migrated, the sentinel comparison on line 207 still uses ==/!= against sql.ErrNoRows. This is exactly the errorlint pattern this PR is fixing elsewhere (and the AI summary states sql.ErrNoRows is handled via errors.Is in checkTiDBVariable, which doesn't match the code). If Scan ever returns a wrapped sql.ErrNoRows (e.g., from a future driver wrapper or middleware), this branch will misclassify it as a query failure.
🛠️ Proposed fix
err := db.QueryRowContext(context.Background(), querySQL).Scan(&name, &value)
- if err != nil && err != sql.ErrNoRows {
+ if err != nil && !errors.Is(err, sql.ErrNoRows) {
return "", errors.ErrMySQLQueryError.Wrap(err).GenWithStack("fail to query session variable %s", variableName)
}📝 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.
Suggested change
| err := db.QueryRowContext(context.Background(), querySQL).Scan(&name, &value) | |
| if err != nil && err != sql.ErrNoRows { | |
| errMsg := "fail to query session variable " + variableName | |
| return "", cerror.ErrMySQLQueryError.Wrap(err).GenWithStack(errMsg) | |
| return "", errors.ErrMySQLQueryError.Wrap(err).GenWithStack("fail to query session variable %s", variableName) | |
| } | |
| err := db.QueryRowContext(context.Background(), querySQL).Scan(&name, &value) | |
| if err != nil && !errors.Is(err, sql.ErrNoRows) { | |
| return "", errors.ErrMySQLQueryError.Wrap(err).GenWithStack("fail to query session variable %s", variableName) | |
| } |
🤖 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/helper.go` around lines 206 - 209, Replace the direct sentinel
comparison against sql.ErrNoRows with an errors.Is check: in the block where
db.QueryRowContext(...).Scan(&name, &value) is called (the session variable
lookup using variableName), change the condition `err != nil && err !=
sql.ErrNoRows` to use `errors.Is(err, sql.ErrNoRows)` so wrapped ErrNoRows is
handled correctly; update the conditional to `if err != nil && !errors.Is(err,
sql.ErrNoRows) { ... }` (same error wrapping via ErrMySQLQueryError stays
unchanged).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue Number: ref #5032
What problem does this PR solve?
Fix multiple categories of golangci-lint issues reported by
make check-static, reducing the total from ~300 to ~250.What is changed and how it works?
Auto-fixes (ineffassign, nilerr):
expr_filter.go: remove duplicateFillVirtualColumnValuecallconsumer.go: remove deadtableNameassignmentdrop_event.go: remove deadoffset += 8before returnfile.go:return nil→return errmysql_writer_ddl.go:return nil→return errpool_test.go:return nil→return errerrorlint — use errors.Is/errors.As instead of ==/type-assertion (12 files):
message_center.go:err != context.DeadlineExceeded→!pkgerror.Is(err, context.DeadlineExceeded)etcd_worker_test.go:== context.DeadlineExceeded→cerrors.Is(..., context.DeadlineExceeded)api_client.go:switch { case context.Canceled }→if cerror.Is(..., context.Canceled), simplifiedif-then-return→return !cerror.Is(...)file.go:err != io.EOF→!cerror.Is(err, io.EOF)csv_decoder.go:err == io.EOF→errors.Is(err, io.EOF)bank/case.go:!= context.Canceled→!cerror.Is(..., context.Canceled)helper.go,many_pk_or_uk/main.go,options_test.go: type assertions →errors.Asincrement-decrement (7 files):
controller.go,dml_event.go,csv_decoder.go,helper.go,sync_point.goetc.:+= 1→++,-= 1→--unused code deletion (14 files):
stream.go: removestreamWrappertype, methods,streamGeneratorremote_target.go,errors.go,claim_check.go: remove unused consts/varscli_changefeed_helper.go: removereadInputactive_active.go: removegetSoftDeleteTimeColumnIndexmodel.go: removegetDefaultVerifyTableConfig,toCredentialbarrier_event.go,splitter.go,utils.go,message.go,thread_pool_test_task.go, etc.redefines-builtin-id:
atomic.go: renamenew→newValConfig:
.golangci.yml: suppress ST1003 (variable naming for legacy identifiers)Makefile: addlocal-static-checktarget for branch-diff lintingpkg/errors re-export:
reexport.go: addWithStack,ErrorStack,ErrorEqual,NewNoStackError,NotFoundf,NotValidf,Normalize,RFCCodeText,Unwrap,RedactLogEnabled, plus type aliasesError,RFCErrorCode,ErrCodeCheck List
Tests:
go build ./...passesCode changes:
Side effects:
Release note
Summary by CodeRabbit
Refactor
Bug Fixes
New Features
Chores