-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
fix race condiftion in calling initialise_reserved_users #4081
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.
lgtm other than the below.
user_id (str): user to add/update | ||
""" | ||
is_insert = yield self.runInteraction( | ||
"upsert_monthly_active_user", self.upsert_monthly_active_user_txn, | ||
user_id | ||
) | ||
# Considered pushing cache invalidation down into txn method, but |
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.
sorry, this isn't terribly easy to read; and I think it's actually more important to put something in the _txn
method. How about getting rid of this, and instead, putting the following in the docstring of the _txn
method:
Note that, after calling this method, it will generally be necessary to invalidate the caches on
user_last_seen_monthly_active
andget_monthly_active_count
. We can't do that here, because we are running in a database thread rather than the main thread, and we can't calltxn.call_after
becausetxn
may not be aLoggingTransaction
.
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.
wfm
|
||
def upsert_monthly_active_user_txn(self, txn, user_id): | ||
"""Updates or inserts monthly active user member | ||
Note that, after calling this method, it will generally be necessary |
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.
Suggest an extra blank line here:
Note that, after calling this method, it will generally be necessary | |
Note that, after calling this method, it will generally be necessary |
No description provided.