Fix worker doc for `synapse.app.frontend_proxy` by dklimpel · Pull Request #13451 · matrix-org/synapse

dklimpel · 2022-08-03T20:09:58Z

Fixes: #3717

Pull Request Checklist

Pull request is based on the develop branch
Pull request includes a changelog file. The entry should:
- Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
- Use markdown where necessary, mostly for code blocks.
- End with either a period (.) or an exclamation mark (!).
- Start with a capital letter.
- Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
Pull request includes a sign off
Code style is correct
(run the linters)

Signed-off-by: Dirk Klimpel dirk@klimpel.org

docs/workers.md

DMRobertson · 2022-08-04T11:59:18Z

docs/workers.md

 `PUT` effectively a no-op.

-It will proxy any requests it cannot handle to the main synapse instance. It
+It will proxy not cached requests to the main synapse instance. It


As of #6964, it looks like the frontend_proxy is really just a generic_worker in disguise. Should we move the frontend_proxy to the "Historical apps" section below?

(I'm very unfamiliar with the history here, maybe @matrix-org/synapse-core can advise).

There are more "Historical apps", IMO.

synapse/synapse/app/generic_worker.py

Lines 430 to 442 in 6dd7fa1

assert config.worker.worker_app in (

"synapse.app.appservice",

"synapse.app.client_reader",

"synapse.app.event_creator",

"synapse.app.federation_reader",

"synapse.app.federation_sender",

"synapse.app.frontend_proxy",

"synapse.app.generic_worker",

"synapse.app.media_repository",

"synapse.app.pusher",

"synapse.app.synchrotron",

"synapse.app.user_dir",

)

synapse.app.media_repository

synapse.app.pusher

synapse.app.federation_sender

Yes indeed, there is absolutely no difference between a frontend_proxy worker and a generic_worker nowadays; we should just get rid of this section.

... which makes me realise that generic_worker can actually handle requests to ^/_matrix/client/(r0|v3|unstable)/keys/upload, and we should add it to the list up there.

Looks like ^/_matrix/client/(api/v1|r0|v3|unstable)/presence/ is already in the list.

I am not sure what I should do? IMO a redesign of the docs should not part of this PR.
Moving URLs to generic_worker has impact to docker image. Or deprecate three worker types is a bigger change.

The only changes we need (for the documentation) are:

remove the section on frontend_proxy

add /_matrix/client/(r0|v3|unstable)/keys/upload to the list of URLs supported by generic_worker. It is currently supported, but not documented.

Moving URLs to generic_worker has impact to docker image.

we may need to update the docker image to match the recommendations from the documentation, but that can be done separately.

I do not see this in this PR. This is a bit more.

frontend_proxy needs to be deprecated

It is not a simple move/add /_matrix/client/(r0|v3|unstable)/keys/upload, it needs an extra paragraph for the config param worker_main_http_uri, what is needed for the URL

It would make sense to create a PR for depracte all of the redundant workers:

synapse.app.media_repository

synapse.app.pusher

synapse.app.federation_sender

I do not see this in this PR.

I can't insist, but it just doesn't seem worth making minor corrections to text that is fundamentally outdated.

This is a bit more.

frontend_proxy needs to be deprecated

I don't think we need a particular deprecation notice. frontend_proxy is just another example of the redundant workers that are covered under Historical apps, much like client_reader and event_creator (which were removed in #7969).

It is not a simple move/add /_matrix/client/(r0|v3|unstable)/keys/upload, it needs an extra paragraph for the config param worker_main_http_uri, what is needed for the URL

Oh bother, that's true. Yes, we should really move the existing text on worker_main_http_uri to the general Worker configuration section. (and probably mention it again next to /_matrix/client/(r0|v3|unstable)/keys/upload.

It would make sense to create a PR for depracte all of the redundant workers:

synapse.app.media_repository

synapse.app.pusher

synapse.app.federation_sender

media_repository isn't (yet) redundant, afaik: a generic_worker cannot handle the /media endpoints. But in general I think it makes more sense to consider each worker app in turn, in separate PRs. #10441 tracks this work.

docs/workers.md

DMRobertson · 2022-08-04T12:00:25Z

Back in the queue for another set of eyes.

Co-authored-by: David Robertson <david.m.robertson1@gmail.com>

changelog.d/13451.doc

richvdh · 2022-08-04T17:53:49Z

docs/workers.md

 `PUT` effectively a no-op.

-It will proxy any requests it cannot handle to the main synapse instance. It
+It will proxy not cached requests to the main synapse instance. It


Yes indeed, there is absolutely no difference between a frontend_proxy worker and a generic_worker nowadays; we should just get rid of this section.

... which makes me realise that generic_worker can actually handle requests to ^/_matrix/client/(r0|v3|unstable)/keys/upload, and we should add it to the list up there.

Looks like ^/_matrix/client/(api/v1|r0|v3|unstable)/presence/ is already in the list.

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

richvdh · 2022-08-10T10:04:45Z

docs/workers.md

+If `use_presence` is set to `false` in the homeserver config, the frontend proxy can also handle REST
+endpoints matching the following regular expression:

    ^/_matrix/client/(api/v1|r0|v3|unstable)/presence/[^/]+/status

 This "stub" presence handler will pass through `GET` request but make the
 `PUT` effectively a no-op.


It really doesn't matter whether use_presence is set to false or not: any worker can now handle ^/_matrix/client/.../presence/[^/]+/status. It is no longer a "stub".

Fix worker doc for synapse.app.frontend_proxy

fb5e393

dklimpel requested a review from a team as a code owner August 3, 2022 20:09

newsfile

eaa25d8

DMRobertson reviewed Aug 4, 2022

View reviewed changes

DMRobertson requested a review from a team August 4, 2022 12:00

Apply suggestions from code review

12877da

Co-authored-by: David Robertson <david.m.robertson1@gmail.com>

richvdh suggested changes Aug 4, 2022

View reviewed changes

dklimpel and others added 2 commits August 9, 2022 08:51

Change wording from not cached to uncached

f7297ab

update newsfile

78501b3

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

richvdh reviewed Aug 10, 2022

View reviewed changes

richvdh mentioned this pull request Aug 10, 2022

Remove the concept of worker "types" #10441

Open

6 tasks

richvdh mentioned this pull request Aug 26, 2022

Remove documentation of legacy frontend_proxy worker app #13645

Merged

dklimpel closed this Aug 29, 2022

dklimpel deleted the patch-3 branch August 31, 2022 17:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix worker doc for `synapse.app.frontend_proxy`#13451

Fix worker doc for `synapse.app.frontend_proxy`#13451
dklimpel wants to merge 5 commits intomatrix-org:developfrom
dklimpel:patch-3

dklimpel commented Aug 3, 2022

Uh oh!

Uh oh!

Uh oh!

DMRobertson Aug 4, 2022

Uh oh!

dklimpel Aug 4, 2022

Uh oh!

richvdh Aug 4, 2022

Uh oh!

dklimpel Aug 9, 2022

Uh oh!

richvdh Aug 9, 2022

Uh oh!

dklimpel Aug 10, 2022

Uh oh!

richvdh Aug 10, 2022 •

edited

Loading

Uh oh!

Uh oh!

DMRobertson commented Aug 4, 2022

Uh oh!

Uh oh!

richvdh Aug 4, 2022

Uh oh!

richvdh Aug 10, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	assert config.worker.worker_app in (
	"synapse.app.appservice",
	"synapse.app.client_reader",
	"synapse.app.event_creator",
	"synapse.app.federation_reader",
	"synapse.app.federation_sender",
	"synapse.app.frontend_proxy",
	"synapse.app.generic_worker",
	"synapse.app.media_repository",
	"synapse.app.pusher",
	"synapse.app.synchrotron",
	"synapse.app.user_dir",
	)

Uh oh!

Conversation

dklimpel commented Aug 3, 2022

Pull Request Checklist

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

richvdh Aug 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DMRobertson commented Aug 4, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

richvdh Aug 10, 2022 •

edited

Loading