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

Convert worker-manager TCP server to an HTTP server for DX #2063

Merged
merged 2 commits into from
Jan 21, 2025

Conversation

lukemelia
Copy link
Contributor

No description provided.

Copy link

github-actions bot commented Jan 20, 2025

Host Test Results

    1 files  ±0      1 suites  ±0   21m 19s ⏱️ - 1m 13s
733 tests +2  731 ✔️ +2  2 💤 ±0  0 ±0 
738 runs  +2  736 ✔️ +2  2 💤 ±0  0 ±0 

Results for commit 227d220. ± Comparison against base commit a246bac.

♻️ This comment has been updated with latest results.

@lukemelia lukemelia force-pushed the worker-manager-queue-depth branch from 0462c19 to 9f8e449 Compare January 21, 2025 00:02
@lukemelia lukemelia force-pushed the worker-manager-queue-depth branch from 9f8e449 to 84cc7b2 Compare January 21, 2025 00:12
@lukemelia
Copy link
Contributor Author

@habdelra thoughts about whether and where to test this?

@lukemelia lukemelia requested a review from a team January 21, 2025 00:28
Copy link
Contributor

@jurgenwerk jurgenwerk left a comment

Choose a reason for hiding this comment

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

I tested this locally in the browser and I was able to see the stats.

packages/realm-server/worker-manager.ts Outdated Show resolved Hide resolved
@habdelra
Copy link
Contributor

So i think the way I would approach testing for this would be to create a new realm server test module for this. in that module I'd set a random DB name using prepareTestDB() from the realm-server's test helpers. I would then spawn a child process to start the worker manager in the beforeEach(), which in turn will spawn workers and start the HTTP server.. You can hold a reference to the worker manager's process, and in the afterEach() kill the worker manager process which should kill the workers since they are subprocesses of the worker manager. since each test uses a different DB, hopefully that will help to accommodate the async used to spin down the worker in case the torn down workers haven't completed torn down by the time the next test starts. Within the body of each test then, you can use the pg queue's publisher to publish jobs to the queue.

controlling queue depth might be a little tricky for the test. perhaps that means actually you publish jobs to the queue first before you spin up the worker manager--that way you can be sure of how many jobs are in the queue. and after the worker manager spins up you should see the queue depth start to decrease.

@habdelra
Copy link
Contributor

habdelra commented Jan 21, 2025

Also keep in mind that in production right now there are 2 worker managers. because there are 2 ECS tasks in the worker ECS service. How do you intend to expose the worker manager HTTP for each ECS worker task? For state originating from the DB it probably doesn't matter, since it will be the same--but for worker manager specific state, like number of running workers (and perhaps the PID of the workers) this may not necessarily be the same

@lukemelia
Copy link
Contributor Author

After discussion with @habdelra, I am going to update this branch to retain the TCP -> HTTP conversion but drop the queue depth. In a separate PR, I will add an endpoint to realm server to report on queue stats starting with queue depth.

- In a separate PR, we will add an endpoint to realm server to report on queue stats starting with queue depth.
@lukemelia lukemelia changed the title Convert worker-manager TCP server to an HTTP server that exposes queue depth Convert worker-manager TCP server to an HTTP server for DX Jan 21, 2025
@lukemelia lukemelia requested a review from habdelra January 21, 2025 16:56
@lukemelia lukemelia merged commit 7417170 into main Jan 21, 2025
50 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants