*: fix golangci-lint auto-fixable issues (PR 2 of 4)#5033
Open
3AceShowHand wants to merge 6 commits into
Open
Conversation
…e bugs Auto-fixed by `golangci-lint run --fix -D errorlint`: - copyloopvar: remove redundant "x := x" in Go 1.22+ loop variables - misspell: fix typos in comments and strings - nosprintfhostport: use net.JoinHostPort instead of Sprintf Manual fixes: - ineffassign (2): remove dead assignments in consumer.go, drop_event.go - nilerr (3): return err instead of nil in file.go, mysql_writer_ddl.go, pool_test.go - durationcheck (1): avoid multiplying two time.Duration values in sync_point.go Skipped in this PR (false positives or need more context): - unconvert: platform-dependent type conversions - govet: testinggoroutine, copylocks need design decisions - errname: renaming error types touches 8+ files - remaining ineffassign: need verification
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
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/XL
![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 performs extensive code cleanup and optimizations, including refactoring loops and conditionals into more idiomatic Go structures, replacing inefficient string formatting with fmt.Fprintf, and fixing various typos in comments and error messages. It also addresses several bugs where errors were being swallowed instead of returned, specifically in pkg/redo/writer/file/file.go, pkg/sink/mysql/mysql_writer_ddl.go, and pkg/workerpool/pool_test.go. Feedback was provided to improve log consistency by using the standard commitTs key instead of commits in maintainer/barrier.go.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (24)
💤 Files with no reviewable changes (15)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughLarge refactor across many files: remove test loop shadowing, adopt fmt.Fprintf for builder writes, simplify loops and if/else chains to switches, inline boolean expressions, use promoted fields/getters, apply targeted bug fixes, and correct comments/tests. ChangesCore Refactor: Loops, Control Flow, and String Formatting
Bug Fixes and Behavioral Logic Changes
Comment and Documentation Corrections
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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🔥 ProblemsTimed out fetching pipeline failures after 30000ms 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 |
![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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/storage-consumer/consumer.go (1)
564-579:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: Restore the schema fallback for RENAME TABLE without explicit schema.
When a
RENAME TABLEstatement omits the schema (e.g.,RENAME TABLE t1 TO t2), the TiDB parser setsoldTable.Schema.Oto an empty string. The current code leavesschemaNameempty in this case, but it should default totableDef.Schema(the current schema context).This breaks the old-table key used for DDL watermark tracking (line 588-590), causing incorrect filtering of DML files and potential data corruption.
🐛 Proposed fix to restore schema fallback
- var schemaName, tableName string stmt, err := parser.New().ParseOneStmt(tableDef.Query, "", "") if err != nil { log.Panic("parse statement failed", zap.Any("DDL", tableDef.Query), zap.Error(err)) } // The query in job maybe "RENAME TABLE table1 to table2" renameStmt, ok := stmt.(*ast.RenameTableStmt) if !ok || len(renameStmt.TableToTables) == 0 { log.Panic("invalid rename table statement", zap.Any("DDL", tableDef.Query)) } oldTable := renameStmt.TableToTables[0].OldTable + schemaName := tableDef.Schema if oldTable.Schema.O != "" { schemaName = oldTable.Schema.O } - tableName = oldTable.Name.O + tableName := oldTable.Name.O return commonType.QuoteSchema(schemaName, tableName), 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 `@cmd/storage-consumer/consumer.go` around lines 564 - 579, The code handling RENAME TABLE uses parser.New().ParseOneStmt and extracts oldTable := renameStmt.TableToTables[0].OldTable but currently leaves schemaName empty when oldTable.Schema.O == "", which breaks DDL watermark keys; modify the logic around oldTable.Schema.O to set schemaName = tableDef.Schema when oldTable.Schema.O is empty (otherwise keep oldTable.Schema.O), so that the value passed into commonType.QuoteSchema(schemaName, tableName) correctly falls back to the current tableDef.Schema for unqualified RENAMEs.
🧹 Nitpick comments (2)
tests/integration_tests/many_pk_or_uk/main.go (1)
111-112: ⚡ Quick winLeftover
c := cis also redundant under Go 1.22+ and inconsistent with thecopyloopvarcleanup.This PR removes
pkOrUK := pkOrUKto satisfycopyloopvar, but the sibling reassignmentc := con Line 112 is the same anti-pattern and should be removed for the same reason (each iteration already gets fresh loop variables in Go 1.22+). Leaving one and not the other is inconsistent with the PR's stated goal and may still be flagged bycopyloopvardepending on configuration.♻️ Proposed fix
for j, pkOrUK := range []string{"UNIQUE NOT NULL", "PRIMARY KEY"} { g.Add(1) tableName := fmt.Sprintf("pk_or_uk_%d_%d", i, j) - - c := c go func() {🤖 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/many_pk_or_uk/main.go` around lines 111 - 112, Remove the redundant loop-local reassignment "c := c" (the same anti-pattern already fixed for "pkOrUK := pkOrUK") so the loop relies on Go 1.22+'s fresh iteration variables; locate the loop that contains the "c := c" statement (and the previously removed "pkOrUK := pkOrUK") and simply delete the "c := c" line to avoid the copyloopvar warning and keep the code consistent.pkg/security/scram_client.go (1)
40-40: 💤 Low valueConsider reverting to explicit embedded field access for clarity.
These changes rely on Go's method promotion from embedded fields (
x.NewClient(...)→x.HashGeneratorFcn.NewClient(...)andx.NewConversation()→x.Client.NewConversation()). While technically correct, the changes:
- Are not mentioned in the PR description (don't match copyloopvar, misspell, nosprintfhostport, ineffassign, nilerr, or durationcheck)
- Reduce code clarity by making it less obvious which embedded field's method is being invoked
- Make the code slightly more fragile to future library changes
The explicit form is more maintainable and self-documenting.
Revert to explicit form
- x.Client, err = x.NewClient(userName, password, authzID) + x.Client, err = x.HashGeneratorFcn.NewClient(userName, password, authzID) if err != nil { return err } - x.ClientConversation = x.NewConversation() + x.ClientConversation = x.Client.NewConversation() return nilAlso applies to: 44-44
🤖 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/security/scram_client.go` at line 40, The review recommends reverting to explicit embedded field access to improve clarity: replace implicit promoted calls like x.NewClient(userName, password, authzID) and x.NewConversation() with the explicit receiver field calls x.HashGeneratorFcn.NewClient(userName, password, authzID) and x.Client.NewConversation() respectively so it's clear which embedded type's method is invoked; update occurrences in scram_client.go (e.g., where x.NewClient and x.NewConversation are used) to call the embedded field methods directly and run tests/build to ensure no breakage.
🤖 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/oauth2-server/main.go`:
- Around line 155-156: The response status is being set after writing the body;
in the handler that calls fmt.Fprintf(w, openIDConfiguration, serverConfig.port,
serverConfig.port, serverConfig.port) move the call to
w.WriteHeader(http.StatusOK) so it executes before the fmt.Fprintf, and replace
the literal 200 with the http.StatusOK constant to ensure the status is sent
correctly; update the block that references fmt.Fprintf and w.WriteHeader
accordingly.
In `@maintainer/barrier.go`:
- Line 470: The log field name is inconsistent: change the zap.Uint64("commits",
be.commitTs) call to use "commitTs" to match other entries; locate the
zap.Uint64 invocation that references be.commitTs (in barrier.go) and replace
the first argument "commits" with "commitTs" so logging keys are consistent
across functions using be.commitTs.
---
Outside diff comments:
In `@cmd/storage-consumer/consumer.go`:
- Around line 564-579: The code handling RENAME TABLE uses
parser.New().ParseOneStmt and extracts oldTable :=
renameStmt.TableToTables[0].OldTable but currently leaves schemaName empty when
oldTable.Schema.O == "", which breaks DDL watermark keys; modify the logic
around oldTable.Schema.O to set schemaName = tableDef.Schema when
oldTable.Schema.O is empty (otherwise keep oldTable.Schema.O), so that the value
passed into commonType.QuoteSchema(schemaName, tableName) correctly falls back
to the current tableDef.Schema for unqualified RENAMEs.
---
Nitpick comments:
In `@pkg/security/scram_client.go`:
- Line 40: The review recommends reverting to explicit embedded field access to
improve clarity: replace implicit promoted calls like x.NewClient(userName,
password, authzID) and x.NewConversation() with the explicit receiver field
calls x.HashGeneratorFcn.NewClient(userName, password, authzID) and
x.Client.NewConversation() respectively so it's clear which embedded type's
method is invoked; update occurrences in scram_client.go (e.g., where
x.NewClient and x.NewConversation are used) to call the embedded field methods
directly and run tests/build to ensure no breakage.
In `@tests/integration_tests/many_pk_or_uk/main.go`:
- Around line 111-112: Remove the redundant loop-local reassignment "c := c"
(the same anti-pattern already fixed for "pkOrUK := pkOrUK") so the loop relies
on Go 1.22+'s fresh iteration variables; locate the loop that contains the "c :=
c" statement (and the previously removed "pkOrUK := pkOrUK") and simply delete
the "c := c" line to avoid the copyloopvar warning and keep the code consistent.
🪄 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: c80aa2fb-a37b-4ab7-8330-21add2204e0f
📒 Files selected for processing (78)
cmd/kafka-consumer/writer.gocmd/oauth2-server/main.gocmd/pulsar-consumer/consumer.gocmd/storage-consumer/consumer.gocoordinator/changefeed/backoff_test.gocoordinator/changefeed/etcd_backend_test.gocoordinator/controller.godownstreamadapter/dispatcher/basic_dispatcher.godownstreamadapter/dispatcher/event_dispatcher.godownstreamadapter/dispatchermanager/dispatcher_manager.godownstreamadapter/dispatchermanager/dispatcher_manager_helper.godownstreamadapter/eventcollector/dispatcher_stat_test.godownstreamadapter/routing/ddl_query_rewriter_test.godownstreamadapter/routing/router_apply_test.godownstreamadapter/routing/router_supported_ddl_test.godownstreamadapter/sink/redo/meta.godownstreamadapter/syncpoint/sync_point.gologservice/eventstore/event_store.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/event/drop_event.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/etcd/client.gopkg/eventservice/event_broker.gopkg/eventservice/event_service_test.gopkg/eventservice/scan_window.gopkg/messaging/message.gopkg/messaging/remote_target.gopkg/notify/notify_test.gopkg/orchestrator/batch_test.gopkg/pdutil/api_client_test.gopkg/redo/reader/reader_test.gopkg/redo/writer/file/file.gopkg/scheduler/replica/replication.gopkg/security/sasl_test.gopkg/security/scram_client.gopkg/sink/codec/common/config.gopkg/sink/codec/csv/csv_message.gopkg/sink/codec/csv/csv_message_test.gopkg/sink/kafka/sarama_config_test.gopkg/sink/mysql/mysql_writer_ddl.gopkg/sink/mysql/mysql_writer_dml_test.gopkg/sink/mysql/mysql_writer_for_syncpoint.gopkg/sink/mysql/mysql_writer_test.gopkg/sink/mysql/sql_builder.gopkg/upstream/upstream.gopkg/version/check_test.gopkg/workerpool/pool_test.goserver/server.gotests/integration_tests/api_v2/cases.gotests/integration_tests/cdc/dailytest/case.gotests/integration_tests/cdc/dailytest/parser.gotests/integration_tests/default_value/main.gotests/integration_tests/many_pk_or_uk/main.gotests/integration_tests/multi_source/main.gotests/integration_tests/util/config.goutils/chann/unlimited_chann.goutils/threadpool/thread_pool_test.go
💤 Files with no reviewable changes (1)
- pkg/common/event/drop_event.go
ti-chi-bot
Bot
added
do-not-merge/needs-triage-completed
and removed
do-not-merge/needs-linked-issue
labels
Collaborator
Author
|
/check-issue-triage-complete |
Collaborator
Author
|
/test all |
!(offset > p.watermarkOffset) is equivalent to offset <= p.watermarkOffset but the latter is cleaner and satisfies staticcheck QF1001.
The ineffassign linter flagged tableName := tableDef.Table because it is always overwritten before use. schemaName := tableDef.Schema must be kept as the default value — it is only conditionally overwritten. Instead of removing both initial values, only drop the dead tableName assignment and keep schemaName intact.
Collaborator
Author
|
/test all |
3AceShowHand
commented
May 12, 2026
3AceShowHand
commented
May 12, 2026
Collaborator
Author
|
/test all |
Collaborator
Author
|
/retest |
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.
Issue Number: close #5034
What problem does this PR solve?
Part of the 4-PR plan (tracked in #5032) to fix all golangci-lint issues. This PR fixes auto-fixable issues (
--fix) plus selected mechanical non-auto-fixable bugs.What is changed and how it works?
Auto-fixed (no manual changes):
copyloopvar(12): remove redundantx := xin Go 1.22+ loop variablesmisspell(13): fix typos in comments and stringsnosprintfhostport(auto): usenet.JoinHostPortinstead ofSprintfgovet(1): fixWriteHeaderorder in oauth2-server, usehttp.StatusOKconstantManual fixes:
ineffassign(2): remove dead assignments inconsumer.go,drop_event.gonilerr(3): returnerrinstead ofnilinfile.go,mysql_writer_ddl.go,pool_test.gopkg/redo/writer/file/file.go:flushAndRotateFilehas already flushed the local file, but external-storage mode still needsrotate()to rename/sync the file, upload it to external storage, and open the next file. Swallowing arotate()error would makeFlush()look successful and can trigger redoPostFlush()before the log is durable in external storage, so returning the error is the safer and correct behavior.pkg/sink/mysql/mysql_writer_ddl.go:isDDLExecutedguards non-idempotentExchangeTablePartitionDDLs. If the check fails, treating it as success can skip the actual DDL while later code may still advance ddl-ts metadata and callPostFlush(). Returning the error lets the existing DDL retry/error path preserve downstream correctness.durationcheck(1): avoid multiplying twotime.Durationvalues insync_point.go"commits"→"commitTs"inbarrier.goSkipped (false positives or need design):
errname: renaming error types touches 8+ files — deferred to follow-upgovet(remaining):testinggoroutine,copylocksneed design decisionsunconvert: platform-dependent type conversionsCheck List
Tests:
go build ./...passesCode changes:
pkg/errorsimports (deferred to PR 3)Related changes:
Release note
Summary by CodeRabbit
Release Notes
Bug Fixes
Refactor
Tests
Documentation