moderation: added lock mechanism. #101
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alejandromumo
force-pushed
the
add_moderation_locking_mechanism
branch
from
7f02362
to
105a804
Compare
ntarocco
reviewed
Aug 14, 2023
| def execute_moderation_actions(user_id, action): | ||
| @shared_task(ignore_result=True, acks_late=True, retry=True) | ||
| @lock_if_renew( | ||
| lock_prefix="user_moderation_lock", |
There was a problem hiding this comment.
See my comment about this in the invenio-cache PR
ntarocco
reviewed
Aug 15, 2023
There was a problem hiding this comment.
After further discussions also with @zzacharo @jrcastro2, we propose to make everything a bit less sophisticated. The drawbacks I see here (and the PR in invenio-cache) are:
- the decorator becomes quite complex because of the params parsing
- the
lock_if_renewfeels very tight to the usage in this specific context, not very easy to re-use - the lock is acquired even if the service will trigger a permission denied (and released). We might want to call the lock
acquireafter the permission check, so that we also have the user param:
def ...
...
user = UserAggregate.get_record(id_)
if user is None:
raise PermissionDeniedError()
Mutex(id).acquire() # `acquire_or_fail` or somethingIf you want to keep the decorator, here the proposal:
- the 2 decorators should be defined here, and they are used only in their specific context.
lock_user_moderationshould assert theid_as second param, and use it. Similar for the celery task decorator. Such decorators should be used only here, cannot be re-used anywhere else. The only responsibility of the decorator is to calculate the cache key:
...
def decorate(*args, **kwargs):
user_id = args[1]
cache_key = f"user_moderation_lock.{user_id}"
...I would not make configurable the prefix, I would remove USERS_RESOURCES_MODERATION_LOCK_KEY_PREFIX.
- the decorator
lock_if_renewis alsoreleasingthe lock at the end. It should be defined here, and used only here. Given that it is a decorator of a celery task, is it executed when calling it, or when the task starts? If the latter, it might be better to add it as first statement of the task, instead of a decorator, and take advantage of the mutex context manager:
with Mutex(id).acquire_or_renew():
...
# this will call release at the endWDYT?
See also comments in the other PR. Let me know what you think, happy to chat IRL.
I agree that these decorators added a HUGE overhead, it was not worth it. I ended up dropping them and just having a wrapper of Example: def block(identity, id_):
...
self.require_permission(identity, "manage", record=user)
# Throws ``LockAcquireFailed`` if not acquired
ModerationMutex(id_).acquire()
... |
zzacharo
approved these changes
Aug 30, 2023
zzacharo
force-pushed
the
add_moderation_locking_mechanism
branch
3 times, most recently
from
e40aef0
to
606a07d
Compare
zzacharo
force-pushed
the
add_moderation_locking_mechanism
branch
from
606a07d
to
dddaf7c
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
closes #102
NEEDS:
Tests are failing since it needs the PR linked above.
Locally tests are passing: