lint: wire make lint into the make check#5021
Open
3AceShowHand wants to merge 7 commits into
Open
Conversation
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
release-note
Contributor
|
Important Review skippedToo many files! This PR contains 187 files, which is 37 over the limit of 150. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (187)
You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughRepo-wide hygiene: upgrade golangci-lint and CI wiring; convert many error checks to errors.Is / errors.As; replace fmt.Sprintf allocations with fmt.Fprintf into strings.Builder; remove per-iteration test shadowing; assorted small refactors and typo fixes. ChangesTooling & CI
Error handling & unwrapping
Formatting simplifications
Tests: parallel subtests fixups
Misc and refactors
Estimated code review effort: Possibly related PRs:
Suggested labels: Suggested reviewers:
Poem:
✨ Finishing Touches🧪 Generate unit tests (beta)
|
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 updates the project's linting configuration and dependencies. Key changes include modifying the Makefile to support incremental linting via the LINT_NEW_FROM_REV variable and integrating static checks into the main check target. However, the review identifies several critical issues: the introduction of an incorrect 'v2' module path for golangci-lint, the use of non-existent versions for multiple dependencies in go.mod, and the removal of the tests directory exclusion which may cause build failures.
|
|
||
| tools/bin/golangci-lint: tools/check/go.mod | ||
| cd tools/check && $(GO) build -mod=mod -o ../bin/golangci-lint github.com/golangci/golangci-lint/cmd/golangci-lint | ||
| cd tools/check && $(GO) build -mod=mod -o ../bin/golangci-lint github.com/golangci/golangci-lint/v2/cmd/golangci-lint |
There was a problem hiding this comment.
The build path github.com/golangci/golangci-lint/v2/cmd/golangci-lint is incorrect. The official golangci-lint tool uses the github.com/golangci/golangci-lint/cmd/golangci-lint path. This change will cause the build to fail.
cd tools/check && $(GO) build -mod=mod -o ../bin/golangci-lint github.com/golangci/golangci-lint/cmd/golangci-lint
| github.com/gogo/protobuf v1.3.2 | ||
| github.com/golang/mock v1.7.0-rc.1 | ||
| github.com/golangci/golangci-lint v1.61.1-0.20240915150923-7187c89d4091 | ||
| github.com/golangci/golangci-lint/v2 v2.12.2 |
There was a problem hiding this comment.
The module path github.com/golangci/golangci-lint/v2 and version v2.12.2 appear to be incorrect. The official golangci-lint repository does not use a /v2 module path, and the version v2.12.2 does not exist in its release history. Furthermore, several other dependencies in this file (e.g., golang.org/x/tools v0.44.0, google.golang.org/protobuf v1.36.10) refer to non-existent versions. These changes will likely cause the build to fail when resolving modules.
| _ "github.com/gogo/protobuf/protoc-gen-gogofaster" | ||
| _ "github.com/golang/mock/mockgen" | ||
| _ "github.com/golangci/golangci-lint/cmd/golangci-lint" | ||
| _ "github.com/golangci/golangci-lint/v2/cmd/golangci-lint" |
There was a problem hiding this comment.
The import path github.com/golangci/golangci-lint/v2/cmd/golangci-lint is incorrect as the official tool does not use a /v2 module path. This should be reverted to github.com/golangci/golangci-lint/cmd/golangci-lint to match the correct module structure.
Suggested change
| _ "github.com/golangci/golangci-lint/v2/cmd/golangci-lint" | |
| _ "github.com/golangci/golangci-lint/cmd/golangci-lint" |
Comment on lines
+313
to
+317
| ifneq ($(LINT_NEW_FROM_REV),) | ||
| tools/bin/golangci-lint run --timeout 10m0s --new-from-rev=$(LINT_NEW_FROM_REV) | ||
| else | ||
| tools/bin/golangci-lint run --timeout 10m0s | ||
| endif |
There was a problem hiding this comment.
The --exclude-dirs "^tests/" flag was removed in this change. This might cause the linter to run on the tests/ directory, which was previously excluded. If the tests do not yet pass the linting rules, this will break the build. It is recommended to keep the exclusion unless you intended to enable linting for tests.
ifneq ($(LINT_NEW_FROM_REV),)
tools/bin/golangci-lint run --timeout 10m0s --exclude-dirs "^tests/" --new-from-rev=$(LINT_NEW_FROM_REV)
else
tools/bin/golangci-lint run --timeout 10m0s --exclude-dirs "^tests/"
endif
|
[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 |
Collaborator
Author
|
/retest |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/orchestrator/batch_test.go (1)
28-35:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winAdd per-iteration rebinding for loop variable
ito fix closure capture bug in testThe closures at lines 32–35 and 65–68 capture the loop variable
iwithout rebinding. When executed later bygetBatchChangedState, all stored closures reference the final loop value (999 in the first loop,etcdTxnMaxOps*2in the second) instead of their intended per-iteration value. This causes the test assertion at line 45 to fail, as it expectschangedState["/key0"]to contain"abc0"but receives the incorrect value from the final iteration.Add
i := iimmediately after eachforstatement to create a new binding per iteration.Suggested fix for first loop
for i := 0; i < patchGroupSize; i++ { + i := i patches := []DataPatch{&SingleDataPatch{ Key: util.NewEtcdKey(fmt.Sprintf("/key%d", i)), Func: func(old []byte) (newValue []byte, changed bool, err error) { newValue = []byte(fmt.Sprintf("abc%d", i)) return newValue, true, nil }, }}Apply the same fix at line 62.
🤖 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/batch_test.go` around lines 28 - 35, The test closures capture the loop variable i incorrectly, so rebind i per iteration by adding a new local binding (i := i) immediately inside each for loop before constructing the SingleDataPatch/closure; update both loops that create patches (the one that builds patches with SingleDataPatch{ Key: util.NewEtcdKey(fmt.Sprintf("/key%d", i)), Func: ... } and the second loop around lines 62) so each closure captures the correct iteration value used later by getBatchChangedState and the assertions.pkg/sink/mysql/helper.go (1)
193-199:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRestrict base64-password retry to access-denied errors.
At Line 194, retry/decode is triggered for any MySQL error. This should stay scoped to access-denied cases; otherwise unrelated errors can take an unnecessary alternate path and obscure diagnosis.
Proposed fix
mysqlErr := &dmysql.MySQLError{} if stderrors.As(errors.Cause(err), &mysqlErr) { - if dePassword, decodeErr := base64.StdEncoding.DecodeString(password); decodeErr == nil && string(dePassword) != password { - dbConfig.Passwd = string(dePassword) - testDB, err = CreateMysqlDBConn(dbConfig.FormatDSN()) - } + if mysqlErr.Number == mysql.ErrAccessDenied || + mysqlErr.Number == mysql.ErrAccessDeniedNoPassword { + if dePassword, decodeErr := base64.StdEncoding.DecodeString(password); decodeErr == nil && string(dePassword) != password { + dbConfig.Passwd = string(dePassword) + testDB, err = CreateMysqlDBConn(dbConfig.FormatDSN()) + } + } }🤖 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 193 - 199, The current retry logic decodes the password and retries on any MySQL error; limit this to only access-denied errors by checking the MySQL error number before attempting a base64 decode and retry. In the block that inspects errors.Cause(err) into mysqlErr (dmysql.MySQLError), add a condition that mysqlErr.Number equals the access-denied code (1045) and only then perform base64.StdEncoding.DecodeString(password), set dbConfig.Passwd and call CreateMysqlDBConn(dbConfig.FormatDSN()); leave other errors untouched so they propagate normally.
🧹 Nitpick comments (6)
pkg/retry/retry_test.go (1)
68-70: ⚡ Quick winRedundant error unwrapping: use either
errors.Causeorerrors.Is, not both.
errors.Causefrompingcap/errorsalready unwraps to the root error, so applyingstderrors.Ison top is redundant. Choose one approach:
- Option 1:
case errors.Cause(err) == context.Canceled:- Option 2:
case errors.Is(err, context.Canceled):♻️ Proposed refactor (Option 1)
err := Do(context.Background(), f, WithMaxTries(3), WithIsRetryableErr(func(err error) bool { switch { - case stderrors.Is(errors.Cause(err), context.Canceled): + case errors.Cause(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/retry/retry_test.go` around lines 68 - 70, The switch block currently uses redundant unwrapping (stderrors.Is(errors.Cause(err), context.Canceled)); replace that with a single check using the standard library: change to `case errors.Is(err, context.Canceled):` (or alternatively `case errors.Cause(err) == context.Canceled:` if you prefer pingcap/errors equality), updating imports if needed so only the chosen form remains.pkg/sink/mysql/mysql_writer.go (1)
264-264: ⚡ Quick winRedundant error unwrapping: use either
errors.Causeorerrors.Is, not both.
errors.Causefrompingcap/errorsalready unwraps to the root error, so applyingstderrors.Ison top is redundant. Choose one approach:
- Option 1:
errors.Cause(err) == cerror.ErrMySQLDuplicateEntry- Option 2:
errors.Is(err, cerror.ErrMySQLDuplicateEntry)(usingpingcap/errors.Isif available, or stdlib)♻️ Proposed refactor (Option 1)
- if stderrors.Is(errors.Cause(err), cerror.ErrMySQLDuplicateEntry) || + if errors.Cause(err) == cerror.ErrMySQLDuplicateEntry ||🤖 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.go` at line 264, The conditional is redundantly unwrapping the error by calling errors.Cause(err) and then passing that to stderrors.Is; replace with a single check using one approach: either compare the cause directly (errors.Cause(err) == cerror.ErrMySQLDuplicateEntry) or use errors.Is on the original error (stderrors.Is(err, cerror.ErrMySQLDuplicateEntry) or pingcap/errors.Is if available). Update the conditional that currently uses errors.Cause(err) and stderrors.Is to use one of these options consistently (refer to errors.Cause, stderrors.Is, and cerror.ErrMySQLDuplicateEntry in mysql_writer.go) and remove the redundant unwrap.pkg/orchestrator/etcd_worker_test.go (1)
276-277: ⚡ Quick winRedundant error unwrapping: use either
errors.Causeorerrors.Is, not both.
errors.Causefrompingcap/errorsalready unwraps to the root error, so applyingstderrors.Ison top is redundant. Choose one approach:
- Option 1:
errors.Cause(err) == context.DeadlineExceeded- Option 2:
errors.Is(err, context.DeadlineExceeded)♻️ Proposed refactor (Option 1)
- if err != nil && (stderrors.Is(errors.Cause(err), context.DeadlineExceeded) || - stderrors.Is(errors.Cause(err), context.Canceled) || + if err != nil && (errors.Cause(err) == context.DeadlineExceeded || + errors.Cause(err) == context.Canceled ||🤖 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 276 - 277, The condition is redundantly unwrapping errors by calling errors.Cause(err) and then stderrors.Is; replace that pattern with a single standard check using stderrors.Is(err, context.DeadlineExceeded) and stderrors.Is(err, context.Canceled) (or alternatively use errors.Cause(err) == context.DeadlineExceeded if you prefer exact root comparison), i.e., update the conditional that references errors.Cause(err) and stderrors.Is so it directly calls stderrors.Is with err for both DeadlineExceeded and Canceled checks.pkg/sink/codec/csv/csv_decoder.go (1)
113-113: ⚡ Quick winRedundant error unwrapping: use either
errors.Causeorerrors.Is, not both.
errors.Causefrompingcap/errorsalready unwraps to the root error, so applyingerrors.Ison top is redundant. Choose one approach:
- Option 1:
errors.Cause(err) == io.EOF- Option 2:
errors.Is(err, io.EOF)♻️ Proposed refactor (Option 1)
- if errors.Is(errors.Cause(err), io.EOF) { + if errors.Cause(err) == io.EOF {🤖 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 unwrapping in csv_decoder.go where you currently call errors.Is(errors.Cause(err), io.EOF); use one approach — prefer idiomatic errors.Is — so change the check to errors.Is(err, io.EOF). Update the conditional that inspects the variable err (the EOF check) to call errors.Is(err, io.EOF) and remove the use of errors.Cause.pkg/upstream/upstream.go (1)
140-146: ⚡ Quick winConsider simplifying the single-case switch to an if statement.
A switch with only one case is less idiomatic than a simple if statement.
♻️ Proposed refactor
func isCreateTiStoreRetryable(ctx context.Context, err error) bool { - switch { - case errors.Is(errors.Cause(err), context.Canceled): + if stderrors.Is(errors.Cause(err), context.Canceled) { return ctx.Err() == nil } return true }Note: You'll also need to ensure
stderrorsis imported as shown in other functions in this PR.🤖 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/upstream/upstream.go` around lines 140 - 146, Simplify the single-case switch in isCreateTiStoreRetryable by replacing it with an if: check if errors.Is(errors.Cause(err), context.Canceled) and if so return ctx.Err() == nil, otherwise return true; also ensure the same error package alias used elsewhere in the PR (stderrors) is imported/used consistently for errors.Cause/Is so imports compile.pkg/pdutil/api_client.go (1)
190-196: ⚡ Quick winConsider simplifying the single-case switch to an if statement.
A switch with only one case is less idiomatic than a simple if statement. The same pattern appears at lines 367-373.
♻️ Proposed refactor
- retry.WithIsRetryableErr(func(err error) bool { - switch { - case stderrors.Is(errors.Cause(err), context.Canceled): - return false - } - return true - })) + retry.WithIsRetryableErr(func(err error) bool { + if stderrors.Is(errors.Cause(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 - 196, Replace the single-case switch used inside the retry.WithIsRetryableErr lambda with a simple if statement to make the code more idiomatic: in the anonymous function passed to retry.WithIsRetryableErr, check if errors.Cause(err) is context.Canceled using an if and return false in that branch, otherwise return true; apply the same change to the similar occurrence around lines 367-373 so both usages of retry.WithIsRetryableErr use an if instead of a switch.
🤖 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 `@cmd/kafka-consumer/consumer.go`:
- Around line 49-53: The current anonymous function discards the boolean result
of errors.As and can return a zero-value kafka.Error; change the check to
explicitly test errors.As success: declare var target kafka.Error, call ok :=
errors.As(err, &target), and only call target.Code() when ok is true (i.e., use
if ok && target.Code() == kafka.ErrTransport { ... }) so you don't call .Code()
on a zero-value; update the conditional around err handling in consumer.go where
the anonymous function is used.
In `@cmd/oauth2-server/main.go`:
- Around line 155-156: The handler for "/.well-known/openid-configuration"
currently calls fmt.Fprintf(w, openIDConfiguration, serverConfig.port,
serverConfig.port, serverConfig.port) before w.WriteHeader(200), which means the
status is sent implicitly and WriteHeader is ignored; change the order to set
the Content-Type header (w.Header().Set("Content-Type","application/json")) and
call w.WriteHeader(200) before writing the body, or simply set Content-Type and
then write the body (since 200 is default), updating the code around the
fmt.Fprintf and w.WriteHeader calls in that handler.
In `@pkg/sink/mysql/helper.go`:
- Around line 74-76: The current code treats any *dmysql.MySQLError as a
non-error by returning (false, nil); instead, only suppress the specific
"unknown system variable" error—mirror the check in
mysql_writer_for_active_active_sync_stats.go by asserting
stderrors.As(errors.Cause(err), &mysqlErr) and then checking mysqlErr.Number for
the unknown-variable constant (e.g. dmysql.ER_UNKNOWN_SYSTEM_VARIABLE or the
same numeric code used in the other file); only return (false, nil) for that
code, and for any other MySQL error return (false, err) so real errors (auth,
privilege, syntax) are propagated.
In `@tests/integration_tests/util/db.go`:
- Around line 156-158: The current check uses && which can never be true for a
single error; change the logic so real failures are not suppressed: detect err
!= nil and then fatal unless the underlying cause is context.DeadlineExceeded or
context.Canceled. Concretely, update the condition around
stderrors.Is(errors.Cause(err), context.DeadlineExceeded) and
stderrors.Is(errors.Cause(err), context.Canceled) to use || inside a negation
(or equivalent inverted logic) so that log.S().Fatal(err) runs for non-context
cancellation/deadline errors.
---
Outside diff comments:
In `@pkg/orchestrator/batch_test.go`:
- Around line 28-35: The test closures capture the loop variable i incorrectly,
so rebind i per iteration by adding a new local binding (i := i) immediately
inside each for loop before constructing the SingleDataPatch/closure; update
both loops that create patches (the one that builds patches with
SingleDataPatch{ Key: util.NewEtcdKey(fmt.Sprintf("/key%d", i)), Func: ... } and
the second loop around lines 62) so each closure captures the correct iteration
value used later by getBatchChangedState and the assertions.
In `@pkg/sink/mysql/helper.go`:
- Around line 193-199: The current retry logic decodes the password and retries
on any MySQL error; limit this to only access-denied errors by checking the
MySQL error number before attempting a base64 decode and retry. In the block
that inspects errors.Cause(err) into mysqlErr (dmysql.MySQLError), add a
condition that mysqlErr.Number equals the access-denied code (1045) and only
then perform base64.StdEncoding.DecodeString(password), set dbConfig.Passwd and
call CreateMysqlDBConn(dbConfig.FormatDSN()); leave other errors untouched so
they propagate normally.
---
Nitpick comments:
In `@pkg/orchestrator/etcd_worker_test.go`:
- Around line 276-277: The condition is redundantly unwrapping errors by calling
errors.Cause(err) and then stderrors.Is; replace that pattern with a single
standard check using stderrors.Is(err, context.DeadlineExceeded) and
stderrors.Is(err, context.Canceled) (or alternatively use errors.Cause(err) ==
context.DeadlineExceeded if you prefer exact root comparison), i.e., update the
conditional that references errors.Cause(err) and stderrors.Is so it directly
calls stderrors.Is with err for both DeadlineExceeded and Canceled checks.
In `@pkg/pdutil/api_client.go`:
- Around line 190-196: Replace the single-case switch used inside the
retry.WithIsRetryableErr lambda with a simple if statement to make the code more
idiomatic: in the anonymous function passed to retry.WithIsRetryableErr, check
if errors.Cause(err) is context.Canceled using an if and return false in that
branch, otherwise return true; apply the same change to the similar occurrence
around lines 367-373 so both usages of retry.WithIsRetryableErr use an if
instead of a switch.
In `@pkg/retry/retry_test.go`:
- Around line 68-70: The switch block currently uses redundant unwrapping
(stderrors.Is(errors.Cause(err), context.Canceled)); replace that with a single
check using the standard library: change to `case errors.Is(err,
context.Canceled):` (or alternatively `case errors.Cause(err) ==
context.Canceled:` if you prefer pingcap/errors equality), updating imports if
needed so only the chosen form remains.
In `@pkg/sink/codec/csv/csv_decoder.go`:
- Line 113: Replace the redundant unwrapping in csv_decoder.go where you
currently call errors.Is(errors.Cause(err), io.EOF); use one approach — prefer
idiomatic errors.Is — so change the check to errors.Is(err, io.EOF). Update the
conditional that inspects the variable err (the EOF check) to call
errors.Is(err, io.EOF) and remove the use of errors.Cause.
In `@pkg/sink/mysql/mysql_writer.go`:
- Line 264: The conditional is redundantly unwrapping the error by calling
errors.Cause(err) and then passing that to stderrors.Is; replace with a single
check using one approach: either compare the cause directly (errors.Cause(err)
== cerror.ErrMySQLDuplicateEntry) or use errors.Is on the original error
(stderrors.Is(err, cerror.ErrMySQLDuplicateEntry) or pingcap/errors.Is if
available). Update the conditional that currently uses errors.Cause(err) and
stderrors.Is to use one of these options consistently (refer to errors.Cause,
stderrors.Is, and cerror.ErrMySQLDuplicateEntry in mysql_writer.go) and remove
the redundant unwrap.
In `@pkg/upstream/upstream.go`:
- Around line 140-146: Simplify the single-case switch in
isCreateTiStoreRetryable by replacing it with an if: check if
errors.Is(errors.Cause(err), context.Canceled) and if so return ctx.Err() ==
nil, otherwise return true; also ensure the same error package alias used
elsewhere in the PR (stderrors) is imported/used consistently for
errors.Cause/Is so imports compile.
🪄 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: 18517b86-ae68-42b7-ace5-caa2b5925be9
📒 Files selected for processing (108)
cmd/kafka-consumer/consumer.gocmd/kafka-consumer/writer.gocmd/oauth2-server/main.gocmd/pulsar-consumer/consumer.gocmd/storage-consumer/main.gocoordinator/changefeed/backoff_test.gocoordinator/changefeed/etcd_backend_test.gocoordinator/controller.gocoordinator/coordinator_test.godownstreamadapter/dispatcher/basic_dispatcher.godownstreamadapter/dispatcher/event_dispatcher.godownstreamadapter/dispatchermanager/dispatcher_manager.godownstreamadapter/dispatchermanager/dispatcher_manager_helper.godownstreamadapter/eventcollector/dispatcher_stat_test.godownstreamadapter/eventcollector/event_collector.godownstreamadapter/routing/ddl_query_rewriter_test.godownstreamadapter/routing/router_apply_test.godownstreamadapter/routing/router_supported_ddl_test.godownstreamadapter/sink/redo/meta.godownstreamadapter/sink/redo/sink.gologservice/eventstore/event_store.gologservice/logpuller/region_request_worker.gologservice/logpuller/subscription_client.gologservice/schemastore/disk_format.gologservice/schemastore/multi_version_test.gologservice/schemastore/persist_storage_test.gologservice/schemastore/schema_store_test.gomaintainer/barrier.gomaintainer/operator/operator_merge.gomaintainer/operator/operator_split.gomaintainer/range_checker/table_span_range_checker.gomaintainer/replica/split_span_checker.gomaintainer/scheduler/balance_splits.gomaintainer/scheduler/drain_test.gopkg/applier/redo.gopkg/binlog-filter/filter_test.gopkg/common/format.gopkg/common/format_test.gopkg/common/helper_test.gopkg/common/span_op.gopkg/common/table_info.gopkg/common/table_info_helper.gopkg/common/table_info_shared_schema_guard_test.gopkg/common/table_info_test.gopkg/common/table_name_test.gopkg/config/sink.gopkg/diff/checkpoint.gopkg/diff/diff.gopkg/diff/util.gopkg/errors/helper.gopkg/errors/utils.gopkg/etcd/client.gopkg/etcd/etcd.gopkg/eventservice/event_broker.gopkg/eventservice/event_service_test.gopkg/eventservice/scan_window.gopkg/logger/log.gopkg/messaging/message.gopkg/messaging/message_center.gopkg/messaging/remote_target.gopkg/notify/notify_test.gopkg/orchestrator/batch_test.gopkg/orchestrator/etcd_worker_test.gopkg/pdutil/api_client.gopkg/pdutil/api_client_test.gopkg/redo/reader/file.gopkg/redo/reader/reader.gopkg/redo/reader/reader_test.gopkg/retry/retry_test.gopkg/scheduler/replica/replication.gopkg/security/sasl_test.gopkg/security/scram_client.gopkg/sink/codec/common/config.gopkg/sink/codec/common/helper.gopkg/sink/codec/csv/csv_decoder.gopkg/sink/codec/csv/csv_message.gopkg/sink/codec/csv/csv_message_test.gopkg/sink/kafka/options_test.gopkg/sink/kafka/sarama_config_test.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_dml_test.gopkg/sink/mysql/mysql_writer_for_active_active_sync_stats.gopkg/sink/mysql/mysql_writer_for_syncpoint.gopkg/sink/mysql/mysql_writer_test.gopkg/sink/mysql/sql_builder.gopkg/tcpserver/tcp_server.gopkg/upstream/upstream.gopkg/version/check_test.gopkg/workerpool/async_pool_test.goserver/module_election.goserver/server.gotests/integration_tests/api_v2/cases.gotests/integration_tests/bank/case.gotests/integration_tests/cdc/cdc.gotests/integration_tests/cdc/dailytest/case.gotests/integration_tests/cdc/dailytest/parser.gotests/integration_tests/complex_transaction/workload.gotests/integration_tests/default_value/main.gotests/integration_tests/many_pk_or_uk/main.gotests/integration_tests/multi_source/main.gotests/integration_tests/resolve_lock/main.gotests/integration_tests/util/config.gotests/integration_tests/util/db.goutils/chann/unlimited_chann.goutils/threadpool/thread_pool_test.go
💤 Files with no reviewable changes (15)
- pkg/common/table_info_shared_schema_guard_test.go
- pkg/sink/mysql/mysql_writer_dml_test.go
- coordinator/changefeed/backoff_test.go
- pkg/common/table_info_test.go
- downstreamadapter/routing/router_supported_ddl_test.go
- downstreamadapter/routing/ddl_query_rewriter_test.go
- pkg/common/table_name_test.go
- downstreamadapter/routing/router_apply_test.go
- pkg/common/format_test.go
- pkg/common/helper_test.go
- downstreamadapter/eventcollector/dispatcher_stat_test.go
- maintainer/scheduler/drain_test.go
- pkg/sink/kafka/sarama_config_test.go
- pkg/notify/notify_test.go
- pkg/security/sasl_test.go
✅ Files skipped from review due to trivial changes (32)
- pkg/common/span_op.go
- pkg/common/table_info.go
- pkg/version/check_test.go
- tests/integration_tests/cdc/dailytest/parser.go
- server/server.go
- pkg/config/sink.go
- maintainer/operator/operator_split.go
- pkg/sink/mysql/mysql_writer_for_syncpoint.go
- maintainer/barrier.go
- tests/integration_tests/api_v2/cases.go
- pkg/scheduler/replica/replication.go
- pkg/sink/mysql/mysql_writer_test.go
- pkg/messaging/remote_target.go
- pkg/diff/diff.go
- pkg/diff/checkpoint.go
- maintainer/operator/operator_merge.go
- utils/threadpool/thread_pool_test.go
- coordinator/changefeed/etcd_backend_test.go
- pkg/sink/codec/csv/csv_message_test.go
- pkg/sink/mysql/sql_builder.go
- cmd/kafka-consumer/writer.go
- pkg/redo/reader/reader_test.go
- tests/integration_tests/util/config.go
- pkg/common/table_info_helper.go
- downstreamadapter/dispatchermanager/dispatcher_manager.go
- pkg/sink/codec/csv/csv_message.go
- tests/integration_tests/cdc/dailytest/case.go
- pkg/binlog-filter/filter_test.go
- pkg/eventservice/scan_window.go
- logservice/eventstore/event_store.go
- pkg/eventservice/event_service_test.go
- maintainer/range_checker/table_span_range_checker.go
Comment on lines
156
to
158
| if err != nil && stderrors.Is(errors.Cause(err), context.DeadlineExceeded) && stderrors.Is(errors.Cause(err), context.Canceled) { | ||
| log.S().Fatal(err) | ||
| } |
Contributor
There was a problem hiding this comment.
Fix impossible context-error condition (currently suppresses real failures).
At Line 156, the check uses A && B for DeadlineExceeded and Canceled. A single error won’t match both, so Fatal is effectively never reached and unexpected exec errors get ignored.
Proposed fix
- if err != nil && stderrors.Is(errors.Cause(err), context.DeadlineExceeded) && stderrors.Is(errors.Cause(err), context.Canceled) {
+ if err != nil &&
+ !stderrors.Is(errors.Cause(err), context.DeadlineExceeded) &&
+ !stderrors.Is(errors.Cause(err), context.Canceled) {
log.S().Fatal(err)
}📝 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
| if err != nil && stderrors.Is(errors.Cause(err), context.DeadlineExceeded) && stderrors.Is(errors.Cause(err), context.Canceled) { | |
| log.S().Fatal(err) | |
| } | |
| if err != nil && | |
| !stderrors.Is(errors.Cause(err), context.DeadlineExceeded) && | |
| !stderrors.Is(errors.Cause(err), context.Canceled) { | |
| log.S().Fatal(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/util/db.go` around lines 156 - 158, The current check
uses && which can never be true for a single error; change the logic so real
failures are not suppressed: detect err != nil and then fatal unless the
underlying cause is context.DeadlineExceeded or context.Canceled. Concretely,
update the condition around stderrors.Is(errors.Cause(err),
context.DeadlineExceeded) and stderrors.Is(errors.Cause(err), context.Canceled)
to use || inside a negation (or equivalent inverted logic) so that
log.S().Fatal(err) runs for non-context cancellation/deadline errors.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/integration_tests/util/db.go (1)
156-158:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winImpossible
&&condition still suppresses real exec failures.The matcher was switched from
==tocerror.Is(...)(likely to satisfyerrorlint), but the underlying logic bug is unchanged: a single error cannot be bothcontext.DeadlineExceededandcontext.Canceled, so the predicate is always false andlog.S().Fatal(err)is dead. Any non-context error fromconn.ExecContextis silently swallowed.The intent is to fatal on unexpected errors and tolerate context cancellation/deadline, i.e., invert the checks and join with
&&:Proposed fix
- if err != nil && cerror.Is(errors.Cause(err), context.DeadlineExceeded) && cerror.Is(errors.Cause(err), context.Canceled) { - log.S().Fatal(err) - } + if err != nil && + !cerror.Is(errors.Cause(err), context.DeadlineExceeded) && + !cerror.Is(errors.Cause(err), context.Canceled) { + log.S().Fatal(err) + }Also worth confirming that
cerror.Is(fromgithub.com/pingcap/ticdc/pkg/errors) correctly matches stdlib sentinel errors likecontext.DeadlineExceeded/context.Canceledaftererrors.Causeunwrapping — if it's not a transparent wrapper overstderrors.Is, this may not behave as==did previously.#!/bin/bash # Verify how cerror.Is is implemented in pingcap/ticdc/pkg/errors, # to confirm it works with stdlib sentinel errors like context.DeadlineExceeded. fd -t f -e go . pkg/errors | head -50 ast-grep --pattern 'func Is($_, $_) $_ { $$$ }' rg -nP --type=go -C3 '^\s*(func|var)\s+Is\b' -g 'pkg/errors/**'🤖 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/util/db.go` around lines 156 - 158, The condition currently uses cerror.Is(errors.Cause(err), context.DeadlineExceeded) && cerror.Is(errors.Cause(err), context.Canceled) which is always false and hides real errors; change the logic to fatal on any non-nil err except when the cause is context.DeadlineExceeded or context.Canceled, e.g. if err != nil && ! (cerror.Is(errors.Cause(err), context.DeadlineExceeded) || cerror.Is(errors.Cause(err), context.Canceled)) { log.S().Fatal(err) }; ensure you update the conditional around the conn.ExecContext error handling and keep errors.Cause and cerror.Is usage, and optionally add a quick check that cerror.Is in pkg/errors correctly matches stdlib sentinel errors after unwrapping.
🧹 Nitpick comments (2)
pkg/etcd/client.go (1)
143-143: 💤 Low valueConsider simplifying error unwrapping.
The combination of
errors.Causewithcerror.Isis redundant sincecerror.Isalready unwraps error chains internally (similar to the standarderrors.Is).♻️ Proposed simplification
- if err != nil && !cerror.Is(errors.Cause(err), context.Canceled) { + if err != nil && !cerror.Is(err, context.Canceled) {🤖 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` at line 143, The conditional uses errors.Cause together with cerror.Is redundantly; change the check to let cerror.Is handle unwrapping directly (i.e., replace the use of errors.Cause(err) with err) so the condition becomes: if err != nil && !cerror.Is(err, context.Canceled); update the occurrence in pkg/etcd/client.go where the variable err is tested with cerror.Is and errors.Cause to remove errors.Cause and pass err directly.logservice/logpuller/region_request_worker.go (1)
108-108: 💤 Low valueSimplify the wrapped-error check pattern.
The pattern
cerror.Is(errors.Cause(err), context.Canceled)is redundant becauseerrors.Cause()unwraps to the root error, thencerror.Is()walks the error chain again. Sincecerror.Is()already traverses the entire error chain, you can simplify this tocerror.Is(err, context.Canceled)directly.This same redundancy appears in multiple files across this PR (e.g., pkg/orchestrator/etcd_worker_test.go:275-276, pkg/workerpool/async_pool_test.go:128, pkg/sink/mysql/mysql_writer.go:263, pkg/logger/log.go:338), while pkg/messaging/message_center.go:476 correctly uses
pkgerror.Is(err, ...)withouterrors.Cause(). Consider applying the simpler pattern consistently throughout.♻️ Proposed refactor
- if cerror.Is(errors.Cause(err), context.Canceled) { + if cerror.Is(err, context.Canceled) {🤖 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 `@logservice/logpuller/region_request_worker.go` at line 108, Replace redundant wrapped-error checks that call errors.Cause before cerror.Is with a direct cerror.Is(err, context.Canceled) call; locate instances like the check in region_request_worker.go (the if using cerror.Is(errors.Cause(err), context.Canceled)) and change them to cerror.Is(err, context.Canceled). Apply the same simplification to other occurrences (e.g., tests and writers) so all checks use cerror.Is directly on the original error value rather than errors.Cause(err).
🤖 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.
Duplicate comments:
In `@tests/integration_tests/util/db.go`:
- Around line 156-158: The condition currently uses cerror.Is(errors.Cause(err),
context.DeadlineExceeded) && cerror.Is(errors.Cause(err), context.Canceled)
which is always false and hides real errors; change the logic to fatal on any
non-nil err except when the cause is context.DeadlineExceeded or
context.Canceled, e.g. if err != nil && ! (cerror.Is(errors.Cause(err),
context.DeadlineExceeded) || cerror.Is(errors.Cause(err), context.Canceled)) {
log.S().Fatal(err) }; ensure you update the conditional around the
conn.ExecContext error handling and keep errors.Cause and cerror.Is usage, and
optionally add a quick check that cerror.Is in pkg/errors correctly matches
stdlib sentinel errors after unwrapping.
---
Nitpick comments:
In `@logservice/logpuller/region_request_worker.go`:
- Line 108: Replace redundant wrapped-error checks that call errors.Cause before
cerror.Is with a direct cerror.Is(err, context.Canceled) call; locate instances
like the check in region_request_worker.go (the if using
cerror.Is(errors.Cause(err), context.Canceled)) and change them to
cerror.Is(err, context.Canceled). Apply the same simplification to other
occurrences (e.g., tests and writers) so all checks use cerror.Is directly on
the original error value rather than errors.Cause(err).
In `@pkg/etcd/client.go`:
- Line 143: The conditional uses errors.Cause together with cerror.Is
redundantly; change the check to let cerror.Is handle unwrapping directly (i.e.,
replace the use of errors.Cause(err) with err) so the condition becomes: if err
!= nil && !cerror.Is(err, context.Canceled); update the occurrence in
pkg/etcd/client.go where the variable err is tested with cerror.Is and
errors.Cause to remove errors.Cause and pass err directly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c08511a7-7016-4c15-97b4-23a3fb5e20d4
📒 Files selected for processing (33)
.github/workflows/pr_build_and_test.yamlcmd/storage-consumer/main.gocoordinator/coordinator_test.gologservice/logpuller/region_request_worker.gologservice/logpuller/subscription_client.gologservice/schemastore/disk_format.gologservice/schemastore/multi_version_test.gologservice/schemastore/persist_storage_test.gopkg/etcd/client.gopkg/logger/log.gopkg/messaging/message_center.gopkg/orchestrator/etcd_worker_test.gopkg/pdutil/api_client.gopkg/redo/reader/file.gopkg/retry/retry_test.gopkg/sink/codec/common/helper.gopkg/sink/kafka/options_test.gopkg/sink/mysql/helper.gopkg/sink/mysql/mysql_writer.gopkg/sink/mysql/mysql_writer_dml_exec.gopkg/sink/mysql/mysql_writer_for_active_active_sync_stats.gopkg/tcpserver/tcp_server.gopkg/txnutil/gc/gc_manager_nextgen_test.gopkg/txnutil/gc/gc_manager_test.gopkg/workerpool/async_pool_test.gotests/integration_tests/bank/case.gotests/integration_tests/cdc/cdc.gotests/integration_tests/complex_transaction/workload.gotests/integration_tests/default_value/main.gotests/integration_tests/many_pk_or_uk/main.gotests/integration_tests/multi_source/main.gotests/integration_tests/resolve_lock/main.gotests/integration_tests/util/db.go
🚧 Files skipped from review as they are similar to previous changes (9)
- logservice/schemastore/disk_format.go
- logservice/logpuller/subscription_client.go
- logservice/schemastore/multi_version_test.go
- .github/workflows/pr_build_and_test.yaml
- logservice/schemastore/persist_storage_test.go
- tests/integration_tests/bank/case.go
- pkg/sink/mysql/helper.go
- pkg/redo/reader/file.go
- coordinator/coordinator_test.go
|
[FORMAT CHECKER NOTIFICATION] Notice: To remove the 📖 For more info, you can check the "Contribute Code" section in the development guide. |
|
@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. |
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
1 participant
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.
What problem does this PR solve?
Issue Number: close #xxx
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
Summary by CodeRabbit