Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: allow procedure to acquire share lock #3061

Merged
merged 14 commits into from
Jan 3, 2024

Conversation

WenyXu
Copy link
Member

@WenyXu WenyXu commented Dec 31, 2023

I hereby agree to the terms of the GreptimeDB CLA

What's changed and what's your intention?

  1. Implement KeyRwLock
  2. Use KeyRwLock instead of LockMap
  3. Use StringKey instead of String
  4. remove redundant code

Background

During reading the paper(Granularity of Locks and Degrees of Consistency in a Shared Data Base).

I realized that to acquire a lock on the subtree, there are two ways:

  • Implicitly locking: Hierarchical locking, Root-to-Leaf.
  • Explicitly locking: Leaf-to-Root (Locks an entire subtree is to lock each node of the subtree in leaf-to-root order)

We're stuck in the first way and always imagine we must acquire the lock in Root-to-Leaf order and need to employ something like a Hierarchical lock.

In fact, the order acquiring locks doesn't really matter in our scenario. We only have three levels, and it's enough to meet the invariant in our system if we acquire one lock on each level.

Invariant in our system

  1. The same table's (different) Region migration procedures are compatible.
  2. The same DB's Ops, DDL, and Region migration procedures are incompatible with one another.

image

image

Checklist

  • I have written the necessary rustdoc comments.
  • I have added the necessary unit tests and integration tests.
  • This PR does not require documentation updates.

Refer to a related PR or issue link (optional)

#2700

@github-actions github-actions bot added docs-not-required This change does not impact docs. Size: L labels Dec 31, 2023
@WenyXu WenyXu marked this pull request as ready for review January 1, 2024 03:56
Copy link

codecov bot commented Jan 1, 2024

Codecov Report

Attention: 12 lines in your changes are missing coverage. Please review.

Comparison is base (6070e88) 85.45% compared to head (cfc3fb1) 85.17%.
Report is 11 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3061      +/-   ##
==========================================
- Coverage   85.45%   85.17%   -0.29%     
==========================================
  Files         799      805       +6     
  Lines      129155   130891    +1736     
==========================================
+ Hits       110373   111481    +1108     
- Misses      18782    19410     +628     

@WenyXu WenyXu force-pushed the feat/key-rwlock branch 2 times, most recently from de56075 to 5bf86d5 Compare January 1, 2024 09:36
@WenyXu WenyXu marked this pull request as draft January 2, 2024 09:31
@MichaelScofield
Copy link
Collaborator

That paper is inspiring, maybe we need to adopt the whole idea from it, including lock scheduler and deadlock detection.

@WenyXu
Copy link
Member Author

WenyXu commented Jan 2, 2024

That paper is inspiring, maybe we need to adopt the whole idea from it, including lock scheduler and deadlock detection.

For deadlock detection, we now simply acquire all locks in the lexicographical order. I think it enough for now. cc @evenyag @fengjiachun

@WenyXu WenyXu marked this pull request as ready for review January 2, 2024 10:11
@WenyXu WenyXu self-assigned this Jan 2, 2024
@WenyXu WenyXu requested a review from NiwakaDev January 2, 2024 10:36
@fengys1996 fengys1996 self-requested a review January 3, 2024 03:14
src/common/procedure/src/local/rwlock.rs Outdated Show resolved Hide resolved
src/common/procedure/src/local/rwlock.rs Outdated Show resolved Hide resolved
@WenyXu
Copy link
Member Author

WenyXu commented Jan 3, 2024

Copy link
Collaborator

@fengjiachun fengjiachun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@evenyag evenyag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rest LGTM

src/common/procedure/src/local/rwlock.rs Outdated Show resolved Hide resolved
@fengjiachun fengjiachun added this pull request to the merge queue Jan 3, 2024
Merged via the queue into GreptimeTeam:main with commit aa22f9c Jan 3, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-not-required This change does not impact docs.
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

5 participants