refactor(core): Centralize scaling mode (no-changelog) #9835
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
n8n-assistant
bot
added
core
krynble
reviewed
Jun 24, 2024
There was a problem hiding this comment.
Hopefully we can get rid of all the watchdog functionality
netroy
requested changes
Jul 4, 2024
There was a problem hiding this comment.
I think we should do this in 2 parts:
- part 1 should be the refactor that reorganizes most of our existing code in
packages/cli/src/scaling-modein a way that we can also add lots of unit tests for - part 2 is when we actually make the switch. This should be in v2.
It's entirely possible that we might be able to address some of the issues without the bullmq upgrade, by simply moving away from the super-entangled code that the current worker stuff is.
ivov
changed the title
bullmq
ivov
changed the title
despairblue
reviewed
Jul 19, 2024
tomi
reviewed
Aug 5, 2024
There was a problem hiding this comment.
I like how the code is now better organized into more meaningfully named classes. Left a couple small comments about code documentation, and questions about error handling. Since this has been reviewed already by multiple people before me I trust that the functionality itself works as expected
|
@tomi Thanks for reviewing!
On the contrary :) The reviews have been code-only - I've tested this PR locally, but I'd be thankful if you also can put this through its paces and see if you find any odd behavior, esp. when cancelling jobs, having main respond to webhooks, etc. |
|
@ivov I did some testing. Found at least one issue. I canceled an execution that was in |
|
I also managed to somehow stop the worker from picking up any new work: I set worker concurrency to 1 and had multiple execution queued. I then cancelled the running execution. It did cancel but the worker didn't pick up any new work anymore Worker logs: Redis has: After I stopped the worker and started a new worker the main instance logged this: |
|
The execution duration seems to count the wait time as well (in Queued state). Not sure if this is correct as it has been running only for couple seconds
|
|
Looks like it's not possible to shutdown the main instance if there are active executions. It just keeps logging over and over again: After a while it started also spamming Eventually it died, but it took way longer than the 30s it said: |
|
Shutting down the worker also produced some errors: After this the execution was left running:
|
|
Migrating discussion to this Notion page to keep it organized. |
|
As discussed, since all issues are on master, we'll track them separately and merge this after today's release. |
tomi
reviewed
Aug 7, 2024
| */ | ||
| @Service() | ||
| export class JobProcessor { | ||
| private readonly runningJobs: { [jobId: JobId]: RunningJob } = {}; |
There was a problem hiding this comment.
This would be more readable:
Suggested change
| private readonly runningJobs: { [jobId: JobId]: RunningJob } = {}; | |
| private readonly runningJobs: Record<JobId, RunningJob> = {}; |
There was a problem hiding this comment.
Sorry missed this one, will address in next round 👍🏻
MiloradFilipovic
added a commit
that referenced
this pull request
Aug 7, 2024
* master: refactor(core): Centralize scaling mode (no-changelog) (#9835) fix(editor): Remove body padding from storybook previews (no-changelog) (#10317) feat(MySQL Node): Return decimal types as numbers (#10313) 🚀 Release 1.54.0 (#10315) feat(Elasticsearch Node): Add bulk operations for Elasticsearch (#9940) feat(Stripe Trigger Node): Add Stripe webhook descriptions based on the workflow ID and name (#9956) feat(MongoDB Node): Add projection to query options on Find (#9972) fix(Invoice Ninja Node): Fix payment types (#10196) feat(HTTP Request Tool Node): Use DynamicStructuredTool with models supporting it (no-changelog) (#10246) feat: Return scopes on executions (no-changelog) (#10310) feat(Webflow Node): Update to use the v2 API (#9996) feat(Lemlist Trigger Node): Update Trigger events (#10311) feat(Calendly Trigger Node): Update event names (no-changelog) (#10129) refactor(core): Reorganize webhook related components under src/webhooks (no-changelog) (#10296) docs: Fix links to license files in readme (no-changelog) (#10257) fix(editor): Update design system Avatar component to show initials also when only firstName or lastName is given (#10308) fix(editor): Update tags filter/editor to not show non existing tag as a selectable option (#10297) fix(editor): Update project tabs test (no-changelog) (#10300) fix(core): VM2 sandbox should not throw on `new Promise` (#10298) # Conflicts: # packages/design-system/src/components/N8nAvatar/Avatar.vue
|
Got released with |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Centralize scaling mode logic to pave the way for adding tests, upgrading lib, etc.