Skip to content
This repository has been archived by the owner on Nov 4, 2021. It is now read-only.

Proposal: Reorganize the codebase #262

Merged
merged 20 commits into from
Mar 22, 2021
Merged

Proposal: Reorganize the codebase #262

merged 20 commits into from
Mar 22, 2021

Conversation

macobo
Copy link
Contributor

@macobo macobo commented Mar 19, 2021

Overview

This PR moves code along the lines proposed in #261. I find it makes it much easier to follow the code flow - what code is being executed in workers, what in main threads and what everywhere.

No real behavioral changes here, just moving files. Some minor changes:

  • Split up server.ts into src/main/pluginsServer and src/shared/server
  • Renamed celery/worker to src/main/ingestion/celery-queue-worker. This avoids confusion with worker threads.

The eventual goal would be to make src/shared as tiny as possible and move everything else under specific subfolders.

Resolves #47.

Commits

  • Move schedule.ts under src/main
  • Move queue.ts under src/main
  • Move kafka-queue logic under src/main
  • Move ingest-event file
  • Move web/server under main.ts
  • Move plugins code under src/worker
  • Move vm code under src/worker
  • Move celery worker under src/main
  • move process-event under src/worker
  • Move src/celery under src/shared
  • Move remaining src/ingestion under src/shared
  • move utils under src/shared
  • Move some top-level code under shared
  • Move main pluginsserver code under src/main
  • Fix typeerror in tests
  • Move createServer under src/shared

Checklist

  • Updated Settings section in README.md, if settings are affected
  • Jest tests

@macobo macobo force-pushed the move-code-around branch from 1c7bf40 to 43e6988 Compare March 19, 2021 10:16
@macobo macobo requested a review from Twixes March 19, 2021 10:31
@macobo macobo changed the title [WIP] Move code along Proposal: Reorganize the codebase Mar 19, 2021
@macobo macobo marked this pull request as ready for review March 19, 2021 10:31
Copy link
Member

@Twixes Twixes left a comment

Choose a reason for hiding this comment

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

I broadly agree. My only two notes are:

  1. I think it's a bit awkward that src/celery/worker.ts got pulled out of the celery directory into src/main/ingestion/celery-queue-worker.ts, especially considering that the celery directory used to be a whole separate package. The analogy to the Kafka queue is good though.
  2. Most of src/server.ts ended up in src/main/pluginsServer.ts, but not all of it. I think we should keep that file intact instead, as basically it's all PluginsServer.

However in any case I'd wait for @mariusandra's input on this too before merging.

@macobo
Copy link
Contributor Author

macobo commented Mar 19, 2021

Thanks for the feedback. Both of these were motivated by trying to keep shared/ as small as possible.

  1. For server.ts I feel strongly. Basically half of the code there only executed/was available in the main thread and is the defacto start point for the whole control flow. The other half is under shared because that god object is used everywhere. I think the issue here is mostly around naming - is Server the right name for the shared state?

  2. For worker.ts I can see it going both ways, I originally thought it was a pure alternative for kafka-queue.ts, but these two behave differently. It just feels a bit awkward having it under shared/ given that it's not used by worker. If it does become a library it will still be easy to extract the code as needed.

@mariusandra
Copy link
Collaborator

Regarding these discussed points:

  1. I guess it's fine to split up the celery code info folder. I moved the lib into our codebase precisely so we could be flexible with this. (plus for other optimisations...)

  2. Totally fine with splitting the "server" and "god" objects from "server.ts". The god object could even be called something like "services.ts" or something, as it basically just hosts connections to all the things we connect to.

Copy link
Collaborator

@mariusandra mariusandra left a comment

Choose a reason for hiding this comment

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

Approved. Some merge conflicts though.

@macobo macobo merged commit 48f9040 into master Mar 22, 2021
@macobo macobo deleted the move-code-around branch March 22, 2021 11:39
fuziontech pushed a commit to PostHog/posthog that referenced this pull request Oct 12, 2021
…ver#262)

* Move schedule.ts under src/main

* Move queue.ts under src/main

* Move kafka-queue logic under src/main

* Move ingest-event file

* Move web/server under main.ts

* Move plugins code under src/worker

* Move vm code under src/worker

* Move celery worker under src/main

* move process-event under src/worker

This one is sort of weird since it gets called in server.ts

* Move src/celery under src/shared

* Move remaining src/ingestion under src/shared

* move utils under src/shared

* Move some top-level code under shared

* Move main pluginsserver code under src/main

* Fix typeerror in tests

* Move createServer under src/shared

* Rename celery worker to be ingestion-specific

* Fix another package.json import

* Fix process-event import
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor src/*.ts
3 participants