-
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
use Transactions for agenda #1372
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Haris Sulaiman <[email protected]>
Signed-off-by: Haris Sulaiman <[email protected]>
Signed-off-by: Haris Sulaiman <[email protected]>
Signed-off-by: Haris Sulaiman <[email protected]>
Signed-off-by: Haris Sulaiman <[email protected]>
Signed-off-by: Haris Sulaiman <[email protected]>
Signed-off-by: Haris Sulaiman <[email protected]>
Signed-off-by: Haris Sulaiman <[email protected]>
Signed-off-by: Haris Sulaiman <[email protected]>
Signed-off-by: Haris Sulaiman <[email protected]>
Signed-off-by: Haris Sulaiman <[email protected]>
I would appreciate it if someone could help me with tests. |
Signed-off-by: Haris Sulaiman <[email protected]>
I took a look at the changes. They are great! |
@koresar Sure I will post it there. I will take my time completing this pr then. |
Signed-off-by: Haris Sulaiman <[email protected]>
Signed-off-by: Haris Sulaiman <[email protected]>
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.
@harisvsulaiman This is nice work and has pointed me towards some reading that I'm probably overdue on wrt transactions in mongo. I've left a couple of style comments and will take a closer look at testing options
Is there any more context you can give on the tests you have started on and where you see the primary blockers?
On the topic of your questions:
Are we going to support mongo pre 4.0? if so how?
I would vote no support for 4.0, but see some options for handling it, from simplest to more involved: document that sessions require 4.0 (minimum); check DB version on connect, throw error if enabled and <4; Log warning and only enable sessions if >= 4.0.
Should we pass session to the job processor to make it fully atomic?
My gut is to leave that to a separate PR but I don't have sufficient context to say one way or the other
should we introduce any breaking changes to the api or is it perfect the way it is?
In my read tHru of the changes, I think the appending of session as the last arg is a fair implementation and not overly invasive. Do you have specific breaking changes in mind?
@@ -172,6 +186,8 @@ class Job<T extends JobAttributesData = JobAttributesData> { | |||
} | |||
} | |||
|
|||
this.shouldUseTransactions = args.shouldUseTransactions ?? false; |
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.
Could be evaluated to an empty string instead of a Boolean if shouldUseTransactions: ''
. Technically no impact since both are falsely, but better IMO to use double negation when interpreting default-false args/flags.
@@ -12,110 +13,127 @@ export const run = async function (this: Job): Promise<Job> { | |||
const { agenda } = this; | |||
const definition = agenda._definitions[this.attrs.name]; | |||
|
|||
let session: ClientSession | undefined = undefined; |
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.
Nit: personal preference for eslint's no-undef-init, but I'd have to take a closer look elsewhere in5e project for consistency over preference
const { MongoMemoryServer } = require("mongodb-memory-server"); | ||
const { | ||
MongoMemoryServer, | ||
MongoMemoryReplSet, |
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.
At a glance, this import doesn't seem to be used. Is this a dev artifact, mistakenly unused, or serve so,e other function?
@pdfowler Thank you for the feedback.
We can check if sessions are supported when we initialize agenda. Then disable it altogether.
I think we should make that optional too. I haven’t gotten around to writing tests for this implementation, I would appreciate it if you could lend a hand. |
Hi all, I was looking into this out of curiosity. Am I right that the main use case for this is that a job's database queries are then handled by one "transaction"? First I was thinking that this is a very interesting idea, but I don't really get it now:
For the other operations, what would the expected benefit be? Don't get me wrong, I really want to support your submission, but I don't fully understand it yet? |
Hello, any update on this? |
Purpose
Users want the Agenda CRUD operations to be part of an ACID MongoDB transaction. (This excellent blog post gives me a hint to say so.)
Given the following scenario with pseudo code of creating a user in a databse:
... the multi document transaction ensures ACID compliance, so that either both or none of the documents will be created in the DB. If step 2 fails, step 1 will be rolled back/not committed.
Todo
See https://www.mongodb.com/blog/post/performance-best-practices-transactions-and-read--write-concerns
Questions
Ps: This is based on the stale pr #715
@koresar @simison
Signed-off-by: Haris Sulaiman [email protected]