MultiplePriorityQueues: Add ability to POP first message from a list of queues by ofirnk · Pull Request #107 · gresrun/jesque

ofirnk · 2016-03-27T21:00:15Z

This PR adds ability to provide a list of queues
[first_priority, second_priority, third_priority, ...]
and receive the first message available.

This is done via a single Redis command that iterates server-side on the provided list of queues.

Suggestions how to improve this PR are welcome 🎉

coveralls · 2016-03-27T21:29:04Z

Coverage increased (+0.03%) to 80.623% when pulling 7523e41 on ofirnk:ofirnk/priority-queues-list into 5483d4e on gresrun:master.

gresrun · 2016-03-29T04:10:25Z

+import static net.greghaines.jesque.worker.WorkerEvent.WORKER_START;
+import static net.greghaines.jesque.worker.WorkerEvent.WORKER_STOP;
+import static net.greghaines.jesque.worker.WorkerImpl.NextQueueStrategy.DRAIN_WHILE_MESSAGES_EXISTS;
+import static net.greghaines.jesque.worker.WorkerImpl.NextQueueStrategy.RESET_TO_HIGHEST_PRIORITY;


Please keep the import order that the rest of the project uses:

static imports (If more than a few from the same class, import star)

regular imports

right, sorry about that 👍

coveralls · 2016-03-29T08:52:03Z

Coverage increased (+0.4%) to 80.948% when pulling 2f321c4 on ofirnk:ofirnk/priority-queues-list into 5483d4e on gresrun:master.

coveralls · 2016-03-29T08:55:27Z

Coverage increased (+0.4%) to 80.948% when pulling 2f321c4 on ofirnk:ofirnk/priority-queues-list into 5483d4e on gresrun:master.

ofirnk · 2016-03-29T08:56:21Z

On latest changeset I've added setOrderedPriorityQueues(List queues) to Worker interface

coveralls · 2016-03-29T08:56:31Z

Coverage increased (+0.4%) to 80.948% when pulling 2f321c4 on ofirnk:ofirnk/priority-queues-list into 5483d4e on gresrun:master.

shlomisut · 2016-04-21T19:55:07Z

@gresrun Can you have a look here ?

rathboma · 2016-09-29T16:21:21Z

Hey, we're just starting to use Jesque to replace our horrible mess of futures and threads (scala), and saw this pull request. Just curious if this functionality is going to make it in? Looks like exactly what we need.

gresrun · 2016-09-29T17:51:05Z

I'll try to take a look at this tonight.

shlomisut · 2016-09-29T17:54:29Z

@rathboma from our end we can say we've been using this in production for the last few months, and it has been working great.

@gresrun thanks, will be happy to see this as part of the main artifact!

gresrun · 2016-10-02T02:04:44Z

-                        ScriptUtils.readScript("/workerScripts/jesque_lpoplpush.lua")));
+                this.popScriptHash.set(this.jedis.scriptLoad(ScriptUtils.readScript(POP_LUA)));
+                this.lpoplpushScriptHash.set(this.jedis.scriptLoad(ScriptUtils.readScript(LPOPLPUSH_LUA)));
+                this.multiPriorityQueuesScriptHash.set(this.jedis.scriptLoad(ScriptUtils.readScript(LPOPLPUSH_LUA)));


I'm not following this logic. Why have two script hashes for loading the same script, LPOPLPUSH_LUA? Is this second one supposed to load workerScripts/fromMultiplePriorityQueues.lua?

You are absolutely correct! It seems that I've messed with 2 different branches and this PR is based on some old branch in my private repo. This is the latest branch and PR I've worked with: kenshoo#6 .
I'll add a commit of only the diff between the two PRs and will squash upon approval.
Sorry for the mess 🙏

gresrun · 2016-10-02T02:15:59Z

+     * {@inheritDoc}
+     */
+    @Override
+    public void setOrderedPriorityQueues(final List<String> queues) {


Not sure I see the rationale behind making this a List argument. You aren't really using it as a list, just adding all the values into the BlockingDeque which all you need is a Collection for.

The rational is that I join the list of queues to make a String param for the Lua script.
As the script is written, it requires a list of queues to fetch items from.
So the order is important - for priority queue behaviour only - and it's implemented via switching based on behaviour on getNextQueue()

gresrun · 2016-10-02T02:20:52Z

+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.Callable;
+


Same here about the imports.

will change also

gresrun · 2016-10-02T02:21:37Z

+import java.util.Collections;
+
+import static net.greghaines.jesque.utils.JesqueUtils.entry;
+import static net.greghaines.jesque.utils.JesqueUtils.map;


Please keep the import order consistent with the rest of the project.

right - will change

gresrun · 2016-10-02T02:25:04Z

+                return (String) this.jedis.evalsha(this.popScriptHash.get(), 3, key, key(INFLIGHT, this.name, curQueue),
+                        JesqueUtils.createRecurringHashKey(key), Long.toString(System.currentTimeMillis()));
+            case RESET_TO_HIGHEST_PRIORITY:
+                return (String) this.jedis.evalsha(this.multiPriorityQueuesScriptHash.get(), 1, curQueue);


This shouldn't work as written. See my first comment.

Agree 👍 please see my updated PR after fixing the branch-mess

ofirnk

All comments are handled in next commit. Thanks for your review 👍

ofirnk · 2016-10-31T15:30:13Z

-                        ScriptUtils.readScript("/workerScripts/jesque_lpoplpush.lua")));
+                this.popScriptHash.set(this.jedis.scriptLoad(ScriptUtils.readScript(POP_LUA)));
+                this.lpoplpushScriptHash.set(this.jedis.scriptLoad(ScriptUtils.readScript(LPOPLPUSH_LUA)));
+                this.multiPriorityQueuesScriptHash.set(this.jedis.scriptLoad(ScriptUtils.readScript(LPOPLPUSH_LUA)));


You are absolutely correct! It seems that I've messed with 2 different branches and this PR is based on some old branch in my private repo. This is the latest branch and PR I've worked with: kenshoo#6 .
I'll add a commit of only the diff between the two PRs and will squash upon approval.
Sorry for the mess 🙏

ofirnk · 2016-10-31T15:38:01Z

+     * {@inheritDoc}
+     */
+    @Override
+    public void setOrderedPriorityQueues(final List<String> queues) {


The rational is that I join the list of queues to make a String param for the Lua script.
As the script is written, it requires a list of queues to fetch items from.
So the order is important - for priority queue behaviour only - and it's implemented via switching based on behaviour on getNextQueue()

ofirnk · 2016-10-31T15:38:48Z

+                return (String) this.jedis.evalsha(this.popScriptHash.get(), 3, key, key(INFLIGHT, this.name, curQueue),
+                        JesqueUtils.createRecurringHashKey(key), Long.toString(System.currentTimeMillis()));
+            case RESET_TO_HIGHEST_PRIORITY:
+                return (String) this.jedis.evalsha(this.multiPriorityQueuesScriptHash.get(), 1, curQueue);


Agree 👍 please see my updated PR after fixing the branch-mess

ofirnk · 2016-10-31T16:50:30Z

+import java.util.Collections;
+
+import static net.greghaines.jesque.utils.JesqueUtils.entry;
+import static net.greghaines.jesque.utils.JesqueUtils.map;


right - will change

ofirnk · 2016-10-31T16:50:49Z

+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.Callable;
+


will change also

Sync with fork

…of queues

…ing to use WorkerPoolimpl with MultiplePriorityQueues

bp-FLN · 2017-12-27T13:52:34Z

Any reason why this feature has not been added to the WorkerPoolImpl?
Are there any technical issues that prevent this from being done?
I'd like to provide a PR but need to make sure first if it's worth implementing.

ofirnk · 2017-12-27T14:53:00Z

AFAICR we just didn't need it there, and this implementation was enough for our use case

ofirnk force-pushed the ofirnk/priority-queues-list branch from 54ec037 to 7523e41 Compare March 27, 2016 21:24

gresrun reviewed Mar 29, 2016
View reviewed changes

ofirnk force-pushed the ofirnk/priority-queues-list branch 2 times, most recently from 723ca54 to 2f321c4 Compare March 29, 2016 08:51

gresrun self-assigned this Sep 29, 2016

gresrun added the Feature label Sep 29, 2016

gresrun requested changes Oct 2, 2016

View reviewed changes

ofirnk commented Oct 31, 2016

View reviewed changes

Ofir Naor added 4 commits November 1, 2016 14:23

Merge pull request #2 from gresrun/master

8261b7d

Sync with fork

MultiplePriorityQueues: Add ability to POP first message from a list …

c3a88ea

…of queues

MultiplePriorityQueues: Fix comments

e9fb8d7

Syncing with kenshoo/pull/6

f12f23d

ofirnk force-pushed the ofirnk/priority-queues-list branch from 481b5d5 to 20f1105 Compare November 1, 2016 13:33

Make Jesque compile and throw unsupported operation exception for try…

f55e59b

…ing to use WorkerPoolimpl with MultiplePriorityQueues

ofirnk force-pushed the ofirnk/priority-queues-list branch from 20f1105 to f55e59b Compare November 1, 2016 13:36

gresrun approved these changes Nov 18, 2016

View reviewed changes

gresrun merged commit fda4afa into gresrun:master Nov 24, 2016

bp-FLN mentioned this pull request Dec 27, 2017

add nextQueueStrategy feature to WorkerPoolImpl #132

Merged

Conversation

ofirnk commented Mar 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Mar 27, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Mar 29, 2016

Uh oh!

coveralls commented Mar 29, 2016

Uh oh!

ofirnk commented Mar 29, 2016

Uh oh!

coveralls commented Mar 29, 2016

Uh oh!

shlomisut commented Apr 21, 2016

Uh oh!

rathboma commented Sep 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gresrun commented Sep 29, 2016

Uh oh!

shlomisut commented Sep 29, 2016

Uh oh!

gresrun Oct 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gresrun Oct 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gresrun Oct 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ofirnk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bp-FLN commented Dec 27, 2017

Uh oh!

ofirnk commented Dec 27, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ofirnk commented Mar 27, 2016 •

edited

Loading

rathboma commented Sep 29, 2016 •

edited

Loading

gresrun Oct 2, 2016 •

edited

Loading

gresrun Oct 2, 2016 •

edited

Loading

gresrun Oct 2, 2016 •

edited

Loading