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

Split out registration to worker #4666

Merged
merged 6 commits into from Feb 18, 2019

Conversation

Projects
None yet
4 participants
@erikjohnston
Copy link
Member

erikjohnston commented Feb 18, 2019

This allows registration to be handled by a worker, though the actual
write to the database still happens on master.

Note: due to the in-memory session map all registration requests must be
handled by the same worker.

erikjohnston added some commits Feb 18, 2019

Split out registration to worker
This allows registration to be handled by a worker, though the actual
write to the database still happens on master.

Note: due to the in-memory session map all registration requests must be
handled by the same worker.
@erikjohnston

This comment has been minimized.

Copy link
Member Author

erikjohnston commented Feb 18, 2019

Note: the changes to synapse/storage/registration.py are simply moving APIs into the RegistrationWorkerStore

@erikjohnston erikjohnston requested a review from matrix-org/synapse-core Feb 18, 2019

Show resolved Hide resolved changelog.d/4666.feature Outdated
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Feb 18, 2019

Codecov Report

Merging #4666 into develop will decrease coverage by 0.18%.
The diff coverage is 65.21%.

@@             Coverage Diff             @@
##           develop    #4666      +/-   ##
===========================================
- Coverage    75.27%   75.08%   -0.19%     
===========================================
  Files          338      340       +2     
  Lines        34620    34828     +208     
  Branches      5669     5723      +54     
===========================================
+ Hits         26060    26152      +92     
- Misses        6967     7070     +103     
- Partials      1593     1606      +13
Update changelog.d/4666.feature
Co-Authored-By: erikjohnston <erikj@jki.re>
@richvdh
Copy link
Member

richvdh left a comment

generally lgtm

Show resolved Hide resolved synapse/app/client_reader.py
Show resolved Hide resolved synapse/rest/client/v2_alpha/register.py Outdated
initial_display_name = content["initial_display_name"]
is_guest = content["is_guest"]

device_id = yield self.device_handler.check_device_registered(

This comment has been minimized.

@richvdh

richvdh Feb 18, 2019

Member

I wish we didn't have to duplicate this code between the rest code and here. Can it not find a handler to go and live in?

This comment has been minimized.

@erikjohnston

erikjohnston Feb 18, 2019

Author Member

I put it in RegistrationHandler. I was going to put it in LoginHandler, then AuthHandler and then DeviceHandler, but they all had various problems that made me cry, so I gave up and put it in RegistrationHandler.

@erikjohnston

This comment has been minimized.

Copy link
Member Author

erikjohnston commented Feb 18, 2019

I'm afraid I had to fix the tests to not mock out all the registration handler stuff, as otherwise I would have ended up mocking out literally everything to make the tests pass, which feels counter productive.

@richvdh
Copy link
Member

richvdh left a comment

lgtm

@erikjohnston erikjohnston merged commit fc2c245 into develop Feb 18, 2019

7 checks passed

ci/circleci: sytestpy2merged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy2postgresmerged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3merged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3postgresmerged Your tests passed on CircleCI!
Details
codecov/patch 65.21% of diff hit (target 0%)
Details
codecov/project 75.08% (target 0%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.