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

Adding skiplocked support #9197

Merged
merged 6 commits into from May 2, 2018
Merged

Conversation

joshuacullen
Copy link
Contributor

@joshuacullen joshuacullen commented Mar 18, 2018

Pull Request check-list

Please make sure to review and check all of these items:

  • Does npm run test or npm run test-DIALECT pass with this change (including linting)?
  • Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Have you added new tests to prevent regressions?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description of change

#8910
Adding SKIP LOCKED to lock queries.

Where should I write tests for this?
Where would you like the docs?

@codecov
Copy link

codecov bot commented Mar 18, 2018

Codecov Report

Merging #9197 into master will decrease coverage by 9.96%.
The diff coverage is 50%.

@sushantdhiman
Copy link
Contributor

sushantdhiman commented Mar 20, 2018

You can add integration tests in this file

describe('row locking', () => {

You can add an example of this option here

If you want you can improve docs further as there is no dedicated section for locks, a subsection about locks in transaction docs will be really nice, but not required to merge this PR

@joshuacullen
Copy link
Contributor Author

Great thanks.

I'll take a look this weekend.

@joshuacullen joshuacullen force-pushed the skip-locked branch 2 times, most recently from b095143 to 26df8a5 Compare April 2, 2018 01:12
@joshuacullen
Copy link
Contributor Author

Added tests and some simple docs

return User.findAll({
limit: 1,
lock: true,
skipLocked: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

In first transaction there is no need to skip locks, I assume all rows are free at this time.

return Promise.all([
t1.commit(),
t2.commit()
])
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing semi colon failing tests

@sushantdhiman
Copy link
Contributor

You need to add a skip lock example here

@@ -219,3 +219,26 @@ sequelize.transaction({
The `transaction` option goes with most other options, which are usually the first argument of a method.
For methods that take values, like `.create`, `.update()`, `.updateAttributes()` etc. `transaction` should be passed to the option in the second argument.
If unsure, refer to the API documentation for the method you are using to be sure of the signature.

## Locks
Copy link
Contributor

Choose a reason for hiding this comment

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

Can improve this by adding more examples from here

Copy link
Contributor

@sushantdhiman sushantdhiman left a comment

Choose a reason for hiding this comment

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

A few changes otherwise looks good

@joshuacullen
Copy link
Contributor Author

@sushantdhiman Pushed some changes for the above

username: Support.Sequelize.STRING,
awesome: Support.Sequelize.BOOLEAN
}),
self = this;
Copy link
Contributor

Choose a reason for hiding this comment

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

remove self = this, it should not be required with arrow functions

Copy link
Contributor

@sushantdhiman sushantdhiman left a comment

Choose a reason for hiding this comment

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

Tests for MySQL case are failing for incorrect SQL

@joshuacullen
Copy link
Contributor Author

Okay cool, will have a look. Looks like skip locked is only available in MYSQL 8.

I'll remove for mysql and fix up tomorrow.

@joshuacullen joshuacullen force-pushed the skip-locked branch 3 times, most recently from 9a34b60 to 554783b Compare May 1, 2018 22:54
@joshuacullen
Copy link
Contributor Author

@sushantdhiman Removed support for mysql

@sushantdhiman sushantdhiman merged commit 7b16308 into sequelize:master May 2, 2018
gabegorelick added a commit to gabegorelick/sequelize that referenced this pull request Aug 27, 2019
This was added in sequelize#9197 but not added to the docs for `findAll`.
gabegorelick added a commit to gabegorelick/sequelize that referenced this pull request Aug 28, 2019
This was added in sequelize#9197 but not added to the docs for `findAll`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants