refactor(core): Separate listeners in scaling service (no-changelog) #10487
Conversation
n8n-assistant
bot
added
core
ivov
changed the title
| export type JobMessage = RepondToWebhookMessage | AbortJobMessage; | ||
|
|
||
| export type RepondToWebhookMessage = { | ||
| export type MessageToMain = { | ||
| kind: 'respond-to-webhook'; | ||
| executionId: string; | ||
| response: IExecuteResponsePromiseData; | ||
| }; | ||
|
|
||
| export type AbortJobMessage = { | ||
| export type MessageToWorker = { |
There was a problem hiding this comment.
I think these would be clearer if the types were still called RespondToWebhookMessage and AbortJobMessage, as that's what they semantically are. If we want we can then create aliaes for MessagesToMain and MessagesToWorker which alias these types
| * Register listeners on a `main` process for Bull queue events. | ||
| */ | ||
| private registerMainListeners() { | ||
| this.queue.on('global:progress', (_jobId: JobId, msg: MessageToMain) => { |
There was a problem hiding this comment.
I guess the type for msg is not correct, or otherwise we wouldn't need the if (msg.kind === ... ). Now I don't know if there are other events that might be emitted, but we could either
- Type the
msgasunknownand have a type guard that checks based onkindif the message is aRespondToWebhookMessage(i.e. there might be other messages as well) - Type the
msgasRespondToWebhookMessageandassert(msg.kind === ...)(i.e. there are no other message types)
| * - `leader` after bootup in single-main setup, | ||
| * - `leader` or `follower` after bootup in multi-main setup. | ||
| * | ||
| * A non-main instance type (e.g. `worker`) is always `unset`. |
There was a problem hiding this comment.
Should it instead be worker when the instance acts as a worker? Then it would be more "complete"
There was a problem hiding this comment.
So the role an instance has (unset, leader, follower) is only ever relevant for a main - and the role is separate from what kind of instance it is (main, worker, webhook, etc.) which is instanceType.
Ideally we should throw whenever attempting to access instanceRole (or its derived getters) from a non-main instance as this should never happen, but that'd be a bit outside the scope of this PR.
n8n
|
||||||||||||||||||||||||||||
| Project |
n8n
|
| Branch Review |
separate-bull-queue-listeners-for-main-and-workers
|
| Run status |
|
| Run duration | 04m 41s |
| Commit |
|
| Committer | Iván Ovejero |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
419
|
| View all changes introduced in this branch ↗︎ | |
|
✅ All Cypress E2E specs passed |
* master: refactor(core): Use `@/databases/` instead of `@db/` (no-changelog) (#10573) ci: Fix destroy benchmark env workflow (no-changelog) (#10572) feat: Add benchmarking of pooled sqlite (no-changelog) (#10550) refactor(editor): User journey link to n8n.io (#10331) fix(Wait Node): Prevent waiting until invalid date (#10523) refactor(core): Standardize filename casing for controllers and databases (no-changelog) (#10564) refactor(core): Allow custom types on getCredentials (no-changelog) (#10567) fix(editor): Scale output item selector input width with value (#10555) refactor(core): Delete InternalHooks (no-changelog) (#10561) fix(core): Make boolean config value parsing backward-compatible (#10560) fix(Google Sheets Trigger Node): Show sheet name is too long error (#10542) fix(editor): Ensure `Datatable` component renders `All` option (#10525) fix(core): Stop explicit redis client disconnect on shutdown (#10551) ci: Use correct branch for benchmark docker build workflow (no-changelog) (#10552) refactor(core): Separate listeners in scaling service (no-changelog) (#10487)
|
Got released with |
The scaling service currently registers all listeners on all instance types - a leftover from the original implementation. Registering all listeners on all instance types is inefficient and makes the service harder to understand.
This PR separates listeners in the scaling service into...
It also expands test coverage in the scaling service.