Support bypassing resolved locks for read - #249
Conversation
ti-chi-bot
Bot
added
dco-signoff: yes
|
Warning Review limit reached
Next review available in: 40 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 Run ID: 📒 Files selected for processing (2)
📝 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🧪 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 |
Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>
Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>
![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
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
[LGTM Timeline notifier]Timeline:
|
2 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