Repository navigation
localbackend: add write timeout handling for object store region jobs - #68148
ti-chi-bot[bot] merged 7 commits into
Conversation
ti-chi-bot
Bot
added
the
release-note
|
@D3Hunter I've received your pull request and will start the review. I'll conduct a thorough review covering code quality, potential issues, and implementation details. ⏳ This process typically takes 10-30 minutes depending on the complexity of the changes. ℹ️ Learn more details on Pantheon AI. |
ti-chi-bot
Bot
added
the
size/M
|
Hi @D3Hunter. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. 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. |
|
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:
📝 WalkthroughWalkthroughAdds a per-write timeout wrapper ChangesWrite timeout refactor and test updates
Sequence Diagram(s)sequenceDiagram
participant Worker as Worker.runJob
participant Wrapper as writeWithTimeout
participant Limiter as writeLimiter
participant TiKV as TiKV / ingest client
Worker->>Wrapper: writeWithTimeout(ctx, job)
Wrapper->>Limiter: WaitN(ctx, storeID, size)
Limiter-->>Wrapper: permit/deny
alt failpoint injects short timeout
Wrapper->>TiKV: w.writeFn using wctx (timed)
else default
Wrapper->>TiKV: w.writeFn using ctx
end
TiKV-->>Wrapper: success or deadline exceeded (with cause)
Wrapper->>Worker: return result (or translated retryable timeout)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
D3Hunter
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/lightning/backend/local/job_worker_test.go (1)
309-326: ⚡ Quick winAdd one more timeout case for the write stream itself.
This subtest only exercises timeout during
WriteClientcreation. Since the wrapper is meant to cover the whole TiKV write path, please add a case whereWriteClientsucceeds andWriteorRecvtimes out as well.🔧 Suggested test shape
+ t.Run("timeout while writing or receiving should be handled by runJob wrapper", func(t *testing.T) { + // Arrange a mock write client that succeeds on creation, + // then blocks or returns context.DeadlineExceeded from Write/Recv. + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/lightning/backend/local/job_worker_test.go` around lines 309 - 326, Add a sibling subtest that simulates a timeout on the write stream (not just WriteClient creation): in addition to the existing test using mockIngestCli.EXPECT().WriteClient(...).Return(nil, context.DeadlineExceeded), add a test where mockIngestCli.EXPECT().WriteClient(...).Return(mockWriteClient, nil) and configure mockWriteClient.EXPECT().Write(...) or mockWriteClient.EXPECT().Recv(...).Return(context.DeadlineExceeded) (or both) to simulate the stream timing out; call cloudW.runJob(ctx, job) with a context created via context.WithTimeoutCause(..., common.ErrWriteTooSlow) and assert the same outcomes as the current test (require.NoError on runJob, job.stage == needRescan, job.lastRetryableErr is common.ErrWriteTooSlow, and ctrl.Satisfied()).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/lightning/backend/local/job_worker_test.go`:
- Around line 309-326: Add a sibling subtest that simulates a timeout on the
write stream (not just WriteClient creation): in addition to the existing test
using mockIngestCli.EXPECT().WriteClient(...).Return(nil,
context.DeadlineExceeded), add a test where
mockIngestCli.EXPECT().WriteClient(...).Return(mockWriteClient, nil) and
configure mockWriteClient.EXPECT().Write(...) or
mockWriteClient.EXPECT().Recv(...).Return(context.DeadlineExceeded) (or both) to
simulate the stream timing out; call cloudW.runJob(ctx, job) with a context
created via context.WithTimeoutCause(..., common.ErrWriteTooSlow) and assert the
same outcomes as the current test (require.NoError on runJob, job.stage ==
needRescan, job.lastRetryableErr is common.ErrWriteTooSlow, and
ctrl.Satisfied()).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: dec71dd6-294d-402d-aecd-636198057c17
📒 Files selected for processing (3)
pkg/lightning/backend/local/job_worker.gopkg/lightning/backend/local/job_worker_test.gopkg/lightning/backend/local/region_job.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #68148 +/- ##
================================================
- Coverage 77.7620% 77.5461% -0.2160%
================================================
Files 1990 1984 -6
Lines 551774 558840 +7066
================================================
+ Hits 429071 433359 +4288
- Misses 121783 124557 +2774
- Partials 920 924 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
🔍 Starting code review for this PR... |
ingress-bot
left a comment
ingress-bot
left a comment
There was a problem hiding this comment.
This review was generated by AI and should be verified by a human reviewer.
Manual follow-up is recommended before merge.
Summary
- Total findings: 3
- Inline comments: 3
- Summary-only findings (no inline anchor): 0
Findings (highest risk first)
🟡 [Minor] (3)
doWritecomment still points to a removed timeout defer location (pkg/lightning/backend/local/job_worker.go:270, pkg/lightning/backend/local/region_job.go:501)- Write-timeout regression signal became nondeterministic for the shortWaitNTimeout path (pkg/lightning/backend/local/job_worker.go:283, pkg/lightning/backend/local/region_job.go:461, lightning/tests/lightning_write_timeout/run.sh:48)
doWritetimeout comment now describes removed control flow (pkg/lightning/backend/local/region_job.go:501, pkg/lightning/backend/local/job_worker.go:270)
|
/retest |
|
@D3Hunter: PRs from untrusted users cannot be marked as trusted with 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. |
|
/retest |
|
@D3Hunter: PRs from untrusted users cannot be marked as trusted with 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. |
D3Hunter
changed the title
|
/retest |
|
@D3Hunter: PRs from untrusted users cannot be marked as trusted with 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. |
ti-chi-bot
Bot
added
size/L
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/lightning/backend/local/region_job.go (1)
469-484: 💤 Low valueStale comment at lines 469–471 references a removed failpoint context.
The two-line comment "original ctx would be used when failpoint is not enabled / that new context would be used when failpoint is enabled" no longer matches this function. The
shortWaitNTimeoutfailpoint, which previously injected a separate write context here, has been moved toregionJobBaseWorker.writeWithTimeoutinjob_worker.go.doWritenow only sees a singlectx, so the contrast described in the comment is misleading for future readers tracing the timeout-injection path.Consider dropping or simplifying that pair of lines, e.g.:
📝 Proposed comment cleanup
- // original ctx would be used when failpoint is not enabled - // that new context would be used when failpoint is enabled err := writeLimiter.WaitN(ctx, allPeers[i].StoreId, int(size)) if err != nil { // We expect to encounter two types of errors here: // 1. context.DeadlineExceeded — occurs when the calculated delay is // less than the remaining time in the context, but the context // expires while sleeping. // 2. "rate: Wait(n=%d) would exceed context deadline" — a fast-fail // path triggered when the delay already exceeds the remaining // time for context before sleeping. // - // "rate: Wait" error is already treated as retryable, - // writeWithTimeout will handle the context.DeadlineExceeded. + // The "rate: Wait" error is already treated as retryable; + // context.DeadlineExceeded is translated to common.ErrWriteTooSlow + // by regionJobBaseWorker.writeWithTimeout in runJob. return errors.Trace(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 `@pkg/lightning/backend/local/region_job.go` around lines 469 - 484, The two-line comment in doWrite that claims "original ctx would be used when failpoint is not enabled / that new context would be used when failpoint is enabled" is stale because the shortWaitNTimeout failpoint was moved into regionJobBaseWorker.writeWithTimeout (job_worker.go) and doWrite now only receives a single ctx; remove or replace those two lines with a concise note that timeout injection is handled in writeWithTimeout (or simply delete the misleading lines) so readers are not misled about multiple contexts in doWrite.pkg/lightning/backend/local/job_worker.go (1)
290-295: ⚡ Quick winUse
goerrors.Is(err, context.DeadlineExceeded)instead oferrors.Cause(err) ==for consistency and robustness.Line 290 uses
errors.Cause(err) == context.DeadlineExceeded, which only unwraps errors via the pingcap/errorsCause()interface. This misses errors wrapped with Go's standardfmt.Errorf("...: %w", ...)pattern (common in gRPC), allowing deadline timeouts to leak through uncaught. The codebase already usesgoerrors.Is()for context sentinel checks elsewhere (e.g.,log.go,retry.go,byte_reader.go), and line 291 in this same block already usesgoerrors.Is()for a similar cause check.Proposed change
- if errors.Cause(err) == context.DeadlineExceeded { + if goerrors.Is(err, context.DeadlineExceeded) { if cause := context.Cause(ctx); goerrors.Is(cause, common.ErrWriteTooSlow) { tidblogutil.Logger(ctx).Info("Experiencing a wait timeout while writing to TiKV") err = errors.Trace(cause) } }🤖 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/lightning/backend/local/job_worker.go` around lines 290 - 295, Replace the equality check using errors.Cause(err) with the Go errors package helper so deadline errors wrapped with %w are recognized: in the block that currently tests errors.Cause(err) == context.DeadlineExceeded (the one that also inspects context.Cause(ctx) and goerrors.Is(cause, common.ErrWriteTooSlow) and logs via tidblogutil.Logger), change the condition to use goerrors.Is(err, context.DeadlineExceeded) so wrapped deadline errors are correctly detected and the existing handling (logging and err = errors.Trace(cause)) remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/lightning/backend/local/job_worker.go`:
- Around line 290-295: Replace the equality check using errors.Cause(err) with
the Go errors package helper so deadline errors wrapped with %w are recognized:
in the block that currently tests errors.Cause(err) == context.DeadlineExceeded
(the one that also inspects context.Cause(ctx) and goerrors.Is(cause,
common.ErrWriteTooSlow) and logs via tidblogutil.Logger), change the condition
to use goerrors.Is(err, context.DeadlineExceeded) so wrapped deadline errors are
correctly detected and the existing handling (logging and err =
errors.Trace(cause)) remains unchanged.
In `@pkg/lightning/backend/local/region_job.go`:
- Around line 469-484: The two-line comment in doWrite that claims "original ctx
would be used when failpoint is not enabled / that new context would be used
when failpoint is enabled" is stale because the shortWaitNTimeout failpoint was
moved into regionJobBaseWorker.writeWithTimeout (job_worker.go) and doWrite now
only receives a single ctx; remove or replace those two lines with a concise
note that timeout injection is handled in writeWithTimeout (or simply delete the
misleading lines) so readers are not misled about multiple contexts in doWrite.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a5a48b9d-3cef-49f2-85a3-3b963669c0fb
📒 Files selected for processing (3)
lightning/tests/lightning_write_timeout/run.shpkg/lightning/backend/local/job_worker.gopkg/lightning/backend/local/region_job.go
| failpoint.Inject("shortWaitNTimeout", func(val failpoint.Value) { | ||
| ms := val.(int) | ||
| innerTimeout := time.Duration(ms) * time.Millisecond | ||
| tidblogutil.Logger(ctx).Info("Injecting a timeout to write context.") |
There was a problem hiding this comment.
| tidblogutil.Logger(ctx).Info("Injecting a timeout to write context.") | |
| tidblogutil.Logger(ctx).Info("injecting a timeout to write context.") |
| } | ||
| if errors.Cause(err) == context.DeadlineExceeded { | ||
| if cause := context.Cause(ctx); goerrors.Is(cause, common.ErrWriteTooSlow) { | ||
| tidblogutil.Logger(ctx).Info("Experiencing a wait timeout while writing to TiKV") |
There was a problem hiding this comment.
| tidblogutil.Logger(ctx).Info("Experiencing a wait timeout while writing to TiKV") | |
| tidblogutil.Logger(ctx).Info("experiencing a wait timeout while writing to TiKV") |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/lightning/backend/local/region_job.go (1)
473-475: 💤 Low valueStale comment: there is no separate "new context" anymore.
The comment on lines 474-475 still describes the prior design where the failpoint produced a separate timeout context for
WaitN. In the current implementation the failpoint only doestime.Sleep, andwriteLimiter.WaitNalways uses the samectxregardless of whether the failpoint is enabled. Recommend removing or rewording so the intent (sleep long enough for the wrapper'sctxto expire and surfaceDeadlineExceeded) is what's documented.♻️ Suggested rewording
- for i := range clients { - // original ctx would be used when failpoint is not enabled - // that new context would be used when failpoint is enabled - err := writeLimiter.WaitN(ctx, allPeers[i].StoreId, int(size)) + for i := range clients { + // When shortWaitNTimeout is enabled, the sleep above is long enough + // for ctx (already wrapped with a timeout in writeWithTimeout) to + // expire here, surfacing DeadlineExceeded from WaitN. + err := writeLimiter.WaitN(ctx, allPeers[i].StoreId, int(size))As per coding guidelines, "Comments SHOULD explain non-obvious intent ... and SHOULD NOT restate what the code already makes clear."
🤖 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/lightning/backend/local/region_job.go` around lines 473 - 475, The existing comment near the loop over clients describing a "new context" is stale—update the comment around the loop that references failpoint and writeLimiter.WaitN to remove the mention of a separate context and instead state the actual intent: the failpoint only does time.Sleep so it delays long enough for the wrapper's ctx to expire and surface a context.DeadlineExceeded from writeLimiter.WaitN; briefly document that behavior (or remove the comment entirely) and reference the failpoint, the loop that iterates over clients, and writeLimiter.WaitN so readers can find the related code.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/lightning/backend/local/job_worker.go`:
- Around line 289-290: Replace the direct comparison errors.Cause(err) ==
context.DeadlineExceeded with a call to goerrors.Is to consistently detect
deadline errors even when wrapped; specifically, in job_worker.go where the code
checks errors.Cause(err) == context.DeadlineExceeded (near the block that then
inspects context.Cause via goerrors.Is), change that condition to
goerrors.Is(err, context.DeadlineExceeded) so the deadline check uses the same
robust unwrapping semantics as the subsequent goerrors.Is usage.
---
Nitpick comments:
In `@pkg/lightning/backend/local/region_job.go`:
- Around line 473-475: The existing comment near the loop over clients
describing a "new context" is stale—update the comment around the loop that
references failpoint and writeLimiter.WaitN to remove the mention of a separate
context and instead state the actual intent: the failpoint only does time.Sleep
so it delays long enough for the wrapper's ctx to expire and surface a
context.DeadlineExceeded from writeLimiter.WaitN; briefly document that behavior
(or remove the comment entirely) and reference the failpoint, the loop that
iterates over clients, and writeLimiter.WaitN so readers can find the related
code.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fc74ca53-2705-4171-ba10-66991b2dc871
📒 Files selected for processing (3)
lightning/tests/lightning_write_timeout/run.shpkg/lightning/backend/local/job_worker.gopkg/lightning/backend/local/region_job.go
| if errors.Cause(err) == context.DeadlineExceeded { | ||
| if cause := context.Cause(ctx); goerrors.Is(cause, common.ErrWriteTooSlow) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect how writeFn implementations propagate context.DeadlineExceeded.
rg -nP -C3 '\bfunc\s+\([^)]*\)\s+(doWrite|write)\s*\(' pkg/lightning/backend/local
rg -nP -C2 'DeadlineExceeded|status\.FromContextError|context\.Cause' pkg/lightning/backend/local pkg/ingestorRepository: pingcap/tidb
Length of output: 3167
🏁 Script executed:
# First, let's examine the doWrite function implementation
sed -n '331,450p' pkg/lightning/backend/local/region_job.go | head -100Repository: pingcap/tidb
Length of output: 3077
🏁 Script executed:
# Examine the write function implementation
sed -n '355,420p' pkg/lightning/backend/local/job_worker.goRepository: pingcap/tidb
Length of output: 1741
🏁 Script executed:
# Check how errors are imported and used in these files
head -50 pkg/lightning/backend/local/job_worker.go | grep -E 'import|errors|goerrors'Repository: pingcap/tidb
Length of output: 164
🏁 Script executed:
# Look for gRPC status error handling in the codebase
rg -n 'status\.FromContextError|status\.FromError' pkg/lightning/backend/local pkg/ingestorRepository: pingcap/tidb
Length of output: 141
🏁 Script executed:
# Get full context around job_worker.go:289-290
sed -n '270,310p' pkg/lightning/backend/local/job_worker.goRepository: pingcap/tidb
Length of output: 1425
🏁 Script executed:
# Check if errors are wrapped with %w anywhere in writeFn implementations
rg '%w' pkg/lightning/backend/local/job_worker.go pkg/lightning/backend/local/region_job.goRepository: pingcap/tidb
Length of output: 38
🏁 Script executed:
# Look for where gRPC stream errors might be handled before reaching line 289
sed -n '270,350p' pkg/lightning/backend/local/job_worker.go | grep -A30 'func (w \*localRegionJobWorker)'Repository: pingcap/tidb
Length of output: 38
🏁 Script executed:
# Check how writeWithTimeout works and what writeFn is
sed -n '250,290p' pkg/lightning/backend/local/job_worker.goRepository: pingcap/tidb
Length of output: 1392
🏁 Script executed:
# Check if pingcap/errors types implement Unwrap() for Is() compatibility
rg -n 'func.*Unwrap|func.*Cause' pkg/vendor/github.com/pingcap/errors 2>/dev/null || \
rg -n 'pingcap/errors' go.modRepository: pingcap/tidb
Length of output: 125
🏁 Script executed:
# Look for any %w wrapping in region_job.go doWrite implementation
sed -n '331,550p' pkg/lightning/backend/local/region_job.go | grep -n '%w\|Wrap'Repository: pingcap/tidb
Length of output: 38
🏁 Script executed:
# Check what Annotate and Trace functions do
rg -A5 'func.*Annotate|func.*Trace' pkg/lightning/backend/local/job_worker.go | head -20Repository: pingcap/tidb
Length of output: 38
🏁 Script executed:
# Look at the complete doWrite to see all error returns
sed -n '331,600p' pkg/lightning/backend/local/region_job.go | grep -n 'return.*error\|errors\.\|err :='Repository: pingcap/tidb
Length of output: 1077
🏁 Script executed:
# Check if there are any gRPC status error wrappings in doWrite
sed -n '331,600p' pkg/lightning/backend/local/region_job.go | grep -n 'status\|Status'Repository: pingcap/tidb
Length of output: 38
🏁 Script executed:
# Look at the test to understand expected behavior
sed -n '310,330p' pkg/lightning/backend/local/job_worker_test.goRepository: pingcap/tidb
Length of output: 945
🏁 Script executed:
# Check if pingcap/errors is implemented with Unwrap support by looking at actual usage patterns in tests
rg -B5 -A5 'goerrors\.Is.*DeadlineExceeded' pkg/lightning/backend/local/Repository: pingcap/tidb
Length of output: 38
🏁 Script executed:
# Let's check the actual implementation path for how errors flow through WriteClient
rg -n 'WriteClient' pkg/lightning/backend/local/job_worker.go | head -5Repository: pingcap/tidb
Length of output: 131
🏁 Script executed:
# Look at ingestCli to see what WriteClient returns
find pkg -name '*ingest*' -type f | head -10Repository: pingcap/tidb
Length of output: 173
🏁 Script executed:
# Check how errors.Trace wraps errors - does it implement Unwrap?
rg -B3 -A10 'type.*Error|func.*Unwrap' pkg/vendor/github.com/pingcap/errors/errors.go 2>/dev/null || \
python3 << 'EOF'
# Try to understand if pingcap/errors preserves error chains
import subprocess
result = subprocess.run(['find', 'pkg', '-name', 'errors.go', '-path', '*/pingcap/errors/*'], capture_output=True, text=True)
print("Found error files:")
print(result.stdout)
EOFRepository: pingcap/tidb
Length of output: 76
🏁 Script executed:
# Let's check the ingestcli WriteClient implementation to see what errors it returns
sed -n '1,50p' pkg/ingestor/ingestcli/ingest_err.goRepository: pingcap/tidb
Length of output: 1539
🏁 Script executed:
# Check what WriteClient interface/method looks like
rg -B5 -A10 'WriteClient.*func' pkg/ingestor/Repository: pingcap/tidb
Length of output: 38
🏁 Script executed:
# Most importantly, let's verify if pingcap/errors implements Unwrap for goerrors.Is compatibility
python3 << 'EOF'
# Check if pingcap/errors in go.mod and look for its implementation
import subprocess
import os
# Find pingcap/errors package in vendor
result = subprocess.run(
['find', 'pkg/vendor/github.com/pingcap/errors', '-name', '*.go'],
capture_output=True, text=True
)
if result.stdout:
files = result.stdout.strip().split('\n')[:3]
for f in files:
print(f"\n=== {f} ===")
with open(f) as fp:
content = fp.read()
# Look for Unwrap method
if 'Unwrap' in content:
lines = content.split('\n')
for i, line in enumerate(lines):
if 'Unwrap' in line:
start = max(0, i-2)
end = min(len(lines), i+8)
print('\n'.join(lines[start:end]))
print('---')
EOFRepository: pingcap/tidb
Length of output: 38
🏁 Script executed:
# Final check: verify the fix would work by checking if errors.Trace returns an error that satisfies goerrors.Is
rg -n 'func.*Trace.*error' pkg/ | grep pingcap || \
grep -r 'func.*Trace' pkg/vendor/github.com/pingcap/errors/ 2>/dev/null | head -5Repository: pingcap/tidb
Length of output: 38
Use goerrors.Is for consistent and robust deadline checking.
Line 289 uses errors.Cause(err) == context.DeadlineExceeded while line 290 already uses goerrors.Is. The current code relies on pingcap/errors to properly unwrap context.DeadlineExceeded, but future code could wrap errors using Go 1.13's %w operator or other patterns that bypass errors.Cause(). Using goerrors.Is(err, context.DeadlineExceeded) handles both current and future wrapping patterns correctly, matching the approach already used on line 290.
Suggested change
- if errors.Cause(err) == context.DeadlineExceeded {
+ if goerrors.Is(err, context.DeadlineExceeded) {
if cause := context.Cause(ctx); goerrors.Is(cause, common.ErrWriteTooSlow) {
tidblogutil.Logger(ctx).Info("experiencing a wait timeout while writing to TiKV")
err = errors.Trace(cause)
}
}📝 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.
| if errors.Cause(err) == context.DeadlineExceeded { | |
| if cause := context.Cause(ctx); goerrors.Is(cause, common.ErrWriteTooSlow) { | |
| if goerrors.Is(err, context.DeadlineExceeded) { | |
| if cause := context.Cause(ctx); goerrors.Is(cause, common.ErrWriteTooSlow) { | |
| tidblogutil.Logger(ctx).Info("experiencing a wait timeout while writing to TiKV") | |
| err = errors.Trace(cause) | |
| } | |
| } |
🤖 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/lightning/backend/local/job_worker.go` around lines 289 - 290, Replace
the direct comparison errors.Cause(err) == context.DeadlineExceeded with a call
to goerrors.Is to consistently detect deadline errors even when wrapped;
specifically, in job_worker.go where the code checks errors.Cause(err) ==
context.DeadlineExceeded (near the block that then inspects context.Cause via
goerrors.Is), change that condition to goerrors.Is(err,
context.DeadlineExceeded) so the deadline check uses the same robust unwrapping
semantics as the subsequent goerrors.Is usage.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: GMHDBJD, joechenrh 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 |
ti-chi-bot
Bot
added
approved
lgtm
and removed
needs-1-more-lgtm
|
/retest |
|
@D3Hunter: PRs from untrusted users cannot be marked as trusted with 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. |
|
/retest |
|
@D3Hunter: PRs from untrusted users cannot be marked as trusted with 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. |
|
/retest |
|
@D3Hunter: PRs from untrusted users cannot be marked as trusted with 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. |
|
@D3Hunter: 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. |
|
/cherry-pick release-nextgen-202603 |
|
@D3Hunter: new pull request created to branch 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 ti-community-infra/tichi repository. |
Resolve conflicts against ~3.5 months of master drift: - pkg/util/codec, pkg/tablecodec: rebase the DESC-aware encoding onto the explicit-collation Encoder introduced by pingcap#69566. EncodeKeyWithDesc becomes an Encoder method (with the package-level wrapper kept for range/probe-key callers), and GenIndexKey dispatches through the caller-supplied encoder on both the ASC fast path and the DESC path. - pkg/executor: thread indexColDescFlags through the refactored buildKeyRanges signature (new rangeMemTracker) and the new unified IndexMergeReaderExecutor.buildPartialWorkerKVRanges. - Resolve files touched only by interim cherry-picks (pingcap#68096, pingcap#68137, pingcap#68148, pingcap#68168) to their canonical upstream versions. - BUILD.bazel: union of dependency edits; adopt upstream shard_count. Verified with go vet + unit tests on all touched packages, the pkg/ddl DESC feature tests (-tags=intest), and a full bazel CI build. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: takaidohigasi <takaidohigasi@gmail.com>
5 participants
What problem does this PR solve?
Issue Number: ref #61702
Problem Summary:
Object store imports use the object store region job worker, which can time out while creating or using the TiKV write client. The write timeout handling was tied to
Backend.doWrite, so object store region jobs did not get the samecommon.ErrWriteTooSlowretry behavior beforerunJobclassified write errors.What changed and how does it work?
Move the 15-minute write timeout wrapper to
regionJobBaseWorker.writeWithTimeout, so both local backend writes and object store region job worker writes are wrapped beforerunJobclassifies retryable write errors. If the context deadline is caused bycommon.ErrWriteTooSlow, return that error so the job is marked retryable and can be rescanned/retried.Remove the duplicate timeout wrapper from
Backend.doWriteand keep the existing short write-timeout failpoint setup. This keeps the timeout behavior in the common region job execution path instead of only in the local backend write implementation.Check List
Tests
./tools/check/failpoint-go-test.sh pkg/lightning/backend/local -run 'Test(CloudRegionJobWorker|RegionJobBaseWorker)$' -count=1make lintSide effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
Bug Fixes
Tests