Support bypassing resolved locks for read - #249
Conversation
ti-chi-bot
Bot
added
dco-signoff: yes
|
Warning Review limit reached
Next review available in: 18 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughLockResolver now supports read-lock bypass decisions and asynchronous cleanup of eligible locks. Cluster starts the resolver worker on its thread pool and stops it during destruction. Pending work is bounded and coordinated with mutex, condition-variable, and atomic state. ChangesLock resolution lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Cluster
participant ThreadPool
participant LockResolver
participant resolveLocksImpl
Cluster->>ThreadPool: enqueue backgroundResolve()
LockResolver->>LockResolver: inspect locks and queue eligible cleanup
ThreadPool->>LockResolver: run backgroundResolve()
LockResolver->>resolveLocksImpl: resolve queued lock batches
Cluster->>LockResolver: stopBgResolve() during destruction
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
include/pingcap/kv/Cluster.h (1)
65-74: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider stopping the background resolver before its dependencies during shutdown.
lock_resolver->stopBgResolve()runs afterrpc_client->stop()andregion_cache->stop(), butbackgroundResolve()depends on both (viaresolveLocksImpl/RegionClient). If the worker is mid-batch when the destructor starts, it may keep issuing RPCs/region lookups against already-stopping components until it next observesstopped— caught bybackgroundResolve's try/catch, but potentially producing spurious warnings and adding shutdown latency bounded bybgResolveLockMaxBackoff(5s). MovingstopBgResolve()earlier (beforerpc_client->stop()/region_cache->stop()) lets the worker exit promptly.♻️ Suggested reordering
~Cluster() { - rpc_client->stop(); - mpp_prober->stop(); - if (region_cache) - region_cache->stop(); if (lock_resolver) lock_resolver->stopBgResolve(); + rpc_client->stop(); + mpp_prober->stop(); + if (region_cache) + region_cache->stop(); thread_pool->stop(); }🤖 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 `@include/pingcap/kv/Cluster.h` around lines 65 - 74, Reorder the Cluster destructor shutdown sequence so lock_resolver->stopBgResolve() runs before rpc_client->stop() and region_cache->stop(). Keep the existing conditional checks and stop the thread_pool afterward, ensuring the background resolver exits before its RPC and region-cache dependencies are stopped.
🤖 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 `@include/pingcap/kv/Cluster.h`:
- Around line 65-74: Reorder the Cluster destructor shutdown sequence so
lock_resolver->stopBgResolve() runs before rpc_client->stop() and
region_cache->stop(). Keep the existing conditional checks and stop the
thread_pool afterward, ensuring the background resolver exits before its RPC and
region-cache dependencies are stopped.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8ea8fd8c-019e-4747-8041-dae48440094b
📒 Files selected for processing (5)
include/pingcap/kv/Backoff.hinclude/pingcap/kv/Cluster.hinclude/pingcap/kv/LockResolver.hsrc/kv/Cluster.ccsrc/kv/LockResolver.cc
ti-chi-bot
Bot
added
needs-1-more-lgtm
| if (lock_resolver) | ||
| { | ||
| thread_pool->enqueue([this] { | ||
| lock_resolver->backgroundResolve(); |
There was a problem hiding this comment.
[P1] Reserve a worker for background lock resolution
I’m Codex reviewer. The production Cluster(pd_addrs, config) constructor creates only three pool workers, but startBackgroundTasks() now enqueues four non-returning loops in order: RPC maintenance, MPP probing, region-cache updates, and this resolver. The first three therefore occupy every worker until shutdown, leaving backgroundResolve() queued forever. As a result, the bounded pending-lock queue is never drained, eventually reaches its 4096-lock limit, and foreground reads fall back to synchronous lock resolution. Please increase the pool size or give the resolver a dedicated worker, and ideally add a liveness test that verifies pending locks are actually consumed.
There was a problem hiding this comment.
Good catch. You are right that this can starve backgroundResolve(): the production Cluster had 3 FixedThreadPool workers but now schedules 4 long-running loops, so the resolver task could remain queued forever. I fixed this in ea3eedb by making the worker count match the number of non-returning background tasks. Production Cluster now reserves 4 workers, and the mock/test Cluster now also initializes oracle +
lock_resolver and reserves 3 workers. I also moved stopBgResolve() before stopping RPC/RegionCache so shutdown order matches the resolver dependencies.
Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>
Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>
Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>
584b1ff to
ea3eedb
Compare
ti-chi-bot
Bot
added
dco-signoff: no
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 (1)
include/pingcap/kv/Cluster.h (1)
71-77: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftBackground lock resolution is not synchronized with cluster shutdown.
stopBgResolveonly sets a flag, and the worker is joined later bythread_pool->stop(). Between those two points the destructor stopsrpc_clientandregion_cache, so an in-flightresolveLocksImplcan issue requests against stopped subsystems. The stop flag is also not observed inside a longBackoffer(bgResolveLockMaxBackoff)sleep.
include/pingcap/kv/Cluster.h#L71-L77: calllock_resolver->stopBgResolve()and thenthread_pool->stop()beforerpc_client->stop(),mpp_prober->stop(), andregion_cache->stop(), so the worker is joined before its dependencies stop.src/kv/LockResolver.cc#L632-L656: make the worker exit promptly afterstoppedis set, for example by checkingstoppedinside the per-batch loop before each RPC and by bounding the background backoff, so the join inthread_pool->stop()does not wait for a full backoff cycle.🤖 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 `@include/pingcap/kv/Cluster.h` around lines 71 - 77, Synchronize lock-resolver shutdown before stopping its dependencies: in include/pingcap/kv/Cluster.h lines 71-77, call stopBgResolve and then thread_pool->stop before stopping rpc_client, mpp_prober, or region_cache. In src/kv/LockResolver.cc lines 632-656, update the background worker loop to check stopped before each RPC and bound or interrupt the Backoffer(bgResolveLockMaxBackoff) wait so joining exits promptly.
🧹 Nitpick comments (1)
src/kv/LockResolver.cc (1)
148-156: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider enqueuing one batch per transaction instead of one per lock.
Each bypassed lock creates a separate queue entry through
addPendingLocksForBgResolve(caller_start_ts, {lock}). A scan that hits many locks of the same transaction therefore consumes many queue slots againstmaxPendingLocksForBgResolve, and the worker resolves each entry with its ownBackoffer. This repeatsgetTxnStatusFromLockandResolveLockrequests for the sametxn_id.Collecting bypassed locks in a local per-
txn_idvector and enqueuing once after the loop would reduce both queue pressure and RPC volume.🤖 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 `@src/kv/LockResolver.cc` around lines 148 - 156, Update the lock-scan flow around addPendingLocksForBgResolve to collect bypassed locks grouped by txn_id in local vectors, then enqueue one batch per transaction after the scan instead of enqueueing each lock immediately. Preserve pushed tracking and synchronous fallback when background enqueueing fails, while ensuring background resolution does not enqueue again.
🤖 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.
Outside diff comments:
In `@include/pingcap/kv/Cluster.h`:
- Around line 71-77: Synchronize lock-resolver shutdown before stopping its
dependencies: in include/pingcap/kv/Cluster.h lines 71-77, call stopBgResolve
and then thread_pool->stop before stopping rpc_client, mpp_prober, or
region_cache. In src/kv/LockResolver.cc lines 632-656, update the background
worker loop to check stopped before each RPC and bound or interrupt the
Backoffer(bgResolveLockMaxBackoff) wait so joining exits promptly.
---
Nitpick comments:
In `@src/kv/LockResolver.cc`:
- Around line 148-156: Update the lock-scan flow around
addPendingLocksForBgResolve to collect bypassed locks grouped by txn_id in local
vectors, then enqueue one batch per transaction after the scan instead of
enqueueing each lock immediately. Preserve pushed tracking and synchronous
fallback when background enqueueing fails, while ensuring background resolution
does not enqueue again.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: eafd08f8-53a7-4b15-8481-3cfd92ad7ad5
📒 Files selected for processing (5)
include/pingcap/kv/Backoff.hinclude/pingcap/kv/Cluster.hinclude/pingcap/kv/LockResolver.hsrc/kv/Cluster.ccsrc/kv/LockResolver.cc
🚧 Files skipped from review as they are similar to previous changes (3)
- include/pingcap/kv/Backoff.h
- src/kv/Cluster.cc
- include/pingcap/kv/LockResolver.h
Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>
ea3eedb to
a84c53d
Compare
ti-chi-bot
Bot
added
dco-signoff: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ekexium, gengliqi 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 |
3 participants
What problem does this PR solve?
This PR adds a best-effort path for read requests to bypass locks whose transaction status has already been determined, reducing repeated lock handling in read paths.
What is changed and how it works?
Summary by CodeRabbit
New Features
Bug Fixes