-
-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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
message_send: Update do_send_messages
codepath to send event on commit.
#30101
Conversation
b68a24d
to
e1d8a8b
Compare
content: str = "test content", | ||
*, | ||
read_by_sender: bool = True, | ||
skip_capture_on_commit_callbacks: bool = 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.
I think we probably need a docstring or something that explains how a caller should decide whether to pass skip_capture_on_commit_callbacks
... possibly something brief linking to a new section in docs/subsystems/events-system.md
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.
Added docstring.
I think it's not a big enough change to highlight in docs?
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.
Yeah I think the new section would be on capturing on-commit callbacks in general, not this override.
e1d8a8b
to
814055a
Compare
This would be a bit clumsy to review, so I'd just write down the flow in which I made the changes -- that should make it a bit easier to review:
|
One more important change I have made is: Earlier, actions function were in the format of:
Now we need to ensure that the send events calls are also within a transaction. So, I just removed the |
@transaction.atomic(savepoint=False) | ||
# subscribing users to streams; the caller of this function | ||
# use a transaction to ensure that the RealmAuditLog entries | ||
# are created atomically with the Subscription object creation (and updates). | ||
def bulk_add_subs_to_db_with_logging( |
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.
Is there any harm in having @transaction.atomic(savepoint=False)
on this? That usage model is nestable.
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 was just trying to avoid nested transactions as they are not of any help in these cases? Let me know if they help in some way.
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.
@transaction.atomic(savepoint=False)
is a noop if we're already in a transaction -- that's effectively what savepoint=False
does.
So the reason to do it here is just to avoid the risk that code reuses this function incorrectly, and there's little downside. Overall it's a lot better to have a function that is hard to use badly than a function with a comment that says "The function calling this needs to do X" -- less non-local information is required to convince oneself that it's correct.
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.
Yeah, my thought was to remove as it is a noop.
But your reasoning makes sense.
Earlier, we were using 'send_event' & 'queue_json_publish' in 'do_send_messages' which can lead to a situation where we enqueue events but the transaction fails at a later stage. Events should not be sent until we know we're not rolling back.
814055a
to
e726012
Compare
Pushed a change for #30101 (comment) and marked this to merge once CI passes. Thanks @prakhar1144! |
CZO Discussion
Screenshots and screen captures:
Self-review checklist
(variable names, code reuse, readability, etc.).
Communicate decisions, questions, and potential concerns.
Individual commits are ready for review (see commit discipline).
Completed manual review and testing of the following: