feat(repeatable): new repeatables structure #2617

roggervalf · 2024-06-22T13:51:06Z

manast · 2024-06-24T08:49:35Z

src/commands/addRepeatableJob-2.lua

+    table.insert(optionalValues, opts['every'])
+  end
+
+  rcall("HMSET", repeatKey .. ":" .. customKey, "name", opts['name'],


Do we need to use a HMSET here? wondering if we just could store directly the msg pack data in a normal Redis key 🤔

not sure, but I feel more natural to use a hmset if the intention is also to update those options or if we need to get specific values from it without getting all attributes

manast · 2024-06-24T08:57:03Z

src/commands/addRepeatableJob-2.lua

+  return customKey
+end
+
+if ARGV[4] == '0' then


Why is this check needed?

as one of my comments above, worker checks existence to know if it needs to add a new delayed job while queue.add just adds repeatables whithout this check. That's my understanding from the actual logic

manast · 2024-06-24T13:11:12Z

src/classes/repeat.ts

@@ -31,7 +36,7 @@ export class Repeat extends QueueBase {
    skipCheckExists?: boolean,


Do you remember the use case where this boolean was needed?

I was inspecting the code and here is false by default https://github.com/taskforcesh/bullmq/blob/master/src/classes/worker.ts#L721 in worker class (check the existence in case we need to stop adding the next delayed job), here is true https://github.com/taskforcesh/bullmq/blob/master/src/classes/queue.ts#L209 (where we are not checking the existence of that repeatable job and we add the repeatable job)

manast · 2024-06-24T13:16:26Z

src/classes/repeat.ts

@@ -70,23 +75,28 @@ export class Repeat extends QueueBase {
        repeatOpts.jobId = opts.jobId;
      }

-      const repeatJobKey = getRepeatKey(name, repeatOpts);
+      const optionsConcat = getRepeatKey(name, repeatOpts);


Maybe we should rename the function getRepeatKey if this is not going to be the key anymore.

sounds good, what about getQualifiedName

As it is returning a concatenation of options, maybe getRepeatCocatOptions ?

sounds good

manast · 2024-06-24T13:17:53Z

src/classes/repeat.ts

    //
    // Generate unique job id for this iteration.
    //
-    const jobId = this.getRepeatJobId({
-      name,
+    const jobId = this.getRepeatDelayedJobId({


It is important that this jobId is generated in the same way as in the previous version to avoid breaking changes.

it won't be a breaking change in this case as the jobId is only used for new delayed jobs and set it once. Also when removing old cron jobs I'm considering checking the old format before the new one

In the edge case if it adds a delayed job with a different jobId but the same delay time, then you will end up having 2 reputable jobs at least in one iteration, but maybe this edge case cannot happen in practice.

manast · 2024-06-24T13:19:32Z

src/classes/repeat.ts

-    const repeatJobId = this.getRepeatJobId({
+    const optionsConcat = getRepeatKey(name, { ...repeat, jobId });
+    const repeatJobKey = repeat.key ?? this.hash(optionsConcat);
+    const oldRepeatJobId = this.getRepeatJobId({


Maybe the word "legacy" instead of "old" makes this more explicit.

roggervalf · 2024-06-22T15:47:19Z

src/interfaces/repeatable-job.ts

@@ -1,9 +1,10 @@
 export type RepeatableJob = {
  key: string;
  name: string;
-  id: string | null;
+  id?: string | null;


jobId is not a useful attribute now that we can pass a custom key, in new repeatable jobs this value won't be used

roggervalf · 2024-06-25T05:04:36Z

src/commands/addRepeatableJob-2.lua

+    table.insert(optionalValues, opts['every'])
+  end
+
+  rcall("HMSET", repeatKey .. ":" .. customKey, "name", opts['name'],


not sure, but I feel more natural to use a hmset if the intention is also to update those options or if we need to get specific values from it without getting all attributes

roggervalf · 2024-06-25T05:09:35Z

src/classes/repeat.ts

@@ -31,7 +36,7 @@ export class Repeat extends QueueBase {
    skipCheckExists?: boolean,


I was inspecting the code and here is false by default https://github.com/taskforcesh/bullmq/blob/master/src/classes/worker.ts#L721 in worker class (check the existence in case we need to stop adding the next delayed job), here is true https://github.com/taskforcesh/bullmq/blob/master/src/classes/queue.ts#L209 (where we are not checking the existence of that repeatable job and we add the repeatable job)

roggervalf · 2024-06-25T05:23:25Z

src/classes/repeat.ts

@@ -70,23 +75,28 @@ export class Repeat extends QueueBase {
        repeatOpts.jobId = opts.jobId;
      }

-      const repeatJobKey = getRepeatKey(name, repeatOpts);
+      const optionsConcat = getRepeatKey(name, repeatOpts);


sounds good, what about getQualifiedName

roggervalf · 2024-06-25T05:25:18Z

src/classes/repeat.ts

    //
    // Generate unique job id for this iteration.
    //
-    const jobId = this.getRepeatJobId({
-      name,
+    const jobId = this.getRepeatDelayedJobId({


it won't be a breaking change in this case as the jobId is only used for new delayed jobs and set it once. Also when removing old cron jobs I'm considering checking the old format before the new one

roggervalf · 2024-06-25T05:29:55Z

src/commands/addRepeatableJob-2.lua

+  return customKey
+end
+
+if ARGV[4] == '0' then


as one of my comments above, worker checks existence to know if it needs to add a new delayed job while queue.add just adds repeatables whithout this check. That's my understanding from the actual logic

manast

LGTM, however, it would be important to improve the documentation, specially to give answers to the issues #2399 #2596

manast · 2024-07-15T09:37:09Z

src/classes/repeat.ts

      const { immediately, ...filteredRepeatOpts } = repeatOpts;

      // The job could have been deleted since this check
-      if (repeatableExists) {
+      if (repeatJobKey) {


The comment The job could have been deleted since this check is a bit confusing now since the "repeatableExists" boolean is not used anymore.

# [5.10.0](v5.9.0...v5.10.0) (2024-07-16) ### Features * **repeatable:** new repeatables structure ([#2617](#2617)) ref [#2612](#2612) fixes [#2399](#2399) [#2596](#2596) ([8376a9a](8376a9a))

roggervalf force-pushed the feat-new-repeatables branch from 26744f4 to 5372365 Compare June 24, 2024 05:38

manast reviewed Jun 24, 2024

View reviewed changes

roggervalf commented Jun 25, 2024

View reviewed changes

dalechyn mentioned this pull request Jun 29, 2024

[Bug]: Removing repeatable job, does not remove current delayed job #2173

Closed

1 task

doaortu mentioned this pull request Jul 6, 2024

Repeatable Jobs - Ability to Upsert/Remove Job via Job ID Alone #2612

Closed

roggervalf force-pushed the feat-new-repeatables branch 2 times, most recently from 4f584a3 to aca3f19 Compare July 12, 2024 01:35

manast reviewed Jul 15, 2024

View reviewed changes

roggervalf added 10 commits July 16, 2024 06:09

feat(repeatable): save repeatable options in hash

df5afe2

refactor: remove methods

17e90be

refactor: consider repeat hashes when calling obliterate

4229882

chore: remove repeatable delayed job keys

45856e3

chore: include reference

c44a62e

chore: address comments

3952920

chore: change missing variable name

32eaae1

refactor: change method name

9fde263

test: add test case

95d3978

docs(repeatable): add documentation

5c6dd5c

roggervalf force-pushed the feat-new-repeatables branch from f14cf04 to 5c6dd5c Compare July 16, 2024 12:09

roggervalf merged commit 8376a9a into master Jul 16, 2024
10 of 11 checks passed

roggervalf deleted the feat-new-repeatables branch July 16, 2024 12:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(repeatable): new repeatables structure #2617

feat(repeatable): new repeatables structure #2617

roggervalf commented Jun 22, 2024 •

edited

Loading

manast Jun 24, 2024

roggervalf Jun 25, 2024

manast Jun 24, 2024

roggervalf Jun 25, 2024

manast Jun 24, 2024

roggervalf Jun 25, 2024

manast Jun 24, 2024

roggervalf Jun 25, 2024

manast Jun 27, 2024

roggervalf Jul 12, 2024

manast Jun 24, 2024

roggervalf Jun 25, 2024

manast Jun 27, 2024

manast Jun 24, 2024

roggervalf Jun 22, 2024 •

edited by manast

Loading

roggervalf Jun 25, 2024

roggervalf Jun 25, 2024

roggervalf Jun 25, 2024

roggervalf Jun 25, 2024

roggervalf Jun 25, 2024

manast left a comment

manast Jul 15, 2024

		@@ -31,7 +36,7 @@ export class Repeat extends QueueBase {
		skipCheckExists?: boolean,

feat(repeatable): new repeatables structure #2617

feat(repeatable): new repeatables structure #2617

Conversation

roggervalf commented Jun 22, 2024 • edited Loading

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

roggervalf Jun 22, 2024 • edited by manast Loading

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

roggervalf commented Jun 22, 2024 •

edited

Loading

roggervalf Jun 22, 2024 •

edited by manast

Loading