Deduplicate membership changes #700
Conversation
|
Is there a reason to only deduplicate joins, or could this be used for all membership changes? |
| @@ -183,6 +185,44 @@ def update_membership( | |||
| third_party_signed=None, | |||
| ratelimit=True, | |||
| ): | |||
| if action == Membership.JOIN: | |||
| result = self.join_cache.get((room_id, target)) | |||
There was a problem hiding this comment.
Should remote_room_hosts and third_party_signed be part of the request key here?
erikjohnston
force-pushed
the
erikj/deduplicate_joins
branch
3 times, most recently
from
a4edf78 to
19cecd3
Compare
|
@NegativeMjark PTAL |
|
LGTM |
| new_defer = defer.Deferred() | ||
| self.key_to_defer[key] = new_defer | ||
|
|
||
| def remove_if_current(_): |
There was a problem hiding this comment.
Might not want to throw away the thing passed to the callback.
There was a problem hiding this comment.
Oh, I see. This is only ever passed the None from _trigger_defer_manager
There was a problem hiding this comment.
Would it be clearer to merge the _trigger_defer_manager and the remove_if_current into one thing?
There was a problem hiding this comment.
Possibly, though I think having them split up makes it clearer what each thing is doing.
There was a problem hiding this comment.
I think having two functions and an addBoth is less clear in this case.
@contextmanager
def manager():
try:
yield
finally:
d = self.key_to_defer.get(key)
if d is new_defer:
self.key_to_defer.pop(key, None)
d.callback(None)
There was a problem hiding this comment.
Well, your example doesn't actually work, as we specifically don't hit the callback for the most current defer in the list.
I've changed it though I think it just makes things look more complicated.
|
Maybe add a few comments explaining how the thing is supposed to work inside the |
|
|
||
| @defer.inlineCallbacks | ||
| def queue(self, key): | ||
| current_defer = self.key_to_defer.setdefault(key, None) |
There was a problem hiding this comment.
Why is this setdefault rather than get?
erikjohnston
force-pushed
the
erikj/deduplicate_joins
branch
from
19cecd3 to
639cd07
Compare
|
|
||
| new_defer.addBoth(remove_if_current) | ||
|
|
||
| yield current_defer |
There was a problem hiding this comment.
Is this going to have the right log context when it resolves?
erikjohnston
force-pushed
the
erikj/deduplicate_joins
branch
from
ccc9e70 to
ee5aef6
Compare
| yield | ||
| finally: | ||
| d.callback(None) | ||
| d = self.key_to_defer.get(key) |
There was a problem hiding this comment.
might want to use a different d to avoid confusion.
|
LGTM |
No description provided.