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

Migrate to Actix-web 4 #349

Closed
nyurik opened this issue Jun 8, 2022 · 4 comments · Fixed by #377
Closed

Migrate to Actix-web 4 #349

nyurik opened this issue Jun 8, 2022 · 4 comments · Fixed by #377
Assignees
Milestone

Comments

@nyurik
Copy link
Member

nyurik commented Jun 8, 2022

Actix has made a number of structural/async changes that might require some structural rethinking.

@nyurik nyurik added this to the 0.6.0 milestone Jun 8, 2022
@nyurik nyurik self-assigned this Jun 8, 2022
@nyurik
Copy link
Member Author

nyurik commented Jun 9, 2022

Seems like these steps could solve this, but there is still an unknown:

  • switch main to be async and decorate it with #[actix_web::main]
  • add async to async fn start(args: Args)
  • use web::block(move || check_postgis_version(...)).await.map_err(...)? pattern for all sync calls to PostgreSQL. This way the DB call happens on a separate thread (via the thread pool). Multiple DB calls should be combined together. No heavy processing should be done in the initial handler.

The above means that every single web request will be processed asynchronously by a different thread that initially handled it, and in theory this might result in some performance penalty.

TBD

Martin has some undocumented Actors sprinkled through out the code: DbActor, CoordinatorActor, WorkerActor. Same for the message handlers. It would be good to get some better understanding of what they do/why they are needed, since actix-web user guide doesn't mention actors, only the lower level API does. Are they needed, or can this be done using actix-web concepts?

@sharkAndshark
Copy link
Collaborator

sharkAndshark commented Jul 8, 2022

I am confused about the actor things also when resolving #288 .

Actor Responsibility
worker_actor Update sources list in App State
coordinate_actor Notify worker_actor to update source list in AppState
db_actor Use sources list in AppState to get_tile ,get_sources when called by service handler

Is it resonable manage sources list in AppState without worker and coordinate?

@nyurik
Copy link
Member Author

nyurik commented Jul 26, 2022

Apparently the only reason the lower-lever actors API was used was to support "real time configuration updates" -- in other words if the config changed, Martin would start serving new data immediately from all threads, at the same time. @stepankuzmin would have deeper insight, but at least that's what I understood.

I would like to propose going back to the simpler high-level API, and remove all actors. This may result in some non-synchronous updating, where some threads would use older configuration.

I do not believe this to be a big problem, because

  • in any serious production, a new version would be rolled out using things like kubernetes, with the new docker image starting with the new config, and the older one shutting down - makes it easy to roll back.
  • it is never a good idea to serve different content from the same URL due to browser and CDN caching -- a new version should simply use a new URL, e.g. /1/z/x/y.mvt -> /2/z/x/y.mvt -> ..., and update the managed URL in a single manifest file. This way older clients would continue to get consistent older vision of the map, and the client could periodically refresh the manifest and reload all tiles when they see updated version.

Translation of the original notes by @stepankuzmin

  1. When starting Martin, find all sources (tables and functions) that can be used to generate tiles
  2. share that data structure between actors
  3. if there is a request to update the sources, update the data structure and again share it between the actors

Without these steps with sharing between actors, it may happen that one actor knows about the new data source, but not yet in another, and the client may receive 404 from actors who do not know about new sources.

@stepankuzmin
Copy link
Collaborator

I like the idea of a simpler API. Watch mode intention is to improve the developer experience, i.e., live data reloading during local development. Unfortunately, this complicates the codebase and keeps us from moving forward. I agree to drop the watch mode altogether, but we should communicate that change to the users.

nyurik added a commit that referenced this issue Aug 15, 2022
…ons (#377)

* remove all actions and other low-level magic code, making it more straightforward for the most common usage
* replace r2d2 with bb8 to make it all async
* use first significant version in cargo.toml - this makes it easier to maintain

This fixes #349
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants