-
Notifications
You must be signed in to change notification settings - Fork 437
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add a queue registry #3030
base: master
Are you sure you want to change the base?
Conversation
@@ -0,0 +1 @@ | |||
export const BullMQRegistryKey = 'bullmq:registry'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we use bull as prefix for all our keys, do we really need to change that pattern?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, because this is a global key, if we used a prefix then we would not be able to auto-discover the queues defeating the whole purpose. The idea is that a UI can easily get all the queues available in a Redis instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no sure why we are not be able to auto-discover queues. I think we should reuse same prefix that users provides with prefix option. Imagine the scenario where 2 different prefixes are use in the same redis instance. With this key, auto-discovery will return a combination of all queues independent of these prefixes, I think discovery should depend on the prefix that is passed by users or if not passed, use bull as the default value that is used by our other keys
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to segment the queues in prefixes you can still use this approach as the queue names are stored in the registry fully qualified. The problem we are having is that auto discovery is too slow as you must check every key in Redis to match a certain pattern, currently this one *:*:id
so some users complain that it takes too much time it can even time out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to segment the queues in prefixes you can still use this approach as the queue names are stored in the registry fully qualified
That means that users need to get all queue qualified names from discovery, then filter by prefix and then they will get all the queues related to that prefix.
The problem we are having is that auto discovery is too slow as you must check every key in Redis to match a certain pattern, currently this one ::id so some users complain that it takes too much time it can even time out.
I'm in favor of this functionality, my only concern is that we should respect the use of the prefix that could be provided by the user. I also use prefix for separation of queues and seeing some other users using custom prefixes. Please take in count that it would be the same functionality, the only consideration is to use the prefix that users provide when creating queues, our default value is bull
@@ -1464,6 +1465,7 @@ export class Scripts { | |||
const client = await this.queue.client; | |||
|
|||
const keys: (string | number)[] = [ | |||
BullMQRegistryKey, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have this class for handling queue keys, https://github.com/taskforcesh/bullmq/blob/master/src/classes/queue-keys.ts we can add a new method to handle global keys
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, this is not really a Queue key, so I am not sure it fits so well on a class that generates queue keys :/
…taskforcesh/bullmq into feat/add-queue-registry-support
|
||
await delay(100); | ||
|
||
const results = await Queue.getRegistry(client, 0, -1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need some documentation about this new method. Also a warning that if users do only use FlowProducers, discovery won't work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, it should work for flow producers also, this was an oversight from my part.
let errorCode: number | null = null; | ||
try { | ||
const result = await queue.obliterate(); | ||
console.log(result); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these console logs can be removed
* @param start - zero based index from where to start returning jobs. | ||
* @param end - zero based index where to stop returning jobs. | ||
*/ | ||
static getRegistry( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
following my previous comment, what if we pass custom prefix here. So we can get the discovery related to that custom value
Why
To be able to efficiently get all the bullmq queues that are available in a Redis host. This feature makes
queue discovery a mostly O(1) operation instead of O(n).
How
Added a new key with a fixed name: "bullmq:registry", that holds a ZSET with the fully queue qualified names. This queue names will be kept in the registry until the queue is completely obliterated.
Additional Notes (Optional)