implement separate web workers #252

Merged
toast merged 2 commits from web-worker into main 2022-12-03 13:33:24 +00:00
Owner

There are now separate web and queue workers.

The configuration entry clusterLimit has been replaced by clusterLimits which allows separate configuration of web and queue workers.

There are now separate web and queue workers. The configuration entry `clusterLimit` has been replaced by `clusterLimits` which allows separate configuration of web and queue workers.
Johann150 added 1 commit 2022-11-25 12:03:02 +00:00
BREAKING: implement separate web workers
Some checks failed
ci/woodpecker/push/lint-client Pipeline was successful
ci/woodpecker/push/lint-foundkey-js Pipeline was successful
ci/woodpecker/push/lint-backend Pipeline was successful
ci/woodpecker/push/build Pipeline was successful
ci/woodpecker/push/test Pipeline was successful
ci/woodpecker/pr/lint-foundkey-js Pipeline was successful
ci/woodpecker/pr/lint-backend Pipeline failed
ci/woodpecker/pr/build Pipeline was successful
ci/woodpecker/pr/lint-client Pipeline failed
ci/woodpecker/pr/test Pipeline failed
48a60b03ea
There are now separate web and queue workers.

The configuration entry `clusterLimit` has been replaced by
`clusterLimits` which allows separate configuration of web and
queue workers.

Changelog: Changed
toast approved these changes 2022-11-26 12:15:02 +00:00
@ -147,0 +143,4 @@
const modes = ['web', 'queue'];
const total = modes.reduce((acc, mode) => acc + clusterLimits[mode], 0);
if (total > os.cpus().length) {
bootLogger.error(`configuration error: cluster limits total (${total}) must not exceed number of cores (${os.cpus().length})`);
Owner

Is this check actually needed? Running more tasks/processes than you have cores is fine. Syscalls are blocking and the scheduler exists for a reason.
I would have a warning if either type of worker had > 1.5x cpulength workers (different performance characteristics).
Certainly not an error.

Is this check actually needed? Running more tasks/processes than you have cores is fine. Syscalls are blocking and the scheduler exists for a reason. I would have a warning if either type of worker had > 1.5x cpulength workers (different performance characteristics). Certainly not an error.
toast marked this conversation as resolved
@ -147,0 +149,4 @@
const workers = new Array(total);
workers.fill('web', 0, clusterLimits.web);
workers.fill('queue', clusterLimits.web);
Owner

This construction is a little odd and seems to me to be prone to off-by-ones.
I would write it as this:

const web = new Array(clusterLimits.web).map(() => spawnWorker('web'));
const queue = new Array(clusterLimits.queue).map(() => spawnWorker('queue'));
const all = [...web, ...queue]);
console.log(`Starting ${all.length} workers...`);
await Promise.all(all);
This construction is a little odd and seems to me to be prone to off-by-ones. I would write it as this: ```js const web = new Array(clusterLimits.web).map(() => spawnWorker('web')); const queue = new Array(clusterLimits.queue).map(() => spawnWorker('queue')); const all = [...web, ...queue]); console.log(`Starting ${all.length} workers...`); await Promise.all(all); ```
Author
Owner

Your example code wouldn't work because new Array(n) generates an array with n empty slots and map basically ignores those.

Your example code wouldn't work because `new Array(n)` generates an array with `n` empty slots and `map` basically ignores those.
Owner

I've thought about it a bit and it's fine.
Could be a TODO for cleanup later, but this is fine and works as expected.

I've thought about it a bit and it's fine. Could be a TODO for cleanup later, but this is fine and works as expected.
toast marked this conversation as resolved
Owner

I'm going to just remove the check for now and merge it to my instance, see how that goes.
Time for wanton deletes!

I'm going to just remove the check for now and merge it to my instance, see how that goes. Time for wanton deletes!
toast added 1 commit 2022-11-26 12:29:13 +00:00
fixup: make cluster limit into a per-mode warning rather than error
Some checks failed
ci/woodpecker/push/lint-foundkey-js Pipeline was successful
ci/woodpecker/push/lint-client Pipeline was successful
ci/woodpecker/push/lint-backend Pipeline was successful
ci/woodpecker/push/build Pipeline was successful
ci/woodpecker/push/test Pipeline was successful
ci/woodpecker/pr/lint-foundkey-js Pipeline was successful
ci/woodpecker/pr/lint-backend Pipeline failed
ci/woodpecker/pr/build Pipeline was successful
ci/woodpecker/pr/lint-client Pipeline failed
ci/woodpecker/pr/test Pipeline failed
6600f6e52e
toast merged commit 2dde8273e2 into main 2022-12-03 13:33:24 +00:00
toast referenced this pull request from a commit 2022-12-03 13:33:24 +00:00
Johann150 deleted branch web-worker 2022-12-04 13:04:03 +00:00
Sign in to join this conversation.
No reviewers
No labels
feature
fix
upkeep
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Reference: FoundKeyGang/FoundKey#252
No description provided.