-
Notifications
You must be signed in to change notification settings - Fork 793
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 support for MongoDB Transactions #715
base: master
Are you sure you want to change the base?
Conversation
Hello people! Do you want me to do something more? Thank you very much! |
Thanks for the PR! Could you elaborate a little more on why this would be a great addition? Thanks! :-) |
If I understand correctly, people want the Agenda CRUD operations to be part of an ACID MongoDB transaction. (This excellent blog post gives me a hint to say so.) Pseudo code of creating a user in a databse:
Either both or none of the documents will be created in the DB. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beside some small questions, this looks good to me. Thanks
@koresar That's it! That way you can schedules jobs as being part of an multi-document transaction condition. Which is cool.
// eslint-disable-next-line prefer-rest-params | ||
noCallback(arguments, 2); | ||
noCallback(arguments, 3); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be a bad reviewer but I don't get why this is changing from 2
to 3
here 😅Can yoy give me some explanations ?
Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will throw an error when passing the new mongoOptions parameter without changing from 2 to 3.
This change may be considered as breaking change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes this is a breaking change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@OmgImAlexis see #1372
@@ -33,6 +34,9 @@ class Job { | |||
this.agenda = args.agenda; | |||
delete args.agenda; | |||
|
|||
this.mongoOptions = args.mongoOptions || {}; | |||
delete args.mongoOptions; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the point of the delete
instruction here ? I'm afraid this code can cause side effects as objects arguments are passed as reference in JS.
What do you think ? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For sure!
Probably I follow the delete from line 35 without thinking about the consequences.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The object should be cloned before mutating.
|
||
describe('transactions', () => { | ||
before(async function () { | ||
if(!mongoURI.includes('replicaSet')){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the condition should relies on v4 checking
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How to check if that's v4?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure in units tests, but I mongo generally you can use this: https://stackoverflow.com/a/16932830/5320409
Maybe we can add a check at instantiation that turn a value in the root class and throw an error if you try to pass a session
object on <v4 function ?
Tell me if you've a better idea ? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your suggestions are perfect.
May I ask what the status is on this PR? |
The status is, I think, abandoned. @shanrauf would you like to finish it? Maybe even restart from the latest master branch? |
@koresar Sure. I'll submit a PR on Wednesday. |
It seems like @LucGranato plans to complete this PR, so I'll hold off on this one. |
Hey, I come back! Since I made this pull request, I'm using this version on my project, and I had zero problems so far. @koresar you are 100% right about the use case, and thanks for your help. @ScreamZ thanks for your review. I just answered your questions about the code. @shanrauf did you made any improvement? Let's finish this PR together. I'm pleased to see this feature going live! |
@LucGranato , looking forward to this landing. Any progress? |
@makinde if your could push this feature forward by solving all the issues everyone would love you. 😃 |
Hi. I’m gonna take a stab at it. Is it fine if I open a new pull req Suggestions
|
Thanks mate! |
Dudes any update on that ? |
Hey guys!
I added support for MongoDB Transactions by using sessions.
Thanks!
Closes #821