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

Integrating LockBox to DBTransaction - introducing lockMulti #14

Merged
merged 4 commits into from
Jun 30, 2022

Conversation

CMCDragonkai
Copy link
Member

@CMCDragonkai CMCDragonkai commented Jun 27, 2022

Description

As well as defaulting some types, this should simplify it usage inside js-db where we are using LockBox to enable PCC locking for the transactions.

Issues Fixed

Tasks

  • 1. Add Lockable as a default type for all the generic types depending on it
  • [ ] 2. Extend the LockRequest type so that string can be taken, and if so, it is assumed that this is just a Lock normally - done on user side
  • 3. Make lock releasers idempotent
  • 4. Enable the ability to do lock mapping for LockBox, where we return a collection of ResourceAcquire<L> instead of a single resource acquisition, this is useful for DBTransaction as it has to manage locking imperatively
  • 5. Changed the LockRequest type to MultiLockRequest, this breaks some type signatures, so this is a major upgrade

Final checklist

  • Domain specific tests
  • Full tests
  • Updated inline-comment documentation
  • Lint fixed
  • Squash and rebased
  • Sanity check the final build

@CMCDragonkai CMCDragonkai marked this pull request as draft June 27, 2022 08:36
@ghost
Copy link

ghost commented Jun 27, 2022

👇 Click on the image for a new way to code review
  • Make big changes easier — review code in small groups of related files

  • Know where to start — see the whole change at a glance

  • Take a code tour — explore the change with an interactive tour

  • Make comments and review — all fully sync’ed with github

    Try it now!

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map Legend

@CMCDragonkai
Copy link
Member Author

I have to remove LockBox.unlock because it doesn't make sense for LockBox when used with RWLockReader and RWLockWriter. The DBTransaction has to replicate some of the functionality of LockBox.

See: MatrixAI/js-db#19 (comment)

The core problem is that the Lockable interface prevents us from allowing lock to return a "collection" of releases. Instead only 1 release function can be returned, and that is the composite of the smaller releases. There is no way to decompose ResourceRelease into smaller ResourceRelease.

Alternatively, LockBox exposes a different function that gives back all the release information, the users still have to keep track of lockReleasers: Map<string, ResourceRelease>, but they could use the alternative function which does the same as LockBox.lock but is given more useful information at the end. Rather than a single ResourceAcquire, it returns a collection of ResourceAcquire. It would also need to present the key information as it has been re-ordered.

Something like this:

  public lockMany(...requests: Array<LockRequest<L>>): Array<[ToString, ResourceAcquire<L>]> {
    return [];
  }

@CMCDragonkai CMCDragonkai self-assigned this Jun 28, 2022
@CMCDragonkai CMCDragonkai changed the title WIP: Default lock constructor for LockBox should be Lock WIP: LockBox integration into DBTransaction - introducing lockMulti Jun 29, 2022
@CMCDragonkai CMCDragonkai changed the title WIP: LockBox integration into DBTransaction - introducing lockMulti WIP: Integrating LockBox to DBTransaction - introducing lockMulti Jun 29, 2022
…ion of keys as separate resources

* Defaulted lock type to `write` for `RWLockReader` and `RWLockWriter`

BREAKING CHANGE: `LockRequest` is now `MultiLockRequest`
@CMCDragonkai CMCDragonkai marked this pull request as ready for review June 30, 2022 10:13
@CMCDragonkai CMCDragonkai changed the title WIP: Integrating LockBox to DBTransaction - introducing lockMulti Integrating LockBox to DBTransaction - introducing lockMulti Jun 30, 2022
@CMCDragonkai CMCDragonkai merged commit 6504452 into staging Jun 30, 2022
@CMCDragonkai
Copy link
Member Author

This will update to 3.0.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

1 participant