Abstract out LockManager interface #7532

ghost · 2020-10-10T21:12:43Z

In order to be able to introduce more locking protocols, we need to abstract out the locking subsystem in TransactionDB into a set of interfaces.

PR #7013 introduces interface LockTracker. This PR is a follow up to take the first step to abstract out a LockManager interface.

Further modifications to the interface may be needed when introducing the first implementation of range lock. But the idea here is to put the range lock implementation based on range tree under the utilities/transactions/lock/range/range_tree.

Test Plan:
point_lock_manager_test

facebook-github-bot

@Cheng-Chang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot

@Cheng-Chang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot

@Cheng-Chang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2020-10-11T22:40:34Z

@cheng-chang has updated the pull request. You must reimport the pull request before landing.

spetrunia · 2020-10-12T10:20:54Z

A question about this call:

class LockManager {
  ...
  // Enable locking for the specified column family.
  // Caller should guarantee that this column family is not already enabled.
  virtual void AddColumnFamily(ColumnFamilyId column_family_id) = 0;

Here, the LockManager is expected to initialize its data structures based on just ColumnFamilyId (not ColumnFamilyHandle).
Current PointLockManager can do this because it doesn't care about the Comparator used, it assumes the comparator is memcmp() (one can see this in class LockMapStripe: the keys are indexed with std::unordered_map<std::string, LockInfo> keys).

A Range-based Lock manager (or a more advanced Point-based lock manager) needs to "know" what Comparator is used.

I can see two solutions:
Solution1: Pass ColumnFamilyHandle* instead (this is what current Range Locking code is using)

Solution2: Assume that the lock manager can somehow call

  db_.versions_.GetColumnFamily(id)->GetComparator();

to get the Comparator. I'm not sure if/how to do that safely though (versions_ is not public, etc).

Should this issue be addressed as part of this patch?

spetrunia · 2020-10-12T15:19:06Z

utilities/transactions/pessimistic_transaction_db.cc

-PessimisticTransactionDB::GetLockStatusData() {
-  return lock_mgr_.GetLockStatusData();
+LockManager::PointLockStatus PessimisticTransactionDB::GetLockStatusData() {
+  return lock_manager_->GetPointLockStatus();


So, now LockManager got two functions, GetPointLockStatus and GetRangeLockStatus.

Should this function (PessimisticTransactionDB::GetLockStatusData) also be replaced with two functions? (And if yes, should this be done in this PR or outside of it?)

I'll add GetRangeLockStatus to TransactionDB's interface.

I talked with @Cheng-Chang about this, and I think that in the future, we should always get status from the lock manager objects, and the methods on PessimisticTransactionDB will go away.

I think the below calls relating to deadlock info would be similar too.

ghost · 2020-10-12T18:16:52Z

A question about this call:
class LockManager {
  ...
  // Enable locking for the specified column family.
  // Caller should guarantee that this column family is not already enabled.
  virtual void AddColumnFamily(ColumnFamilyId column_family_id) = 0;
Here, the LockManager is expected to initialize its data structures based on just ColumnFamilyId (not ColumnFamilyHandle).
Current PointLockManager can do this because it doesn't care about the Comparator used, it assumes the comparator is memcmp() (one can see this in class LockMapStripe: the keys are indexed with std::unordered_map<std::string, LockInfo> keys).

A Range-based Lock manager (or a more advanced Point-based lock manager) needs to "know" what Comparator is used.

I can see two solutions:
Solution1: Pass ColumnFamilyHandle* instead (this is what current Range Locking code is using)

Solution2: Assume that the lock manager can somehow call
  db_.versions_.GetColumnFamily(id)->GetComparator();
to get the Comparator. I'm not sure if/how to do that safely though (versions_ is not public, etc).

Should this issue be addressed as part of this patch?

Solution1 looks good to me, I'll do it in this PR.

facebook-github-bot · 2020-10-12T18:26:55Z

@cheng-chang has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2020-10-12T18:58:12Z

@cheng-chang has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2020-10-12T19:08:29Z

@cheng-chang has updated the pull request. You must reimport the pull request before landing.

ghost · 2020-10-12T19:08:39Z

@spetrunia PTAL

spetrunia · 2020-10-12T20:18:01Z

@Cheng-Chang looks good to me now.

facebook-github-bot

@Cheng-Chang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2020-10-12T22:33:16Z

@cheng-chang has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot

@Cheng-Chang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2020-10-12T22:53:09Z

@cheng-chang has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2020-10-13T19:54:32Z

@cheng-chang has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot

@Cheng-Chang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

this information is exposed through LockManager interface

facebook-github-bot · 2020-10-16T21:33:02Z

@cheng-chang has updated the pull request. You must reimport the pull request before landing.

ajkr

LGTM.

ajkr · 2020-10-17T19:57:48Z

utilities/transactions/lock/lock_manager.cc

+                            const TransactionDBOptions& opt) {
+  assert(db);
+  // TODO: determine the lock manager implementation based on configuration.
+  return new PointLockManager(db, opt);


I have some difficulty imagining what the configuration will look like for choosing between PointLockManager vs. RangeLockManager since my understanding is transactions can have a mix of point and range ops. Some possibilities I was thinking of:

Since Range endpoints are inclusive, RangeLockManager could also lock point keys by setting start == end

There could be a PointAndRangeLockManager composed of a PointLockManager and a RangeLockManager that delegates/coordinates locking requests.

Is the plan one of these, or something else?

The first one. When RangeLockManager is used, acquiring a point lock on $P is the same as acquiring a range lock on $P <= key <= $P.

ghost · 2020-10-19T15:13:29Z

@lth I'm going to merge the PR to unblock Sergey, feel free to leave reviews, I'll address them in a follow up PR.

facebook-github-bot

@Cheng-Chang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2020-10-19T15:15:30Z

@cheng-chang has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot

@Cheng-Chang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2020-10-19T18:36:48Z

@Cheng-Chang merged this pull request in 0ea7db7.

lth

Looks pretty good to me too.

Regarding needing the handle for comparators for AddColumnFamily, write prepared needed to solve a similar problem, so it'd be good if the two solutions converged eventually. This is implemented in UpdateCFComparatorMap. Maybe we can add a TODO?

lth · 2020-10-21T00:18:19Z

include/rocksdb/utilities/transaction_db.h

+  Endpoint start;
+  Endpoint end;
+  std::vector<TransactionID> ids;
+  bool exclusive;


Does range locking support shared locks? Or is this here just for future extensibility?

For future extensibility.

lth · 2020-10-21T00:37:19Z

utilities/transactions/pessimistic_transaction_db.cc

-PessimisticTransactionDB::GetLockStatusData() {
-  return lock_mgr_.GetLockStatusData();
+LockManager::PointLockStatus PessimisticTransactionDB::GetLockStatusData() {
+  return lock_manager_->GetPointLockStatus();


I talked with @Cheng-Chang about this, and I think that in the future, we should always get status from the lock manager objects, and the methods on PessimisticTransactionDB will go away.

I think the below calls relating to deadlock info would be similar too.

Summary: In order to be able to introduce more locking protocols, we need to abstract out the locking subsystem in TransactionDB into a set of interfaces. PR facebook#7013 introduces interface `LockTracker`. This PR is a follow up to take the first step to abstract out a `LockManager` interface. Further modifications to the interface may be needed when introducing the first implementation of range lock. But the idea here is to put the range lock implementation based on range tree under the `utilities/transactions/lock/range/range_tree`. Pull Request resolved: facebook#7532 Test Plan: point_lock_manager_test Reviewed By: ajkr Differential Revision: D24238731 Pulled By: cheng-chang fbshipit-source-id: 2a9458cd8b3fb008d9529dbc4d3b28c24631f463

Cheng Chang added 2 commits October 10, 2020 09:50

move point lock related classes to standalone folder

2d83121

abstract out LockManager interface

448d19b

ghost requested a review from ajkr October 10, 2020 21:12

facebook-github-bot added the CLA Signed label Oct 10, 2020

ghost mentioned this pull request Oct 10, 2020

Range Locking: Allow different LockManagers, add Range Lock definitions #7443

Closed

facebook-github-bot reviewed Oct 10, 2020

View reviewed changes

replace NewLockTracker with LockTrackerFactory

4d3e99c

facebook-github-bot reviewed Oct 10, 2020

View reviewed changes

facebook-github-bot reviewed Oct 11, 2020

View reviewed changes

update builds

81df3f3

spetrunia reviewed Oct 12, 2020

View reviewed changes

spetrunia approved these changes Oct 12, 2020

View reviewed changes

facebook-github-bot reviewed Oct 12, 2020

View reviewed changes

resolve comments from Sergey

609d7ce

facebook-github-bot reviewed Oct 13, 2020

View reviewed changes

ghost requested a review from lth October 16, 2020 20:48

remove GetRangeLockStatus from TransactionDB API

e008aa7

this information is exposed through LockManager interface

ajkr approved these changes Oct 18, 2020

View reviewed changes

facebook-github-bot reviewed Oct 19, 2020

View reviewed changes

Merge branch 'fmaster' into lock-manager

984f6e1

facebook-github-bot reviewed Oct 19, 2020

View reviewed changes

facebook-github-bot closed this in 0ea7db7 Oct 19, 2020

facebook-github-bot added the Merged label Oct 19, 2020

lth reviewed Oct 21, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Abstract out LockManager interface #7532

Abstract out LockManager interface #7532

ghost commented Oct 10, 2020

facebook-github-bot left a comment

facebook-github-bot left a comment

facebook-github-bot left a comment

facebook-github-bot commented Oct 11, 2020

spetrunia commented Oct 12, 2020

spetrunia Oct 12, 2020

ghost Oct 12, 2020 •

edited by ghost

Loading

lth Oct 21, 2020

ghost commented Oct 12, 2020

facebook-github-bot commented Oct 12, 2020

facebook-github-bot commented Oct 12, 2020

facebook-github-bot commented Oct 12, 2020

ghost commented Oct 12, 2020

spetrunia commented Oct 12, 2020

facebook-github-bot left a comment

facebook-github-bot commented Oct 12, 2020

facebook-github-bot left a comment

facebook-github-bot commented Oct 12, 2020

facebook-github-bot commented Oct 13, 2020

facebook-github-bot left a comment

facebook-github-bot commented Oct 16, 2020

ajkr left a comment

ajkr Oct 17, 2020

spetrunia Oct 19, 2020

ghost commented Oct 19, 2020

facebook-github-bot left a comment

facebook-github-bot commented Oct 19, 2020

facebook-github-bot left a comment

facebook-github-bot commented Oct 19, 2020

lth left a comment •

edited

Loading

lth Oct 21, 2020

ghost Oct 21, 2020

lth Oct 21, 2020

Abstract out LockManager interface #7532

Abstract out LockManager interface #7532

Conversation

ghost commented Oct 10, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Oct 11, 2020

spetrunia commented Oct 12, 2020

spetrunia Oct 12, 2020

Choose a reason for hiding this comment

ghost Oct 12, 2020 • edited by ghost Loading

Choose a reason for hiding this comment

lth Oct 21, 2020

Choose a reason for hiding this comment

ghost commented Oct 12, 2020

facebook-github-bot commented Oct 12, 2020

facebook-github-bot commented Oct 12, 2020

facebook-github-bot commented Oct 12, 2020

ghost commented Oct 12, 2020

spetrunia commented Oct 12, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Oct 12, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Oct 12, 2020

facebook-github-bot commented Oct 13, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Oct 16, 2020

ajkr left a comment

Choose a reason for hiding this comment

ajkr Oct 17, 2020

Choose a reason for hiding this comment

spetrunia Oct 19, 2020

Choose a reason for hiding this comment

ghost commented Oct 19, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Oct 19, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Oct 19, 2020

lth left a comment • edited Loading

Choose a reason for hiding this comment

lth Oct 21, 2020

Choose a reason for hiding this comment

ghost Oct 21, 2020

Choose a reason for hiding this comment

lth Oct 21, 2020

Choose a reason for hiding this comment

ghost Oct 12, 2020 •

edited by ghost

Loading

lth left a comment •

edited

Loading