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

Add AcceptJob #259

Merged
merged 5 commits into from
Feb 1, 2022
Merged

Add AcceptJob #259

merged 5 commits into from
Feb 1, 2022

Conversation

seitau
Copy link
Contributor

@seitau seitau commented Jan 27, 2022

Related to #241 #94

Added AcceptJob to job store which updates job as ACCEPTED in transaction.
This feature will prevent jobs from being executed multiple times concurrently.

@seitau seitau force-pushed the fix-accept branch 8 times, most recently from 903af53 to 08523a9 Compare January 27, 2022 09:27
@seitau seitau changed the title add: accept job Add AcceptJob Jan 27, 2022
.github/workflows/test.yml Outdated Show resolved Hide resolved
@seitau seitau marked this pull request as ready for review January 27, 2022 09:33
@latenssi
Copy link
Member

latenssi commented Jan 31, 2022

@seita-uc Thank you for the PR, it looks great!

Just one thing: could we avoid the usage of psql here if you tried the custom Transaction function I wrote to circumvent sqlite limitation regarding SELECT FOR UPDATE?

The function can be found here: datastore/gorm/transaction.go

An example usage can be found here: keys/store_gorm.go#L55

It should be a drop-in replacement for the Transaction function in the gorm package.

@latenssi
Copy link
Member

Note: we just removed the .env.test file in #258

@seitau
Copy link
Contributor Author

seitau commented Jan 31, 2022

@latenssi Thank you for review! I missed the change of refactoring transactions so I'm gonna fix to follow the existing implementation.

Copy link
Member

@latenssi latenssi left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you @seita-uc

@nanuuki nanuuki merged commit 9b3cd80 into flow-hydraulics:main Feb 1, 2022
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.

3 participants