redo: configuration intialization set correctly when start the redo and reduce the complexity#4510
Merged
ti-chi-bot[bot] merged 22 commits intopingcap:masterfrom Mar 18, 2026
Merged
Conversation
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
release-note
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 focuses on simplifying and improving 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
Activity
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
|
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:
📝 WalkthroughWalkthroughThis PR refactors the redo log writer configuration system by introducing a new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ❌ 5❌ Failed checks (2 warnings, 3 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![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 simplifies the redo sink implementation by refactoring metrics handling, improving error propagation in the constructor, and inlining some helper functions. The changes make the code more straightforward. I've found a minor typo in a field name that should be corrected for better maintainability.
ti-chi-bot
Bot
added
release-note-none
Collaborator
Author
|
/test all |
![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 (2)
pkg/redo/writer/memory/encoding_worker.go (1)
50-51:⚠️ Potential issue | 🔴 CriticalMissing nil check on
rlwill cause panic.If
event.ToRedoLog()returnsnil, the subsequent access torl.Typeon line 51 will panic. According to the AI summary, a nil guard was removed during this refactor. This should be restored to prevent runtime panics and allow proper error propagation.🐛 Proposed fix to restore nil check
func toPolymorphicRedoEvent( event writer.RedoEvent, tableSchemaStore *commonEvent.TableSchemaStore, ) (*polymorphicRedoEvent, error) { rl := event.ToRedoLog() + if rl == nil { + return nil, errors.ErrUnexpected.FastGenByArgs("redo log is nil") + } if rl.Type == commonEvent.RedoLogTypeDDL {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/redo/writer/memory/encoding_worker.go` around lines 50 - 51, The code calls event.ToRedoLog() and immediately accesses rl.Type which will panic if ToRedoLog returns nil; add a nil guard after rl := event.ToRedoLog() (e.g., if rl == nil) and handle it consistently with surrounding logic—either return or propagate an error from the current function or skip processing this event—so you avoid dereferencing rl and preserve proper error propagation for ToRedoLog failures; update any callers or error returns in the enclosing function to match the chosen handling.downstreamadapter/sink/redo/sink.go (1)
86-94:⚠️ Potential issue | 🟠 MajorClose the first writer when the second one fails.
If the DDL writer succeeds and Line 86 fails to create the row writer, this returns immediately and abandons the already-created DDL writer. On the file-backend path,
pkg/redo/writer/file/file.go:222-247showsClose()is where allocator and metric state are released, so repeated init failures can leak resources.♻️ Suggested cleanup on the second init failure path
dmlWriter, err := factory.NewRedoLogWriter(s.ctx, s.cfg, redo.RedoRowLogFileType) if err != nil { + if closeErr := ddlWriter.Close(); closeErr != nil && errors.Cause(closeErr) != context.Canceled { + log.Warn("failed to close ddl writer after row writer init failure", + zap.String("keyspace", s.cfg.ChangeFeedID.Keyspace()), + zap.String("changefeed", s.cfg.ChangeFeedID.Name()), + zap.Error(closeErr)) + } log.Error("redo: failed to create redo log writer", zap.String("keyspace", s.cfg.ChangeFeedID.Keyspace()), zap.String("changefeed", s.cfg.ChangeFeedID.Name()), zap.Duration("duration", time.Since(start)), zap.Error(err)) return nil, err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@downstreamadapter/sink/redo/sink.go` around lines 86 - 94, The second NewRedoLogWriter call (creating dmlWriter with redo.RedoRowLogFileType) can fail and currently returns without cleaning up the previously-created ddl writer; update the error path so that when NewRedoLogWriter for the row writer returns an error you call Close() on the already-created ddl writer (the variable used for the DDL writer) before returning the error, ensuring any allocator/metric state released in Close() is run; reference the existing variables/funcs ddlWriter, dmlWriter, NewRedoLogWriter, Close(), s.ctx and s.cfg when making the change.
🤖 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/sink.go`:
- Line 128: The metric collector is being dereferenced unconditionally (e.g.,
s.mericCollector.observeDDLWrite) which breaks tests that construct a minimal
Sink without a collector; either guard each invocation with a nil-check (if
s.mericCollector != nil { s.mericCollector.observeDDLWrite(...) }) or initialize
s.mericCollector to a no-op implementation in the Sink constructor/zero-value so
observe* methods are safe to call; update all places where s.mericCollector is
used (observeDDLWrite and other observe* calls in sendMessages/related methods)
to use one of these two approaches to restore nil-safety.
In `@pkg/redo/writer/memory/encoding_worker.go`:
- Around line 157-162: The ctx.Done() select branch in the encoding worker (the
select that sends redoLogEvent to e.outputCh and reads from e.closed) returns
ctx.Err() directly; change it to wrap the context error consistently using
errors.Trace(context.Cause(ctx)) (matching the other usage), i.e., replace the
direct return of ctx.Err() in the ctx.Done() case with a wrapped error via
errors.Trace(context.Cause(ctx)) so stack traces are preserved for the code
paths around e.closed, e.outputCh and redoLogEvent.
---
Outside diff comments:
In `@downstreamadapter/sink/redo/sink.go`:
- Around line 86-94: The second NewRedoLogWriter call (creating dmlWriter with
redo.RedoRowLogFileType) can fail and currently returns without cleaning up the
previously-created ddl writer; update the error path so that when
NewRedoLogWriter for the row writer returns an error you call Close() on the
already-created ddl writer (the variable used for the DDL writer) before
returning the error, ensuring any allocator/metric state released in Close() is
run; reference the existing variables/funcs ddlWriter, dmlWriter,
NewRedoLogWriter, Close(), s.ctx and s.cfg when making the change.
In `@pkg/redo/writer/memory/encoding_worker.go`:
- Around line 50-51: The code calls event.ToRedoLog() and immediately accesses
rl.Type which will panic if ToRedoLog returns nil; add a nil guard after rl :=
event.ToRedoLog() (e.g., if rl == nil) and handle it consistently with
surrounding logic—either return or propagate an error from the current function
or skip processing this event—so you avoid dereferencing rl and preserve proper
error propagation for ToRedoLog failures; update any callers or error returns in
the enclosing function to match the chosen handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d7b9a901-138a-4c10-a3cb-28239876271a
📒 Files selected for processing (8)
downstreamadapter/dispatchermanager/dispatcher_manager_redo.godownstreamadapter/sink/redo/metrics.godownstreamadapter/sink/redo/metrics_collector.godownstreamadapter/sink/redo/sink.godownstreamadapter/sink/redo/sink_test.gopkg/redo/writer/factory/factory.gopkg/redo/writer/memory/encoding_worker.gopkg/redo/writer/memory/mem_log_writer.go
💤 Files with no reviewable changes (1)
- downstreamadapter/sink/redo/metrics.go
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
downstreamadapter/sink/redo/sink.go (3)
86-94:⚠️ Potential issue | 🟠 MajorClose the already-created DDL writer on DML writer init failure.
If Line 86 fails,
ddlWriterfrom Line 77 stays open. That leaves a leaked writer in the partial-construction path.♻️ Suggested fix
dmlWriter, err := factory.NewRedoLogWriter(s.ctx, s.cfg, redo.RedoRowLogFileType) if err != nil { + if closeErr := ddlWriter.Close(); closeErr != nil && errors.Cause(closeErr) != context.Canceled { + log.Error("redo sink fails to close ddl writer", + zap.String("keyspace", s.cfg.ChangeFeedID.Keyspace()), + zap.String("changefeed", s.cfg.ChangeFeedID.Name()), + zap.Error(closeErr)) + } log.Error("redo: failed to create redo log writer", zap.String("keyspace", s.cfg.ChangeFeedID.Keyspace()), zap.String("changefeed", s.cfg.ChangeFeedID.Name()), zap.Duration("duration", time.Since(start)), zap.Error(err)) return nil, err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@downstreamadapter/sink/redo/sink.go` around lines 86 - 94, When NewRedoLogWriter for the DML writer (dmlWriter) fails, you must close the previously created ddlWriter to avoid leaking the open writer; after the err check in the dmlWriter init path, if ddlWriter != nil call its Close (or Close(ctx) if that API exists) and handle/log any close error before returning the original err. Update the block around factory.NewRedoLogWriter(s.ctx, s.cfg, redo.RedoRowLogFileType) to ensure ddlWriter is closed on failure.
16-34:⚠️ Potential issue | 🟡 MinorResolve import formatting before merge (CI currently red).
Pipeline reports
gci import formatting wrote changes; make fmt failed. Please run formatter and commit the result.As per coding guidelines, "
**/*.go: Usegofmtto keep Go code clean; runmake fmtbefore pushing to format code withgci,gofumports, andshfmt, plus log-style checks".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@downstreamadapter/sink/redo/sink.go` around lines 16 - 34, The import block in downstreamadapter/sink/redo/sink.go is not formatted per project CI (gci/gofumports); run the repository formatter and commit the changes: execute make fmt (which runs gci, gofumports, shfmt and related checks) to reorder and format imports and apply gofmt to the file (affecting the import block around the import list that includes symbols like helper, commonEvent, redo, writer, factory, chann, atomic); after formatting, verify no diffs remain and push the formatted file.
77-84:⚠️ Potential issue | 🟠 MajorWrap writer/factory errors at source instead of returning raw errors.
Lines 84, 93, 126, and 213 return raw
err. These call sites should attach stack trace once, then let upstream propagate.🧩 Suggested fix
- return nil, err + return nil, errors.Trace(err) @@ - return nil, err + return nil, errors.Trace(err) @@ - return err + return errors.Trace(err) @@ - return err + return errors.Trace(err)As per coding guidelines, "When an error comes from a third-party or library call in Go, wrap it immediately with
errors.Trace(err)orerrors.WrapError(...)to attach a stack trace; upstream callers should propagate wrapped errors without wrapping again".Also applies to: 86-93, 123-127, 211-214
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@downstreamadapter/sink/redo/sink.go` around lines 77 - 84, The factory.NewRedoLogWriter calls (e.g., the call that assigns ddlWriter via factory.NewRedoLogWriter(s.ctx, s.cfg, redo.RedoDDLLogFileType)) and the other sites that currently do `return nil, err` should wrap the incoming error with the project’s trace wrapper before returning so a stack trace is attached once; replace raw returns with the traced error (e.g., use errors.Trace(err) or errors.WrapError(...) according to the repo helper) at each call site (the NewRedoLogWriter returns for DDL/DML and the other places noted in the comment) and leave upstream callers to propagate the wrapped error.
♻️ Duplicate comments (1)
downstreamadapter/sink/redo/sink.go (1)
97-97:⚠️ Potential issue | 🔴 CriticalFix
metricCollectortypo to restore build and metrics path.Line 97 assigns
s.mericCollector, but the struct field ismetricCollector(Line 50). This breaks compilation and all metric observations/close calls in Lines 128, 186, and 215.🔧 Minimal fix
- s.mericCollector = newMetricCollector(changefeedID) + s.metricCollector = newMetricCollector(changefeedID) @@ - s.mericCollector.observeDDLWrite(time.Since(start)) + s.metricCollector.observeDDLWrite(time.Since(start)) @@ - s.mericCollector.close() + s.metricCollector.close() @@ - s.mericCollector.observeRowWrite(len(events), time.Since(start)) + s.metricCollector.observeRowWrite(len(events), time.Since(start))Also applies to: 128-128, 186-186, 215-215
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@downstreamadapter/sink/redo/sink.go` at line 97, There is a typo: the struct field is named metricCollector but the code assigns s.mericCollector; change the assignment to use s.metricCollector = newMetricCollector(changefeedID) and update all other references (s.mericCollector) to s.metricCollector (including the metric observation/close sites that call the collector), ensuring you reference the metricCollector field and the newMetricCollector factory consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@downstreamadapter/sink/redo/sink.go`:
- Around line 86-94: When NewRedoLogWriter for the DML writer (dmlWriter) fails,
you must close the previously created ddlWriter to avoid leaking the open
writer; after the err check in the dmlWriter init path, if ddlWriter != nil call
its Close (or Close(ctx) if that API exists) and handle/log any close error
before returning the original err. Update the block around
factory.NewRedoLogWriter(s.ctx, s.cfg, redo.RedoRowLogFileType) to ensure
ddlWriter is closed on failure.
- Around line 16-34: The import block in downstreamadapter/sink/redo/sink.go is
not formatted per project CI (gci/gofumports); run the repository formatter and
commit the changes: execute make fmt (which runs gci, gofumports, shfmt and
related checks) to reorder and format imports and apply gofmt to the file
(affecting the import block around the import list that includes symbols like
helper, commonEvent, redo, writer, factory, chann, atomic); after formatting,
verify no diffs remain and push the formatted file.
- Around line 77-84: The factory.NewRedoLogWriter calls (e.g., the call that
assigns ddlWriter via factory.NewRedoLogWriter(s.ctx, s.cfg,
redo.RedoDDLLogFileType)) and the other sites that currently do `return nil,
err` should wrap the incoming error with the project’s trace wrapper before
returning so a stack trace is attached once; replace raw returns with the traced
error (e.g., use errors.Trace(err) or errors.WrapError(...) according to the
repo helper) at each call site (the NewRedoLogWriter returns for DDL/DML and the
other places noted in the comment) and leave upstream callers to propagate the
wrapped error.
---
Duplicate comments:
In `@downstreamadapter/sink/redo/sink.go`:
- Line 97: There is a typo: the struct field is named metricCollector but the
code assigns s.mericCollector; change the assignment to use s.metricCollector =
newMetricCollector(changefeedID) and update all other references
(s.mericCollector) to s.metricCollector (including the metric observation/close
sites that call the collector), ensuring you reference the metricCollector field
and the newMetricCollector factory consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 01d05201-a492-410c-9c89-c204d586de9f
📒 Files selected for processing (1)
downstreamadapter/sink/redo/sink.go
wk989898
approved these changes
Mar 17, 2026
ti-chi-bot
Bot
added
needs-1-more-lgtm
ti-chi-bot
Bot
added
size/XXL
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
ti-chi-bot
Bot
removed
the
do-not-merge/work-in-progress
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
downstreamadapter/sink/redo/sink.go (1)
118-132:⚠️ Potential issue | 🟡 MinorMissing nil-check for metricCollector at line 127.
Line 127 calls
s.metricCollector.observeDDLWrite()unconditionally, while lines 224-226 guardobserveRowWritewith a nil-check. For consistency and to support test scenarios that construct minimalSinkinstances, add a nil-check here as well.🛡️ Proposed fix for consistency
s.isNormal.Store(false) return err } - s.metricCollector.observeDDLWrite(time.Since(start)) + if s.metricCollector != nil { + s.metricCollector.observeDDLWrite(time.Since(start)) + } log.Info("redo sink send DDL event", zap.Any("startTs", event.GetStartTs()), zap.Any("commitTs", event.GetCommitTs()),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@downstreamadapter/sink/redo/sink.go` around lines 118 - 132, The WriteBlockEvent method in Sink calls s.metricCollector.observeDDLWrite(...) without checking for nil; update Sink.WriteBlockEvent to guard the metric call the same way as observeRowWrite does elsewhere: after successful ddlWriter.WriteEvents, only call s.metricCollector.observeDDLWrite(time.Since(start)) if s.metricCollector != nil (and keep s.isNormal.Store(false) behavior and logging unchanged), using the existing s.ctx, s.ddlWriter.WriteEvents and e variables to locate the spot.
🧹 Nitpick comments (1)
pkg/redo/writer/config.go (1)
93-112: Logic is correct but could benefit from a clarifying comment.The branching logic handles:
- Non-external storage (e.g., blackhole after FixLocalScheme) → returns
uri.Pathfile://scheme (including normalizedlocal/nfs) → returnsuri.Path- External storage with file backend (e.g., S3 + local staging) → returns computed path under DataDir
- External storage without file backend → returns empty string
The overlap between cases 1 and 2 after
FixLocalSchemenormalization means case 1 effectively only handles blackhole storage. Consider adding a brief comment explaining the post-normalization flow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/redo/writer/config.go` around lines 93 - 112, Add a brief clarifying comment at the top of newWriterDir describing the post-FixLocalScheme branching: explain that UseExternalStorage() false now only occurs for "blackhole" (so returns cfg.uri.Path), uri.Scheme == "file" (including normalized "local"/"nfs") also returns cfg.uri.Path, external storage with cfg.useFileBackend uses the computed path under config.GetGlobalServerConfig().DataDir/config.DefaultRedoDir/changefeedID.Keyspace()/changefeedID.Name(), and external storage without a file backend returns an empty string; reference the symbols newWriterDir, Config, UseExternalStorage, cfg.uri.Path, cfg.uri.Scheme, useFileBackend, config.GetGlobalServerConfig, DataDir, DefaultRedoDir, and changefeedID.Keyspace()/Name() in the comment.
🤖 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_test.go`:
- Around line 28-29: The import block in meta_test.go is mis-ordered (writertest
and misc imports) causing gci failure; reorder the imports into the canonical
groups and run the formatter: update the import section so standard library,
external modules, and internal packages are in the correct order (ensure symbols
like writertest and misc remain imported) and then run `make fmt` to apply gci
formatting and fix the pipeline error.
In `@pkg/redo/writer/file/file_test.go`:
- Around line 233-234: The test is building expected object names from global
state (config.GetGlobalServerConfig().AdvertiseAddr) which is racy; instead
derive the expected filename from the writer.Config used in the test fixture
(the instance passed to NewWriter or WriteFile call) and use that to construct
the fmt.Sprintf pattern passed to mockStorage.EXPECT().WriteFile; replace any
global-config references in the three expectations (the one at lines ~233,
~277-280, ~338-341) to use writer.Config.AdvertiseAddr (or the specific
capture-id helper on the writer.Config) so the test asserts against the actual
writer.Config instance, and switch any plain t.Fatal/asserts to testify/require
calls for deterministic failures.
In `@pkg/redo/writer/testutil/config.go`:
- Line 7: Update the truncated license header URL from "LICENSE-20" to the
correct "LICENSE-2.0" in the file header comment; locate the top-of-file license
block (the package header comment in config.go) and replace the malformed URL so
the Apache license link reads "http://www.apache.org/licenses/LICENSE-2.0".
---
Outside diff comments:
In `@downstreamadapter/sink/redo/sink.go`:
- Around line 118-132: The WriteBlockEvent method in Sink calls
s.metricCollector.observeDDLWrite(...) without checking for nil; update
Sink.WriteBlockEvent to guard the metric call the same way as observeRowWrite
does elsewhere: after successful ddlWriter.WriteEvents, only call
s.metricCollector.observeDDLWrite(time.Since(start)) if s.metricCollector != nil
(and keep s.isNormal.Store(false) behavior and logging unchanged), using the
existing s.ctx, s.ddlWriter.WriteEvents and e variables to locate the spot.
---
Nitpick comments:
In `@pkg/redo/writer/config.go`:
- Around line 93-112: Add a brief clarifying comment at the top of newWriterDir
describing the post-FixLocalScheme branching: explain that UseExternalStorage()
false now only occurs for "blackhole" (so returns cfg.uri.Path), uri.Scheme ==
"file" (including normalized "local"/"nfs") also returns cfg.uri.Path, external
storage with cfg.useFileBackend uses the computed path under
config.GetGlobalServerConfig().DataDir/config.DefaultRedoDir/changefeedID.Keyspace()/changefeedID.Name(),
and external storage without a file backend returns an empty string; reference
the symbols newWriterDir, Config, UseExternalStorage, cfg.uri.Path,
cfg.uri.Scheme, useFileBackend, config.GetGlobalServerConfig, DataDir,
DefaultRedoDir, and changefeedID.Keyspace()/Name() in the comment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5ecc832b-39d2-49c0-b18e-61d9bf068d20
📒 Files selected for processing (22)
downstreamadapter/dispatchermanager/dispatcher_manager_redo.godownstreamadapter/sink/redo/meta_test.godownstreamadapter/sink/redo/sink.godownstreamadapter/sink/redo/sink_test.gopkg/config/consistent.gopkg/redo/reader/file.gopkg/redo/reader/reader_test.gopkg/redo/writer/config.gopkg/redo/writer/factory/factory.gopkg/redo/writer/file/file.gopkg/redo/writer/file/file_log_writer.gopkg/redo/writer/file/file_log_writer_test.gopkg/redo/writer/file/file_test.gopkg/redo/writer/file/test_helper_test.gopkg/redo/writer/memory/encoding_worker.gopkg/redo/writer/memory/encoding_worker_test.gopkg/redo/writer/memory/file_worker.gopkg/redo/writer/memory/mem_log_writer.gopkg/redo/writer/memory/mem_log_writer_test.gopkg/redo/writer/testutil/config.gopkg/redo/writer/writer.gopkg/redo/writer/writer_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/redo/writer/memory/mem_log_writer.go
- pkg/redo/writer/factory/factory.go
- pkg/redo/writer/memory/encoding_worker_test.go
- pkg/redo/writer/memory/mem_log_writer_test.go
- pkg/redo/writer/file/file_log_writer_test.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/sink_test.go`:
- Around line 213-216: The test constructs a config with an unsupported URI
("blackhole-invalid://") which now fails inside New via writer.NewConfig
validation; update the test (TestRedoSinkError) to either use a valid sink URI
(e.g., "blackhole://" or another supported scheme returned by
newTestConsistentConfig) or assert that New(...) returns an error (replace
require.NoError with require.Error and adjust subsequent expectations),
referencing newTestConsistentConfig and New (which calls writer.NewConfig) to
locate the fix.
In `@downstreamadapter/sink/redo/sink.go`:
- Around line 64-67: The call to writer.NewConfig(changefeedID, cfg) returns a
raw library error; wrap it immediately (e.g., return nil, errors.Trace(err) or
return nil, errors.WrapError(err, "creating writer config")) so the stack
context is preserved; apply the same change to the other library-call returns
flagged in this file (the other raw returns) so every third-party/library error
at the call site uses errors.Trace or errors.WrapError instead of returning err
directly.
- Around line 85-93: When NewRedoLogWriter for DML (dmlWriter) fails, the
previously-created ddlWriter must be closed to avoid leaking resources; update
the error path after calling factory.NewRedoLogWriter(ctx, config,
redo.RedoRowLogFileType) so that if err != nil you first close ddlWriter (call
its Close/Close(ctx) method consistent with its type) and optionally log any
close error, then return the original creation error; reference ddlWriter,
dmlWriter and factory.NewRedoLogWriter in your change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4911f6ce-2ee3-424b-b7a0-dab75e5a13bd
📒 Files selected for processing (9)
downstreamadapter/sink/redo/meta_test.godownstreamadapter/sink/redo/sink.godownstreamadapter/sink/redo/sink_test.gopkg/redo/reader/reader_test.gopkg/redo/testutil/config.gopkg/redo/writer/file/test_helper_test.gopkg/redo/writer/memory/encoding_worker_test.gopkg/redo/writer/memory/mem_log_writer_test.gopkg/redo/writer/writer_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/redo/writer/memory/encoding_worker_test.go
- pkg/redo/writer/file/test_helper_test.go
Collaborator
Author
|
/test all |
Collaborator
Author
|
/test all |
3AceShowHand
changed the title
flowbehappy
approved these changes
Mar 18, 2026
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: asddongmen, flowbehappy, wk989898 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 |
This was referenced Mar 19, 2026
This was referenced Mar 31, 2026
This was referenced Apr 14, 2026
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
4 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: close #4512
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
Release Notes
New Features
Bug Fixes
Refactor