perf: keep jobs in waiting list when queue is paused #2769

roggervalf · 2024-09-12T02:07:11Z

This PR removes the legacy code regarding the old paused queue functionality, which was based on renaming the wait list key to paused. As paused keys are now handled as a queue state, we do not need the old mechanism anymore and so the code can be removed and simplified. We need some migration steps for old queues that may be in paused status at the time of the upgrade.

manast

Some initial comments, more will come.

manast · 2024-09-20T09:46:32Z

docs/gitbook/guide/flows/fail-parent.md

@@ -16,13 +16,13 @@ const originalTree = await flow.add({
      name,
      data: { idx: 0, foo: 'bar' },
      queueName: 'childrenQueueName',
-      opts: { failParentOnFailure: true },
+      opts: { onChildFailure: 'fail' },


This is a nice change.

manast · 2024-09-20T09:48:03Z

python/bullmq/scripts.py

@@ -451,6 +462,15 @@ async def retryJobs(self, state: str, count: int, timestamp: int):
        result = await self.commands["moveJobsToWait"](keys=keys, args=args)
        return result

+    async def repairDeprecatedPausedKey(self, maxCount: int):


We need proper documentation on how and when a call to this method is needed.

manast · 2024-09-20T09:49:19Z

src/classes/job.ts

@@ -39,10 +39,8 @@ const logger = debuglog('bull');

 const optsDecodeMap = {
  de: 'debounce',
-  fpof: 'failParentOnFailure',


Won't we need to keep this old mappings to not cause a data breaking change?

Maybe we should introduce a "migration" mechanism here. Something like migration steps for going between versions that are run if required in Lua scripts for atomicity. But we would need to keep a version number in the meta key, adding complexity.

we can keep this mappings that will be only used to format options of jobs with these old values, but no migration is needed as we are evaluation old and new option in lua scripts, new jobs will use new option

manast · 2024-09-20T09:51:35Z

src/classes/job.ts

@@ -1210,27 +1208,6 @@ export class Job<
      throw new Error(`Delay and repeat options could not be used together`);
    }

-    if (this.opts.removeDependencyOnFailure && this.opts.failParentOnFailure) {


nice to be able to remove this

manast · 2024-09-20T09:52:35Z

src/classes/queue.ts

+  /**
+   * Remove legacy markers before v5
+   */
+  removeLegacyMarkers(): Promise<void> {


Documentation about this is also needed. Most importantly practical information in how to apply something like this in the context of a production deployment.

I added a me migrations section for it

manast · 2024-09-20T09:53:31Z

src/classes/queue.ts

+   *
+   * @param maxCount - Max quantity of jobs to be moved to wait per iteration.
+   */
+  async repairDeprecatedPausedKey(maxCount: number = 1000): Promise<void> {


Should "repair" be the proper name or actually something like "migrate" would be more accurate?

I can change it, let me do it

… into refactor-paused

manast · 2024-11-05T21:17:40Z

docs/gitbook/guide/migrations/v6.md

+
+If you have paused queues after upgrading to this version. These jobs will be moved to wait state when initializing any of our instances (Worker, Queue, QueueEvents or FlowProducer).
+
+Paused key is not longer needed as this state is already represented by queue meta key. It also improve the process of pausing or resuming a queue as we don't need to rename any key.


by meta key -> inside the meta key

"It also improves..."

manast · 2024-11-05T21:19:07Z

docs/gitbook/guide/migrations/v6.md

+
+Paused key is not longer needed as this state is already represented by queue meta key. It also improve the process of pausing or resuming a queue as we don't need to rename any key.
+
+## Remove legacy markers


I think we are going to need more detailed information on how to safely perform the migration.

manast · 2024-11-10T09:54:10Z

docs/gitbook/guide/migrations/v6.md

+
+# Migration to v6
+
+Make sure to pass **skipMigrationsExecution** option in any of our instances as false in order to execute all necessary changes when coming from an older version


do you think default should be perform the migration? I think it makes more sense to have the reverse, default is not to do the migration, you must explicitly do it, In fact it would be better to give an error if the queue has not been migrated yet and require you to manually migrate first as part of your deployment steps.

So basically we should have only one way to migrate which would be using the run migrations utility, and not implicitly when the queue is instantiated.

manast · 2024-11-10T09:59:52Z

docs/gitbook/guide/migrations/v6.md

+
+1. Pause your queues.
+2. Upgrade to v6.
+3. Instantiate any instance passing skipMigrationsExecution option as false where migrations will be executed.


it feels a bit awkward to require to use false to actually trigger a one time behaviour and then after that you must use true.

manast · 2024-11-10T10:04:55Z

src/classes/queue-base.ts

+                'Queue has pending migrations. See https://docs.bullmq.io/guide/migrations',
+              );
+            } else {
+              return runMigrations(client, {


I do not think we should silently run migrations here, this just opens a window to shoot ourselves in the foot.

manast · 2024-11-10T10:10:03Z

src/classes/queue.ts

+   *
+   * @param maxCount - Max quantity of jobs to be moved to wait per iteration.
+   */
+  async migrateDeprecatedPausedKey(maxCount = 1000): Promise<void> {


should be moved to a migration step.

manast · 2024-11-10T10:27:03Z

src/commands/migrateDeprecatedPausedKey-2.lua

+    ARGV[1] count
+]]
+
+local maxCount = tonumber(ARGV[1])


I do not understand this script. It should not be possible to have both wait and paused keys at the same time, either it is wait or paused but not both, so basically just renaming the key from paused to wait should be enough as a migration step + setting the queue status to paused.

manast

Just a small comment in the python version.

manast · 2024-11-18T21:56:23Z

python/bullmq/scripts.py

@@ -465,6 +463,15 @@ async def retryJobs(self, state: str, count: int, timestamp: int):
        result = await self.commands["moveJobsToWait"](keys=keys, args=args)
        return result

+    async def migrateDeprecatedPausedKey(self, maxCount: int):


Do we really need this? I think for migration users should use the migration script available in the NodeJs for now.

But what I think we need in the python version is to throw an exception if you are trying to run this version on an older BullMQ version that has not yet been migrated.

I'll add this logic in a following pr

roggervalf added 2 commits September 11, 2024 21:05

refactor: use isQueuePausedOrMaxed instead of getTargetQueueList

481b664

chore: consider moving jobs from paused to wait if there are

2654bb8

roggervalf changed the base branch from master to v6 September 12, 2024 05:43

refactor: obliterate to consider removing from wait

9286d68

roggervalf changed the base branch from v6 to master September 13, 2024 00:42

roggervalf added 6 commits September 12, 2024 19:46

refactor: remove paused key from addStandardJob

1f2f889

refactor: remove paused key scripts

a66ddba

refactor: remove paused key from other scripts

87447a8

refactor: remove paused key on moveToActive

c3acbb9

refactor: remove paused key on moveToFinished

e973ec5

chore: fix moveToFinishedKeys ref

65c3beb

roggervalf changed the base branch from master to v6 September 13, 2024 01:42

chore: add repairPausedKey script

7a93835

roggervalf changed the base branch from v6 to master September 14, 2024 02:51

manast reviewed Sep 20, 2024

View reviewed changes

roggervalf changed the base branch from master to v6 September 20, 2024 12:28

roggervalf added 3 commits September 20, 2024 22:56

chore: fix merge conflicts

0cde0c3

docs(migrations): add new section

537ce42

docs: fix typo

e593d8a

roggervalf changed the base branch from v6 to master September 21, 2024 04:31

roggervalf changed the base branch from master to v6 September 21, 2024 14:10

roggervalf changed the base branch from v6 to master September 24, 2024 03:54

chore: restore few decoded options

efe1a56

roggervalf changed the base branch from master to v6 September 24, 2024 04:24

refactor: add executeMigrations script

42ebfcb

roggervalf changed the base branch from v6 to master September 26, 2024 04:58

Merge branch 'master' into refactor-paused

616e45d

roggervalf changed the base branch from master to v6 September 26, 2024 04:59

roggervalf changed the base branch from v6 to master September 27, 2024 01:29

docs(flows): add missing word in flows guide (#2788)

a8103c0

roggervalf and others added 3 commits October 1, 2024 12:14

refactor(script): consider using rename when no wait jobs

4cd34a1

fix: return client in get client in all paths

50b0313

Merge branch 'refactor-paused' of https://github.com/taskforcesh/bullmq…

9f97ec0

… into refactor-paused

roggervalf changed the base branch from master to v6 October 8, 2024 04:26

chore: fix merge conflicts

1f7245a

roggervalf changed the base branch from v6 to master October 12, 2024 19:50

roggervalf added 7 commits October 15, 2024 22:37

chore: fix merge conflicts

ee33a81

chore: add migrations in list

92d9154

test: fix test cases

268b9d4

docs(guide): update migrations for v6

6e20049

test: waitUntilReady in test cases

d0c4015

feat: add skipMigrationsExecution option

fd3ba17

docs: update v6 clarification for skipMigrationsExecution

82da69a

roggervalf changed the base branch from master to v6 October 18, 2024 05:04

manast reviewed Nov 5, 2024

View reviewed changes

docs: address comments about migrations

f1b85ab

manast reviewed Nov 10, 2024

View reviewed changes

chore: fix merge conflicts

852b41e

roggervalf changed the base branch from v6 to master November 16, 2024 14:56

roggervalf added 2 commits November 16, 2024 12:53

feat(queue-base): add runMigrations method

7514be9

Merge branch 'master' into refactor-paused

e2e3c9d

roggervalf changed the base branch from master to v6 November 16, 2024 18:08

manast approved these changes Nov 18, 2024

View reviewed changes

chore: remove extra method

e370b84

roggervalf changed the base branch from v6 to master November 19, 2024 03:19

chore: fix merge conflicts

ac13902

roggervalf changed the base branch from master to v6 November 19, 2024 03:38

roggervalf merged commit d304267 into v6 Nov 19, 2024
12 checks passed

roggervalf deleted the refactor-paused branch November 19, 2024 03:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf: keep jobs in waiting list when queue is paused #2769

perf: keep jobs in waiting list when queue is paused #2769

roggervalf commented Sep 12, 2024 •

edited by manast

Loading

manast left a comment

manast Sep 20, 2024

manast Sep 20, 2024

manast Sep 20, 2024

manast Sep 20, 2024

roggervalf Sep 21, 2024

manast Sep 20, 2024

manast Sep 20, 2024

roggervalf Sep 21, 2024

manast Sep 20, 2024

roggervalf Sep 21, 2024

manast Nov 5, 2024

manast Nov 5, 2024

manast Nov 5, 2024

manast Nov 10, 2024

manast Nov 10, 2024

manast Nov 10, 2024

manast Nov 10, 2024

manast Nov 10, 2024

manast Nov 10, 2024

manast left a comment

manast Nov 18, 2024

manast Nov 18, 2024

roggervalf Nov 19, 2024


		If you have paused queues after upgrading to this version. These jobs will be moved to wait state when initializing any of our instances (Worker, Queue, QueueEvents or FlowProducer).

		Paused key is not longer needed as this state is already represented by queue meta key. It also improve the process of pausing or resuming a queue as we don't need to rename any key.


		Paused key is not longer needed as this state is already represented by queue meta key. It also improve the process of pausing or resuming a queue as we don't need to rename any key.

		## Remove legacy markers


		# Migration to v6

		Make sure to pass skipMigrationsExecution option in any of our instances as false in order to execute all necessary changes when coming from an older version

perf: keep jobs in waiting list when queue is paused #2769

perf: keep jobs in waiting list when queue is paused #2769

Conversation

roggervalf commented Sep 12, 2024 • edited by manast Loading

manast left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manast left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roggervalf commented Sep 12, 2024 •

edited by manast

Loading