-
Notifications
You must be signed in to change notification settings - Fork 4
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 a select_for_update lock #76
Conversation
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.
One potential transaction-related issue, but if it's actually an issue, it's not newly introduced in this MR. Looks good to me.
@@ -510,6 +510,11 @@ def find_root_genome(genome): | |||
|
|||
|
|||
def lock_genome(genome): | |||
""" | |||
Do a select for update, which, when executed inside a transaction, places a |
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.
FYI I only spent two minutes poking into this, but from what I could tell, the places where you call lock_genome
aren't inside transactions. Unless they're happening higher up in the call chain...
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.
Correct. It's upstream caller that's calling the atomic, because you need the locking and the rest of the work to happen all within a transaction. Should I change the method name to lock_genome_call_if_in_transaction? or you think the comment is sufficient.
src/edge/models/fragment.py
Outdated
""" | ||
|
||
fragments = Fragment.objects.select_for_update().filter(pk=self.id) | ||
# Lock only happens when querset is evaluated, therefore need to do at least fragments[0] |
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.
Unless querset
is a "short" name I'm not familiar with, I assume you meant queryset
here. Not that it matters given that it's a comment...
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, typo, will fix. Thanks.
No description provided.