Sanitized options #504

manast · 2017-04-21T17:04:26Z

This PR aims to cleanup the constructor of the Queue and make the options more sane. Also support for configuring a lot of settings that were constants before.

manast · 2017-04-21T17:05:09Z

@bradvogel @chuym new PR you may be interested of. This is a small one, but will break backwards compatibility.

bradvogel · 2017-04-21T23:10:46Z

Awesome! Will review shortly

bradvogel · 2017-04-22T21:12:43Z

README.md

-var audioQueue = Queue('audio transcoding', 6379, '127.0.0.1');
-var imageQueue = Queue('image transcoding', 6379, '127.0.0.1');
-var pdfQueue = Queue('pdf transcoding', 6379, '127.0.0.1');
+var videoQueue = Queue('video transcoding', {redis: {port: 6379, host: '127.0.0.1'}});


Might be clearer to use the redis string version here, eg redis://127.0.0.1:6379, since the documentation lists the redisConnectionString as the second option (albeit optional - but some people might not notice that)

bradvogel · 2017-04-22T21:19:11Z

README.md

+var videoQueue = Queue('video transcoding', {redis: {port: 6379, host: '127.0.0.1'}});
+var audioQueue = Queue('audio transcoding');
+var imageQueue = Queue('image transcoding');
+var pdfQueue = Queue('pdf transcoding');


I think it's clearer to use use new in the examples, e.g. new Queue('pdf transcoding');, so it's clearer that an object instance is returned.

bradvogel · 2017-04-22T22:02:59Z

README.md

 var imageQueue = Queue('image transcoding');
-var pdfQueue = Queue('pdf transcoding');
+var pdfQueue = new Queue('pdf transcoding');


for the examples above too, for consistency.

Ok. My idea was to show different ways to instantiate in the examples. I could clarify with a comment instead. I like to be able to instantiate without new, but maybe this is not a best practice anymore.

bradvogel · 2017-04-22T22:06:38Z

README.md

+    createClient?: (type: enum('client', 'subscriber'), redisOpts?: RedisOpts) => redisClient,
+
+    // Advanced settings
+    settings?: QueueSettings {


I'm working on a PR to document these in more detail

bradvogel · 2017-04-22T22:07:36Z

README.md

+      stalledInterval?: number = 5000,
+      maxStalledCount?: number = 1, // The maximum number of times a job can be recovered from the 'stalled' state
+      guardInterval?: number = 5000,
+      retryProcessDelay?: number = 5000


Should include lockRenewTime?

Added this in #507

…frequently asked question in various Github issues).

bradvogel · 2017-04-23T01:22:07Z

Created PR #507 into this PR. Let me know how that looks.

The readme is coming along nicely!

bradvogel · 2017-04-23T04:54:32Z

README.md

+
+    // Advanced settings
+    settings?: QueueSettings {
+      lockDuration?: number = 5000,


What do you think about defaulting the lock to 30 seconds? My thoughts:

A lot of people complain about jobs being double processed currently. So there must be a lot of poorly written job processor code out there that stalls the event loop :). Or folks, like me, forget that we're running our code on tiny instances in the cloud where the CPU is so limited that a tiny bit of JS work will max the CPU. A 30sec timeout would give a bit more buffer. At least until we figure out a generic solution like [Feature] Support for running jobs in child processes #488.

An expired lock (due to event loop stalling) is quite fatal now that we check the lock prior to moving the job to the completed or failed (previously we would still move it if there wasn't a lock). So if a long running job (let's say 2min) stalls the event loop for even just 5sec, it means that the job is basically tombstoned at that point. It might still continue processing, but another worker would have likely picked it up as a stalled job and double-processed it. Or if it doesn't happen to get picked up as stalled, when it finally completes it still won't be moved to completed because it lost the lock at one time.

The tradeoff is that it will take longer for jobs to be considered 'stalled'. So instead waiting max 5sec to find out if a job was stalled, we'd wait 30sec. I think this is generally OK and that most people aren't running jobs that are that time-sensitive. Actual stalled jobs [due to process crashes] should be extremely rare anyways.

What do you think?

I guess it is ok to have a larger value as default, specially now when it can be configured. I also look forward #488, which for long duration jobs should work much better.

bradvogel · 2017-04-23T05:09:40Z

README.md

+    // Advanced settings
+    settings?: QueueSettings {
+      lockDuration?: number = 5000,
+      stalledInterval?: number = 5000,


Per above comment, I think this should be moved to 30sec also. It doesn't do much good to make it a lot less than the lock time. Also note that since this is run once per process, if it was set to 30sec here, across 6+ worker processes it'd get called about every 5sec anyways.

Document options

manast added 5 commits April 21, 2017 14:43

do not start guardian until queue is ready

2fa99d5

fixed #503

03cabe5

3.0.0-alpha.2

e9784d3

updated changelog

9e882c3

fixed #451 and sanitized queue options

5271193

assigned lockExtener timer id to variable

2a215fd

bradvogel reviewed Apr 22, 2017

View reviewed changes

use connection string as example, and new for instantiation

515d6f6

bradvogel reviewed Apr 22, 2017

View reviewed changes

bradvogel added 5 commits April 22, 2017 18:17

Capitalize Redis

80a1507

Use new consistently.

b9c420a

Remove bad link to bull-ui project.

c40f649

Expand "Important Notes" section to discuss how stalled jobs work (a …

fe97c07

…frequently asked question in various Github issues).

Document new advanced settings.

81639b9

bradvogel mentioned this pull request Apr 23, 2017

Document options #507

Merged

bradvogel reviewed Apr 23, 2017

View reviewed changes

manast added 2 commits April 23, 2017 11:09

Merge pull request #507 from mixmaxhq/document-options

d5abe9f

Document options

Merge branch 'v3.0.0' into sanitized-options

616249c

manast merged commit 450f530 into v3.0.0 Apr 23, 2017

bradvogel mentioned this pull request Apr 23, 2017

longer default lock timeout #512

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sanitized options #504

Sanitized options #504

manast commented Apr 21, 2017

manast commented Apr 21, 2017 •

edited

bradvogel commented Apr 21, 2017

bradvogel Apr 22, 2017

bradvogel Apr 22, 2017

bradvogel Apr 22, 2017

manast Apr 23, 2017

bradvogel Apr 22, 2017 •

edited

bradvogel Apr 23, 2017

bradvogel Apr 22, 2017

bradvogel Apr 23, 2017

bradvogel commented Apr 23, 2017 •

edited

bradvogel Apr 23, 2017 •

edited

manast Apr 23, 2017

bradvogel Apr 23, 2017

Sanitized options #504

Sanitized options #504

Conversation

manast commented Apr 21, 2017

manast commented Apr 21, 2017 • edited

bradvogel commented Apr 21, 2017

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

bradvogel Apr 22, 2017 • edited

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

bradvogel commented Apr 23, 2017 • edited

bradvogel Apr 23, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manast commented Apr 21, 2017 •

edited

bradvogel Apr 22, 2017 •

edited

bradvogel commented Apr 23, 2017 •

edited

bradvogel Apr 23, 2017 •

edited