Skip to content
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

fix(queue): enabled queues to share childPool instance #2237

Merged

Conversation

jfontaine-lifion
Copy link
Contributor

My team use bull queues to create dynamic queues and noticed that we for every instance of a queue a childPool instance is create. This change allows multiple bull queues to share the same bull queue instance.

@mitdave88
Copy link

@jfontaine-lifion This is awesome. Might save memory. Provided, not all queues process simultaneously.
@manast

Copy link
Member

@manast manast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. This could be useful for other users. Please check my comments.

lib/scripts.js Outdated
return queue.client.extendLock([
queue.toKey(jobId) + ':lock',
queue.keys.stalled,
queue.token,
duration,
jobId,
jobId
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not following the current format standard, we use trailing commas (as the default in "prettier"): https://stackoverflow.com/questions/61370583/trailing-comma-after-last-line-in-object

@@ -2837,11 +2848,14 @@ describe('Queue', () => {
});
});

it('should clean the number of jobs requested even if first jobs timestamp doesn\'t match', async () => {
it("should clean the number of jobs requested even if first jobs timestamp doesn't match", async () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should use single quotes instead of double.

@@ -2121,6 +2121,17 @@ describe('Queue', () => {
queue.on('failed', cb);
queue.on('error', done);
});

it('should share child pool across all different queues created', async () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be great to also test the opposite behavior, i.e. that the pools are different if we are not specifying the isSharedChildPool.

@@ -2853,7 +2867,7 @@ describe('Queue', () => {
expect(len).to.be.eql(2);
});

it('shouldn\'t clean anything if all jobs are in grace period', async () => {
it("shouldn't clean anything if all jobs are in grace period", async () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use single quotes

lib/queue.js Outdated
@@ -215,7 +216,8 @@ const Queue = function Queue(name, url, opts) {
guardInterval: 5000,
retryProcessDelay: 5000,
drainDelay: 5,
backoffStrategies: {}
backoffStrategies: {},
isSharedChildPool: false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use trailing commas

@jfontaine-lifion
Copy link
Contributor Author

Thanks. This could be useful for other users. Please check my comments.

@manast I applied the feedback and added some unit tests

@manast manast merged commit 16fdbe9 into OptimalBits:develop Dec 21, 2021
github-actions bot pushed a commit that referenced this pull request Dec 21, 2021
# [4.2.0](v4.1.4...v4.2.0) (2021-12-21)

### Features

* **queue:** enabled queues to share childPool instance ([#2237](#2237)) ([16fdbe9](16fdbe9))
@manast
Copy link
Member

manast commented Dec 21, 2021

🎉 This PR is included in version 4.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants