Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Fix worker doc for synapse.app.frontend_proxy #13451

Closed
wants to merge 5 commits into from
Closed

Fix worker doc for synapse.app.frontend_proxy #13451

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fb5e393
Fix worker doc for `synapse.app.frontend_proxy`
dklimpel Aug 3, 2022
eaa25d8
newsfile
dklimpel Aug 3, 2022
12877da
Apply suggestions from code review
dklimpel Aug 4, 2022
f7297ab
Change wording from `not cached` to `uncached`
dklimpel Aug 9, 2022
78501b3
update newsfile
dklimpel Aug 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/13451.doc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Corrects misleading wording in worker documentation for `synapse.app.frontend_proxy`.
dklimpel marked this conversation as resolved.
Show resolved Hide resolved
4 changes: 2 additions & 2 deletions docs/workers.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -589,15 +589,15 @@ regular expressions:

^/_matrix/client/(r0|v3|unstable)/keys/upload

If `use_presence` is False in the homeserver config, it can also handle REST
If `use_presence` is set to `false` in the homeserver config, it can also handle REST
dklimpel marked this conversation as resolved.
Show resolved Hide resolved
endpoints matching the following regular expressions:
dklimpel marked this conversation as resolved.
Show resolved Hide resolved

^/_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 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@richvdh richvdh Aug 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

dklimpel marked this conversation as resolved.
Show resolved Hide resolved
must therefore be configured with the location of the main instance, via
the `worker_main_http_uri` setting in the `frontend_proxy` worker configuration
file. For example:
Expand Down