Skip to content
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

make notification of signatures work with workers #6254

Merged
merged 6 commits into from Nov 1, 2019

Conversation

@uhoreg
Copy link
Member

uhoreg commented Oct 25, 2019

The UNION query is kind of ugly, but shows one way of fixing the issue. Basically, the function needs to return the rows from the user signature stream in addition to the device lists stream.

Alternatively, I could use a new stream ID generator instead of using the device stream ID generator, which would involve more code. Though I suspect that it would be the better option.

uhoreg added 2 commits Oct 25, 2019
Copy link
Member

richvdh left a comment

looks generally ok, though I think it should probably be a separate stream. I don't know if a separate stream id is required though. @erikjohnston do you have any thoughts on how this stuff is meant to work?

@erikjohnston

This comment has been minimized.

Copy link
Member

erikjohnston commented Oct 29, 2019

Yeah, you probably want it to be a separate stream, but can share stream ID generator. (current_state_deltas and events do this).

Adding a stream is a matter of creating a new stream, similar to https://github.com/matrix-org/synapse/blob/master/synapse/replication/tcp/streams/federation.py, and adding it to the stream map https://github.com/matrix-org/synapse/blob/master/synapse/replication/tcp/streams/__init__.py. Then in the worker stores that use the stream add code to stream_positions() and process_replication_rows() e.g. https://github.com/matrix-org/synapse/blob/master/synapse/replication/slave/storage/devices.py#L40-L43

result["device_lists"] = self._device_list_id_gen.get_current_token()
result["user_signature"] = result[
"device_lists"
] = self._device_list_id_gen.get_current_token()

This comment has been minimized.

Copy link
@uhoreg

uhoreg Oct 30, 2019

Author Member

This is kind of ugly, but it's what black came up with. 🤷‍♂

Alternatively, I could set result["user_signature"] and result["device_lists"] as separate statements.

This comment has been minimized.

Copy link
@richvdh

richvdh Oct 31, 2019

Member

Alternatively, I could set result["user_signature"] and result["device_lists"] as separate statements.

this, please. use a local temp var for the token and copy it to both fields. and add a comment to say they share a stream id.

@uhoreg uhoreg requested a review from richvdh Oct 30, 2019
Copy link
Member

richvdh left a comment

just a couple of nits

result["device_lists"] = self._device_list_id_gen.get_current_token()
result["user_signature"] = result[
"device_lists"
] = self._device_list_id_gen.get_current_token()

This comment has been minimized.

Copy link
@richvdh

richvdh Oct 31, 2019

Member

Alternatively, I could set result["user_signature"] and result["device_lists"] as separate statements.

this, please. use a local temp var for the token and copy it to both fields. and add a comment to say they share a stream id.

@@ -42,14 +42,19 @@ def __init__(self, db_conn, hs):

def stream_positions(self):
result = super(SlavedDeviceStore, self).stream_positions()
result["device_lists"] = self._device_list_id_gen.get_current_token()
result["user_signature"] = result[

This comment has been minimized.

Copy link
@richvdh

richvdh Oct 31, 2019

Member

I know the existing code isn't good for this, but I'd rather we used the symbolic constants (UserSignatureStream.NAME in this case) for the stream names, which makes it much easier to find the places the streams are used.

return result

def process_replication_rows(self, stream_name, token, rows):
if stream_name == "device_lists":
self._device_list_id_gen.advance(token)
for row in rows:
self._invalidate_caches_for_devices(token, row.user_id, row.destination)
elif stream_name == "user_signature":

This comment has been minimized.

Copy link
@richvdh

richvdh Oct 31, 2019

Member
Suggested change
elif stream_name == "user_signature":
elif stream_name == UserSignatureStream.NAME:
@uhoreg uhoreg requested a review from richvdh Oct 31, 2019
Copy link
Member

richvdh left a comment

lgtm

@uhoreg uhoreg merged commit 3b4216f into develop Nov 1, 2019
20 checks passed
20 checks passed
buildkite/synapse Build #5319 passed (23 minutes, 46 seconds)
Details
buildkite/synapse/check-sample-config Passed (1 minute, 37 seconds)
Details
buildkite/synapse/check-style Passed (2 minutes, 6 seconds)
Details
buildkite/synapse/isort Passed (18 seconds)
Details
buildkite/synapse/mypy Passed (31 seconds)
Details
buildkite/synapse/newspaper-newsfile Passed (16 seconds)
Details
buildkite/synapse/packaging Passed (19 seconds)
Details
buildkite/synapse/pipeline Passed (5 seconds)
Details
buildkite/synapse/python-3-dot-5-slash-postgres-9-dot-5 Passed (16 minutes, 58 seconds)
Details
buildkite/synapse/python-3-dot-5-slash-sqlite Passed (7 minutes, 2 seconds)
Details
buildkite/synapse/python-3-dot-5-slash-sqlite-slash-old-deps Passed (9 minutes, 51 seconds)
Details
buildkite/synapse/python-3-dot-6-slash-sqlite Passed (7 minutes, 19 seconds)
Details
buildkite/synapse/python-3-dot-7-slash-postgres-11 Passed (21 minutes, 25 seconds)
Details
buildkite/synapse/python-3-dot-7-slash-postgres-9-dot-5 Passed (18 minutes, 18 seconds)
Details
buildkite/synapse/python-3-dot-7-slash-sqlite Passed (6 minutes, 51 seconds)
Details
buildkite/synapse/synapse-port-db-slash-python-3-dot-5-slash-postgres-9-dot-5 Passed (1 minute, 15 seconds)
Details
buildkite/synapse/synapse-port-db-slash-python-3-dot-7-slash-postgres-11 Passed (1 minute, 14 seconds)
Details
buildkite/synapse/sytest-python-3-dot-5-slash-postgres-9-dot-6-slash-monolith Passed (15 minutes, 55 seconds)
Details
buildkite/synapse/sytest-python-3-dot-5-slash-postgres-9-dot-6-slash-workers Passed (15 minutes, 17 seconds)
Details
buildkite/synapse/sytest-python-3-dot-5-slash-sqlite-slash-monolith Passed (11 minutes, 43 seconds)
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.