logs: resolve lock by ekexium · Pull Request #1901 · tikv/client-go

ekexium · 2026-02-26T10:11:46Z

Add more logs in the resolve lock paths.

The only concern is when conflicts are frequent, the logs can flood.

Summary by CodeRabbit

Chores
- Enhanced diagnostic logging for transaction lifecycle: clearer informational and debug logs for two-phase commit cleanup, batch rollback RPCs, pessimistic rollback flows, and lock resolution paths to aid troubleshooting and observability.

Signed-off-by: ekexium <[email protected]>

ti-chi-bot · 2026-02-26T10:11:51Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign myonkeminta for approval. For more information see the Code Review Process.
Please ensure that each of them provides their approval before proceeding.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

txnkv/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

coderabbitai · 2026-02-26T10:12:03Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: deafc2bc-b9f0-4a1d-b7f9-63323a6c7166

📥 Commits

Reviewing files that changed from the base of the PR and between f7fdd6b and 09a9685.

📒 Files selected for processing (1)

txnkv/transaction/2pc.go

🚧 Files skipped from review as they are similar to previous changes (1)

txnkv/transaction/2pc.go

📝 Walkthrough

Walkthrough

Adds and adjusts logging across two-phase commit cleanup, batch cleanup RPCs, pessimistic rollback, and lock resolution; introduces a local helper mapping transaction status to action labels. Changes are logging-only and do not alter exported signatures or control-flow outcomes.

Changes

Cohort / File(s)	Summary
2PC cleanup logging `txnkv/transaction/2pc.go`	Adjusts final cleanup logging: emits `Info` for non-pessimistic secondary-lock cleanup and `Debug` for pessimistic rollback paths; failure logging unchanged.
Batch cleanup RPC logging `txnkv/transaction/cleanup.go`	Adds an info-level log at the end of 2PC secondary-lock batch rollback RPC reporting `txnStartTS`, `regionID`, and `keyCount`.
Pessimistic rollback logging `txnkv/transaction/pessimistic.go`	Adds a debug log after successful `PessimisticRollback` RPC including `txnStartTS`, computed `forUpdateTS`, `regionID`, and `keyCount`; no control-flow or return-value changes.
Lock resolver logging & helper `txnkv/txnlock/lock_resolver.go`	Adds a non-exported `resolveActionLabel(status TxnStatus) string` helper and inserts informational logging across resolve paths (`resolveRegionLocks`, `resolveLock`, `resolvePessimisticLock`) to emit action ("commit"/"rollback"), timestamps, region, and flags. No exported API changes.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐇 I hopped through logs both keen and bright,
I logged the rollbacks, commits, and light,
Timestamps, regions, keys in tidy flight,
A helper whispers "commit" or "rollback" right,
I nibble bugs away and leave the traces light.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The pull request title 'logs: resolve lock' directly and accurately describes the main change: adding logging to resolve-lock code paths.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

🧹 Nitpick comments (1)

txnkv/txnlock/lock_resolver.go (1)
1266-1276: Consider log level for high-frequency paths.

This Info-level log is emitted when a lock is already resolved by CheckTxnStatus, which could be frequent in workloads with high lock contention. Consider whether Debug level might be more appropriate for the "skipped rpc" cases to reduce log volume in production, while keeping Info level for actual RPC completions.

That said, if enhanced observability is the explicit goal of this PR, Info level is acceptable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@txnkv/txnlock/lock_resolver.go` around lines 1266 - 1276, The Info-level log
inside the resolved-by-check block should be lowered to Debug to avoid
high-volume logging on frequent "skipped rpc" paths: update the
logutil.Logger(bo.GetCtx()).Info call in the block guarded by resolveLite &&
bytes.Equal(l.Key, l.Primary) (where resolveAction is from resolveActionLabel
and status provides CommitTS) to use Debug (preserving the same message and zap
fields: action, txnStartTS, commitTS, resolveLite, resolvedByCheckTxnStatus) so
observability is kept but noisy Info logs are avoided in production.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@txnkv/txnlock/lock_resolver.go`:
- Around line 1266-1276: The Info-level log inside the resolved-by-check block
should be lowered to Debug to avoid high-volume logging on frequent "skipped
rpc" paths: update the logutil.Logger(bo.GetCtx()).Info call in the block
guarded by resolveLite && bytes.Equal(l.Key, l.Primary) (where resolveAction is
from resolveActionLabel and status provides CommitTS) to use Debug (preserving
the same message and zap fields: action, txnStartTS, commitTS, resolveLite,
resolvedByCheckTxnStatus) so observability is kept but noisy Info logs are
avoided in production.

ℹ️ Review info

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between dfa54d8 and 7ae3421.

📒 Files selected for processing (4)

txnkv/transaction/2pc.go
txnkv/transaction/cleanup.go
txnkv/transaction/pessimistic.go
txnkv/txnlock/lock_resolver.go

Copilot

Pull request overview

Adds more informational logging around txn lock resolution and 2PC secondary cleanup to make “resolve lock” paths easier to trace.

Changes:

Log “rpc finished” and “skipped rpc” events for resolve-lock / resolve-region / resolve-pessimistic-lock paths.
Introduce a small helper to label resolve action as commit vs rollback.
Promote 2PC cleanup completion log level from Debug to Info and add per-batch rollback logs.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
txnkv/txnlock/lock_resolver.go	Adds structured Info logs for resolve RPC completion/skip and a helper for action labeling.
txnkv/transaction/pessimistic.go	Adds Info log after secondary pessimistic rollback batch handling.
txnkv/transaction/cleanup.go	Adds Info log for secondary cleanup batch rollback completion.
txnkv/transaction/2pc.go	Changes cleanup “done” log to Info with new wording.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

txnkv/transaction/pessimistic.go

txnkv/txnlock/lock_resolver.go

Copilot · 2026-02-26T10:17:10Z

txnkv/transaction/2pc.go

 				zap.Bool("isPessimistic", c.isPessimistic), zap.Bool("isOnePC", c.isOnePC()))
 		} else {
-			logutil.Logger(ctx).Debug("2PC clean up done",
+			logutil.Logger(ctx).Info("2PC secondary lock cleanup finished",


This changes a previously-Debug completion log to Info. If 2PC cleanup runs frequently, this can significantly increase log volume in production. Consider keeping this at Debug, adding sampling/rate limiting, or gating with a verbosity/config flag so Info-level remains high-signal.

Suggested change

logutil.Logger(ctx).Info("2PC secondary lock cleanup finished",

logutil.Logger(ctx).Debug("2PC secondary lock cleanup finished",

It means the query fails because of some internal errors which should not happen most of the times, and the error would be returned to the client.

We could check if there is exact prewrite lock rollback or cleanup, only print it in this path. The pessimistic lock rollback does not matter.

Another option is to print related information in the rollback or resolve action where keys are also printable.

Signed-off-by: ekexium <[email protected]>

cfzjywxk · 2026-03-24T14:07:59Z

txnkv/transaction/2pc.go

-				zap.Bool("isOnePC", c.isOnePC()))
+				zap.Uint64("txnStartTS", c.startTS), zap.Bool("isOnePC", c.isOnePC()))
+		} else {
+			logutil.Logger(ctx).Debug("2PC pessimistic lock rollback finished",


Should the prewrite lock rollback which happens in situations like when pessimistic prewrite fails also be printed?

Signed-off-by: ekexium <[email protected]>

logs: resolve lock

7ae3421

Signed-off-by: ekexium <[email protected]>

ti-chi-bot bot added the dco-signoff: yes Indicates the PR's author has signed the dco. label Feb 26, 2026

ekexium requested a review from Copilot February 26, 2026 10:11

ti-chi-bot bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 26, 2026

coderabbitai bot reviewed Feb 26, 2026

View reviewed changes

Copilot AI reviewed Feb 26, 2026

View reviewed changes

ekexium requested review from MyonKeminta and cfzjywxk February 26, 2026 12:15

apply suggestions from review

f7fdd6b

Signed-off-by: ekexium <[email protected]>

cfzjywxk reviewed Mar 24, 2026

View reviewed changes

apply suggestions from review

09a9685

Signed-off-by: ekexium <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

logs: resolve lock#1901

logs: resolve lock#1901
ekexium wants to merge 3 commits intotikv:masterfrom
ekexium:feat/self-lock-cleanup-logs

ekexium commented Feb 26, 2026 •

edited by coderabbitai bot

Loading

Uh oh!

ti-chi-bot bot commented Feb 26, 2026

Uh oh!

coderabbitai bot commented Feb 26, 2026 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Poem

❌ Failed checks (1 warning)

Uh oh!

coderabbitai bot left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI Feb 26, 2026

Uh oh!

cfzjywxk Feb 26, 2026

Uh oh!

cfzjywxk Mar 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	logutil.Logger(ctx).Info("2PC secondary lock cleanup finished",
	logutil.Logger(ctx).Debug("2PC secondary lock cleanup finished",

Conversation

ekexium commented Feb 26, 2026 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

ti-chi-bot bot commented Feb 26, 2026

Uh oh!

coderabbitai bot commented Feb 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Poem

❌ Failed checks (1 warning)

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

cfzjywxk Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

cfzjywxk Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ekexium commented Feb 26, 2026 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Feb 26, 2026 •

edited

Loading