perf(queue): allow to add more than one base marker #2841
Conversation
roggervalf
force-pushed
the
perf-multiple-markers
branch
from
9f629d8 to
fe654fb
Compare
roggervalf
changed the title
roggervalf
force-pushed
the
perf-multiple-markers
branch
from
8d38fc5 to
d21b980
Compare
|
I see the problem, but I feel the solution is quite complex, maybe there is another way to do it that is simpler, we could for example always add a marker after every work in a bulk even if it is not really needed, that will be a bit less efficient but it is much simpler to implement as you just do it in the multi/exec pipeline... maybe there are even better solutions. |
roggervalf
force-pushed
the
perf-multiple-markers
branch
from
d21b980 to
f1f5f7f
Compare
src/classes/job.ts
Outdated
| @@ -297,8 +299,18 @@ export class Job< | |||
| }); | |||
| } | |||
|
|
|||
| if (opts.markerCount > 1) { | |||
There was a problem hiding this comment.
why not using the number of jobs instead of an option?
Also, If the jobs are standard jobs, no markers are needed at all right? this should only be needed for prioritised or if they are delayed less than the blockTime unless I am missing something.
There was a problem hiding this comment.
I added as an option to not add more matkers than necessary, imagine that you add 1000 jobs and you only have 2 workers. If add 1000 matkers workers Will consume all of them unnecessarily. Only need 2 markes one pr each worker to wake up them. Also is a good point about limiting by the type of job, this should only consider standard and prioritized jobs. Delayed matkers will be added by addDelayedJob script so I can ignore this type in that case. Standard jobs are also using matkers, this is needed for this case as well
There was a problem hiding this comment.
Yes, but this option will never be used correctly anyway, as it is difficult to keep track how many actual workers are there. It would be possible to do a client list and find the workers also, but this is slower and it does not work in GCP as they do not support the CLIENT LIST command (but we could ignore this as it is GCPs fault so to speak)
There was a problem hiding this comment.
But in any case, these markers are only needed for prioritised and delayed jobs faster than the default blockTime right?
There was a problem hiding this comment.
markers are also needed for standard jobs as we are not using brpoplpush anymore
There was a problem hiding this comment.
How fast would a call to "getWorkers" be?
There was a problem hiding this comment.
what about this: a queue instance keeps a "getWorkersCount" cached for lets say 5 minutes, or some configurable value, and based on this count it will apply the number of markers to add?
There was a problem hiding this comment.
the number of workers should not change particularly often anyway
There was a problem hiding this comment.
Something like, in "addBulk", if we hace a fresh workers count we use that one, if it is not fresh we get a new count and use that count.
There was a problem hiding this comment.
I don't think that it's necessary to call getWorkers every time as you said, this number should not change particularly often. Maximum marker count can be handled by users in my opinion or they can avoid of passing markerCount in bulk operations.
src/classes/job.ts
Outdated
| @@ -297,8 +299,18 @@ export class Job< | |||
| }); | |||
| } | |||
|
|
|||
| if (opts.markerCount > 1) { | |||
| const markers: (number | string)[] = []; | |||
| Array.from(Array(opts.markerCount - 1).keys()).forEach(index => { | |||
There was a problem hiding this comment.
You can use "fill" instead: const array = Array(length).fill(0);
roggervalf
force-pushed
the
perf-multiple-markers
branch
from
65e9708 to
63355ca
Compare
|
in favor of #2904 |
when using bulk operations, only one marker is added, in this case only one worker is awake. Other worker instances will be waiting for 5 seconds to pick jobs from waiting