-
Notifications
You must be signed in to change notification settings - Fork 450
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?
Changes from all commits
455aa17
8b49696
9ca6c83
cabd314
f9c7cff
64dec2d
86e84c3
27b367e
c4046bb
c4a9b53
8266464
e94ee5b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -43,6 +43,7 @@ import { | |
} from '../utils'; | ||
import { ChainableCommander } from 'ioredis'; | ||
import { version as packageVersion } from '../version'; | ||
import { BullMQRegistryKey } from '../consts/bullmq-registry-key'; | ||
export type JobData = [JobJsonRaw | number, string?]; | ||
|
||
export class Scripts { | ||
|
@@ -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 commentThe 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 commentThe 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 :/ |
||
this.queue.keys.meta, | ||
this.queue.toKey(''), | ||
]; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
export const BullMQRegistryKey = 'bullmq:registry'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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 commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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.
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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,7 @@ import { v4 } from 'uuid'; | |
import { FlowProducer, Job, Queue, Worker } from '../src/classes'; | ||
import { delay, removeAllQueueData } from '../src/utils'; | ||
import { version as currentPackageVersion } from '../src/version'; | ||
import { BullMQRegistryKey } from '../src/consts/bullmq-registry-key'; | ||
|
||
describe('queues', function () { | ||
const redisHost = process.env.REDIS_HOST || 'localhost'; | ||
|
@@ -715,4 +716,142 @@ describe('queues', function () { | |
await worker.close(); | ||
}); | ||
}); | ||
|
||
describe('Queue registry', () => { | ||
let client: IORedis; | ||
|
||
beforeEach(async function () { | ||
client = new IORedis(); | ||
await client.del(BullMQRegistryKey); | ||
}); | ||
|
||
afterEach(async function () { | ||
await client.quit(); | ||
}); | ||
|
||
it('should add the queue to the registry ZSET when created', async () => { | ||
const queue = new Queue('test-registry', { connection: client, prefix }); | ||
|
||
// Wait a tick for the queue’s init (if needed) | ||
await queue.waitUntilReady(); | ||
|
||
// Check if the queue is in the registry | ||
const result = await client.zscore( | ||
BullMQRegistryKey, | ||
queue.qualifiedName, | ||
); | ||
expect(result).to.not.be.null; | ||
|
||
// Clean up | ||
await queue.obliterate(); | ||
await queue.close(); | ||
}); | ||
|
||
it('should NOT add the queue to the registry if skipMetasUpdate is true', async () => { | ||
const queue = new Queue('test-registry-skip', { | ||
skipMetasUpdate: true, | ||
connection: client, | ||
prefix, | ||
}); | ||
|
||
await queue.waitUntilReady(); | ||
|
||
// If skipMetasUpdate is true, we expect no entry in the registry | ||
const result = await client.zscore( | ||
BullMQRegistryKey, | ||
queue.qualifiedName, | ||
); | ||
expect(result).to.be.null; | ||
|
||
await queue.obliterate(); | ||
await queue.close(); | ||
}); | ||
|
||
it('should remove the queue from registry after obliterating a paused queue', async () => { | ||
const queue = new Queue('test-registry-remove', { | ||
connection: client, | ||
prefix, | ||
}); | ||
await queue.waitUntilReady(); | ||
|
||
// Pause the queue so obliterate can work normally in BullMQ | ||
await queue.pause(); | ||
|
||
// Add it to ensure it’s in the registry | ||
let score = await client.zscore(BullMQRegistryKey, queue.qualifiedName); | ||
expect(score).to.not.be.null; | ||
|
||
// Obliterate | ||
await queue.obliterate(); | ||
|
||
// The queue should now be gone from the registry | ||
score = await client.zscore(BullMQRegistryKey, queue.qualifiedName); | ||
expect(score).to.be.null; | ||
|
||
await queue.close(); | ||
}); | ||
|
||
it('should return paginated queue names via getRegistry()', async () => { | ||
const queueNames = ['registry-a', 'registry-b', 'registry-c']; | ||
const queues: Queue[] = []; | ||
|
||
// Create multiple queues | ||
for (const name of queueNames) { | ||
const queue = new Queue(name, { connection: client, prefix }); | ||
queues.push(queue); | ||
} | ||
|
||
await Promise.all(queues.map(q => q.waitUntilReady())); | ||
|
||
await delay(100); | ||
|
||
const results = await Queue.getRegistry(client, 0, -1); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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. |
||
expect(results).to.have.lengthOf(3); | ||
expect(results).to.include.members( | ||
queueNames.map(name => `${prefix}:${name}`), | ||
); | ||
|
||
// Let’s do partial pagination: only the first 2 | ||
const paginatedResults = await Queue.getRegistry(client, 0, 1); | ||
// Because ZRANGE end index is inclusive, "0,1" means 2 items | ||
expect(paginatedResults).to.have.lengthOf(2); | ||
expect(queueNames.map(name => `${prefix}:${name}`)).to.include.members( | ||
paginatedResults, | ||
); | ||
|
||
// Clean up | ||
for (const queue of queues) { | ||
await queue.obliterate(); | ||
await queue.close(); | ||
} | ||
}); | ||
|
||
// This test should pass however it seems that the paused logic in obliterate is | ||
// not working as expected. | ||
it.skip('should fail to obliterate if queue is not paused', async () => { | ||
const queue = new Queue('test-registry-not-paused', { | ||
connection: client, | ||
prefix, | ||
}); | ||
await queue.waitUntilReady(); | ||
|
||
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 commentThe reason will be displayed to describe this comment to others. Learn more. these console logs can be removed |
||
} catch (err: any) { | ||
console.log(err); | ||
errorCode = err.message.includes('-1') ? -1 : null; | ||
} | ||
|
||
// Verify that it actually was not removed from the registry | ||
const score = await client.zscore(BullMQRegistryKey, queue.qualifiedName); | ||
expect(score).to.not.be.null; | ||
|
||
// If your script / code is returning an error or an error code, check it | ||
expect(errorCode).to.eql(-1); | ||
|
||
await queue.close(); | ||
}); | ||
}); | ||
}); |
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