allow batch enqueueing of jobs using Redis Pipelining #1
Conversation
|
@maciejp: On top of yesterday's work, I added some more JavaDocs, created an This is a PR to our own master branch. But I would when you are fine with this open the exact same PR to the original library. |
| @@ -38,7 +42,7 @@ | |||
|
|
|||
| /** | |||
| * Constructor. | |||
| * | |||
There was a problem hiding this comment.
unnecessary change (in a few places below as well)
| * @throws Exception | ||
| * in case something goes wrong | ||
| */ | ||
| protected abstract void doBatchEnqueue(String queue, List<String> msg) throws Exception; |
| @@ -157,13 +195,13 @@ public boolean acquireLock(final String lockName, final String lockHolder, final | |||
| * @throws Exception | |||
| * in case something goes wrong | |||
| */ | |||
| protected abstract boolean doAcquireLock(final String lockName, final String lockHolder, | |||
| protected abstract boolean doAcquireLock(final String lockName, final String lockHolder, | |||
| * @param jobs | ||
| * the jobs to be enqueued | ||
| * @throws IllegalArgumentException | ||
| * if the queue is null or empty or if the list of jobs is null |
There was a problem hiding this comment.
looks to me that passing null as a list of jobs would result in a NPE
| jedis.quit(); | ||
| } | ||
| } | ||
|
|
||
| private static void doWork(final List<Job> jobs, final Map<String, ? extends Class<? extends Runnable>> jobTypes, |
There was a problem hiding this comment.
one small refactoring that we could do is changing the signature of this method & TestUtils.enqueueJobs to accept a single job and then use it wherever the list of jobs has 1 element.
worker creation could also be extracted to a method
There was a problem hiding this comment.
I would not do this right now. Let's keep it as it is.
| @@ -54,6 +54,9 @@ worker.end(true); | |||
| try { workerThread.join(); } catch (Exception e){ e.printStackTrace(); } | |||
| ``` | |||
|
|
|||
| If enqueueing multiple jobs at the same time, there is `client.batchEnqueue(String queue, List<Job> jobs)` which does it | |||
There was a problem hiding this comment.
should we maybe mention how does it do it and why is it more optimal?
There was a problem hiding this comment.
I think this is too detailed for the README. The section is called 'Quickstart'.
jack-kerouac
force-pushed
the
batch-enqueue-using-pipelining
branch
3 times, most recently
from
fb932f3
to
a3bd403
Compare
Using Redis Pipelining is much faster than one-by-one enqueueing since there is a single Round-Trip-Time for the entire batch, instead of one RTT for each job. This improves the performance of enqueueing a large number of jobs considerably. I decided against splitting large batches into multiple chunks (as recommended by https://redis.io/topics/pipelining#redis-pipelining), but mentioned it in the JavaDoc of this method. Thus, it is up to the user of this library to decide whether he wants to consider chunking the batch.
Not once per job in the list. Thus, I refactored the validation methods to be smaller. I decided against inlining `validateArguments()` method since I am not sure whether this is in line with the design principle of the author of this library.
jack-kerouac
force-pushed
the
batch-enqueue-using-pipelining
branch
from
a3bd403
to
02e3af9
Compare
|
Will close this PR in favor of opening one to the upstream repo. |
This change allows enqueueing a list of jobs with higher performance than one-by-one invoking the existing
client.enqueue()method. It therefore uses Redis Pipelining: https://redis.io/topics/pipelining.I decided against supporting splitting the list of jobs into chunks of e.g. 10,000 jobs, which is recommended by Redis ('Important Note' at https://redis.io/topics/pipelining#redis-pipelining). I mentioned this in the JavaDoc and left it to the client of this method to keep the library simple. Some clients might want to ignore this and I wanted to leave it up to the user.
This change also includes a bit of refactoring of the
validatemethods to be able to validate the queue name parameter independently of a job parameter.Closes gresrun#123