Open
Conversation
Signed-off-by: Jianjun Liao <jianjun.liao@outlook.com>
ti-chi-bot
bot
added
do-not-merge/needs-linked-issue
first-time-contributor
Contributor
📝 WalkthroughWalkthroughUpdated Go toolchain to 1.25.6, migrated external storage usage from Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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/XXL
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request primarily focuses on updating the project's core dependencies, most notably the Go language version and the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 TiDB dependency and other related dependencies. The changes primarily involve adapting to new APIs from the updated dependencies, especially the external storage API which moved from pingcap/tidb/br/pkg/storage to pingcap/tidb/pkg/objstore. The changes also include updates to the Go version, gRPC handling, and some DML/DDL decoding logic. The modifications appear to be correct and consistent with the dependency updates. The code migration to the new APIs seems to be thorough and correct across the codebase.
![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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
downstreamadapter/sink/redo/meta.go (1)
319-323:⚠️ Potential issue | 🔴 CriticalMove
redo.FixLocalScheme(uri)after the error check to prevent nil dereference.At line 320,
redo.FixLocalScheme(uri)is called before checking the error fromobjstore.ParseRawURLat line 321. If parsing fails,uriwill be nil, causing a panic whenFixLocalSchemeaccessesuri.Scheme. Additionally, wrap the error witherrors.Trace()per the coding guidelines for third-party library calls.Fix
if m.extStorage == nil { uri, err := objstore.ParseRawURL(util.GetOrZero(m.cfg.Storage)) - redo.FixLocalScheme(uri) if err != nil { - return err + return errors.Trace(err) } + redo.FixLocalScheme(uri) m.extStorage, err = redo.InitExternalStorage(ctx, *uri) if err != nil { return err } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@downstreamadapter/sink/redo/meta.go` around lines 319 - 323, Move the call to redo.FixLocalScheme(uri) so it runs after checking the error returned by objstore.ParseRawURL(util.GetOrZero(m.cfg.Storage)) to avoid a nil dereference; specifically, call objstore.ParseRawURL first, check if err != nil and return errors.Trace(err), then call redo.FixLocalScheme(uri). Ensure the returned error from ParseRawURL is wrapped with errors.Trace() per guidelines.pkg/config/consistent.go (1)
114-118:⚠️ Potential issue | 🟠 MajorPreserve parse error cause and mask storage URI.
Line 115-117: The error handling drops the original
ParseRawURLerror cause and exposes the raw storage URI in the error message, creating a credential/token leak risk. This violates the coding guideline requiring third-party errors to be wrapped withWrapError().Replace with error wrapping that preserves the cause:
🔧 Proposed fix
- uri, err := objstore.ParseRawURL(util.GetOrZero(c.Storage)) + storageURI := util.GetOrZero(c.Storage) + uri, err := objstore.ParseRawURL(storageURI) if err != nil { - return cerror.ErrInvalidReplicaConfig.GenWithStackByArgs( - fmt.Sprintf("invalid storage uri: %s", util.GetOrZero(c.Storage))) + return cerror.WrapError( + cerror.ErrInvalidReplicaConfig, + fmt.Errorf("invalid storage uri: %s: %w", util.MaskSensitiveDataInURI(storageURI), err), + ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/config/consistent.go` around lines 114 - 118, The current error handling for objstore.ParseRawURL drops the original parse error and prints the raw storage URI; instead wrap the original err using WrapError (preserving the cause) and avoid leaking credentials by masking the URI (e.g., do not include util.GetOrZero(c.Storage) verbatim). Update the error return for ParseRawURL failures around objstore.ParseRawURL/util.GetOrZero to call WrapError(err, cerror.ErrInvalidReplicaConfig) (or use the project WrapError helper) and produce a safe message that masks the storage URI (like "<redacted-storage-uri>") while preserving the original err as the cause.
🧹 Nitpick comments (1)
deployments/next-gen-local-integration-test.Dockerfile (1)
31-35: Pin the Go tarball checksum to improve supply-chain security.Go 1.25.6 is available and the SHA256 checksum can be obtained from go.dev. Consider adding checksum verification:
f022b6aad78e362bcba9b0b94d09ad58c5a70c6ba3b7582905fababf5fe0181aThis ensures the downloaded artifact matches the official release and reduces risk from tampering or CDN issues.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deployments/next-gen-local-integration-test.Dockerfile` around lines 31 - 35, Add SHA256 verification for the downloaded Go tarball: define a GOLANG_SHA256 variable (use the provided value f022b6aad78e362bcba9b0b94d09ad58c5a70c6ba3b7582905fababf5fe0181a) alongside GOLANG_VERSION/GOLANG_DOWNLOAD_URL and update the RUN that downloads golang.tar.gz to verify the checksum (e.g., echo "$GOLANG_SHA256 golang.tar.gz" | sha256sum -c -) and abort the build on mismatch before extracting; update references to GOLANG_DOWNLOAD_URL and GOLANG_VERSION where needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@downstreamadapter/sink/redo/meta.go`:
- Around line 98-107: The objstore.ParseRawURL call needs its error wrapped and
the subsequent "noop://" parse must not discard errors: change the first
ParseRawURL error return to return errors.Trace(err), and when handling the
blackhole case call objstore.ParseRawURL("noop://") into (newURI, err) and only
assign uri = newURI if err == nil; if parsing fails, return errors.Trace(err)
instead of silently keeping the stale uri; references: objstore.ParseRawURL,
util.GetOrZero, redo.FixLocalScheme, redo.IsBlackholeStorage, and use
errors.Trace(err) for wrapping.
In `@go.mod`:
- Around line 95-96: The go.mod replace rule for google.golang.org/grpc is
stale: the require block lists google.golang.org/grpc v1.77.0 but the replace
maps v1.67.1 => v1.65.0, so the replace has no effect; update the replace
directive to target v1.77.0 (e.g., replace google.golang.org/grpc v1.77.0 =>
<desired-version>) or remove the replace entry entirely if it's no longer
needed, ensuring the replace/require versions for google.golang.org/grpc are
consistent.
- Line 56: The go.mod currently pins github.com/pingcap/tidb to a vulnerable
commit (the require line containing github.com/pingcap/tidb
v1.1.0-beta.0.20260205093021-1eb3d0a8c7a9) — update the require entry for module
github.com/pingcap/tidb to v8.2.0 or later, remove any replace directives that
force the old revision, run `go mod tidy` to refresh go.sum, and ensure CI
passes; check for any code changes needed to accommodate TiDB v8.2.0 API changes
after updating the require in go.mod.
In `@logservice/logpuller/grpc_conn.go`:
- Line 30: The new import "google.golang.org/grpc/mem" and use of
experimental.WithBufferPool() require gRPC v1.66.0+ but the repo forces v1.65.0
via the go.mod replacement (google.golang.org/grpc v1.67.1 =>
google.golang.org/grpc v1.65.0); fix by either updating the go.mod replacement
to a gRPC version >= v1.66.0 (so the mem package and experimental.WithBufferPool
are available) OR revert the code in grpc_conn.go to the older buffer-pool API
(remove the mem import and replace experimental.WithBufferPool usage with the
pre-1.66 approach used previously in this codebase). Ensure you update only the
go.mod replacement or the grpc_conn.go references to mem and
experimental.WithBufferPool accordingly so imports and build align.
In `@pkg/redo/writer/factory/factory.go`:
- Around line 34-36: Wrap the error returned by objstore.ParseRawURL using
errors.Trace before returning and add the pingcap errors import; specifically,
in the block calling objstore.ParseRawURL(util.GetOrZero(lwCfg.Storage)) replace
the direct return of err with return nil, errors.Trace(err) and add an import
for "github.com/pingcap/errors" at the top of the file so the stack trace is
preserved.
---
Outside diff comments:
In `@downstreamadapter/sink/redo/meta.go`:
- Around line 319-323: Move the call to redo.FixLocalScheme(uri) so it runs
after checking the error returned by
objstore.ParseRawURL(util.GetOrZero(m.cfg.Storage)) to avoid a nil dereference;
specifically, call objstore.ParseRawURL first, check if err != nil and return
errors.Trace(err), then call redo.FixLocalScheme(uri). Ensure the returned error
from ParseRawURL is wrapped with errors.Trace() per guidelines.
In `@pkg/config/consistent.go`:
- Around line 114-118: The current error handling for objstore.ParseRawURL drops
the original parse error and prints the raw storage URI; instead wrap the
original err using WrapError (preserving the cause) and avoid leaking
credentials by masking the URI (e.g., do not include util.GetOrZero(c.Storage)
verbatim). Update the error return for ParseRawURL failures around
objstore.ParseRawURL/util.GetOrZero to call WrapError(err,
cerror.ErrInvalidReplicaConfig) (or use the project WrapError helper) and
produce a safe message that masks the storage URI (like
"<redacted-storage-uri>") while preserving the original err as the cause.
---
Nitpick comments:
In `@deployments/next-gen-local-integration-test.Dockerfile`:
- Around line 31-35: Add SHA256 verification for the downloaded Go tarball:
define a GOLANG_SHA256 variable (use the provided value
f022b6aad78e362bcba9b0b94d09ad58c5a70c6ba3b7582905fababf5fe0181a) alongside
GOLANG_VERSION/GOLANG_DOWNLOAD_URL and update the RUN that downloads
golang.tar.gz to verify the checksum (e.g., echo "$GOLANG_SHA256 golang.tar.gz"
| sha256sum -c -) and abort the build on mismatch before extracting; update
references to GOLANG_DOWNLOAD_URL and GOLANG_VERSION where needed.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (39)
README.mdcmd/storage-consumer/consumer.gocoordinator/coordinator_test.gocoordinator/ensure_gc_cleanup_async_test.gocoordinator/gccleaner/cleaner_test.godeployments/integration-test.Dockerfiledeployments/next-gen-local-integration-test.Dockerfiledownstreamadapter/sink/cloudstorage/dml_writers.godownstreamadapter/sink/cloudstorage/sink.godownstreamadapter/sink/cloudstorage/writer.godownstreamadapter/sink/redo/meta.gogo.modlogservice/logpuller/grpc_conn.gopkg/common/event/chunk.gopkg/common/event/mounter.gopkg/config/consistent.gopkg/redo/config.gopkg/redo/config_test.gopkg/redo/reader/file.gopkg/redo/writer/factory/factory.gopkg/redo/writer/file/file.gopkg/redo/writer/file/file_test.gopkg/redo/writer/memory/file_worker.gopkg/security/credential.gopkg/sink/cloudstorage/path.gopkg/sink/codec/canal/canal_json_decoder.gopkg/sink/codec/common/ddl_test.gopkg/sink/codec/open/decoder.gopkg/sink/codec/simple/decoder.gopkg/sink/codec/simple/encoder_test.gopkg/sink/kafka/claimcheck/claim_check.gopkg/sink/mysql/mysql_writer_test.gopkg/tcpserver/tcp_server.gopkg/txnutil/gc/gc_service_test.gopkg/util/external_storage.gopkg/util/external_storage_test.gotests/integration_tests/debezium01/go.modtools/check/go.modtools/workload/go.mod
Signed-off-by: Jianjun Liao <jianjun.liao@outlook.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
go.mod (1)
57-57:⚠️ Potential issue | 🔴 CriticalBlocker: pinned TiDB revision is flagged with critical advisories.
Line 57 still pins
github.com/pingcap/tidb v1.1.0-beta.0.20260205093021-1eb3d0a8c7a9, which OSV flags (GHSA-7fxj-fr3v-r9gj, GHSA-9g6g-xqv5-8g5w / GO-2024-3284). Please move to a patched TiDB revision before merge.#!/bin/bash set -euo pipefail # Verify currently pinned TiDB version in go.mod awk '/github.com\/pingcap\/tidb / {print "go.mod line", NR ":", $0}' go.mod # Query OSV for the pinned version cat >/tmp/osv_tidb_query.json <<'JSON' { "package": {"name": "github.com/pingcap/tidb", "ecosystem": "Go"}, "version": "1.1.0-beta.0.20260205093021-1eb3d0a8c7a9" } JSON curl -sS https://api.osv.dev/v1/query -d `@/tmp/osv_tidb_query.json` | jq '{vulns: [.vulns[]?.id]}'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go.mod` at line 57, The go.mod currently pins the vulnerable TiDB revision "github.com/pingcap/tidb v1.1.0-beta.0.20260205093021-1eb3d0a8c7a9"; update go.mod to reference a patched TiDB release/commit (replace that exact module version string) by choosing a non-vulnerable tag or commit known to include the fixes for GHSA-7fxj-fr3v-r9gj / GHSA-9g6g-xqv5-8g5w (verify via OSV or TiDB release notes), run `go get github.com/pingcap/tidb@<patched-version>` and then `go mod tidy` to update go.sum and vendor as needed, and re-run tests/build to confirm no breakage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@go.mod`:
- Line 57: The go.mod currently pins the vulnerable TiDB revision
"github.com/pingcap/tidb v1.1.0-beta.0.20260205093021-1eb3d0a8c7a9"; update
go.mod to reference a patched TiDB release/commit (replace that exact module
version string) by choosing a non-vulnerable tag or commit known to include the
fixes for GHSA-7fxj-fr3v-r9gj / GHSA-9g6g-xqv5-8g5w (verify via OSV or TiDB
release notes), run `go get github.com/pingcap/tidb@<patched-version>` and then
`go mod tidy` to update go.sum and vendor as needed, and re-run tests/build to
confirm no breakage.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
go.modpkg/common/kv_entry.gopkg/util/external_storage.gopkg/util/external_storage_test.go
ti-chi-bot
bot
added
do-not-merge/needs-triage-completed
and removed
do-not-merge/needs-linked-issue
labels
Author
|
/test check-issue-triage-complete |
|
@Leavrth: The specified target(s) for The following commands are available to trigger optional jobs: Use DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
asddongmen
approved these changes
Feb 27, 2026
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: asddongmen 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 |
[LGTM Timeline notifier]Timeline:
|
Author
|
/check-issue-triage-complete |
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.
What problem does this PR solve?
Issue Number: ref #4244
What is changed and how it works?
update tidb dep to support
startAfteroflistobjectV2of s3Check 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
Improvements
Refactor
Chores
Tests