lua based ack implementation #3

dvizelman · 2017-08-20T07:29:14Z

No description provided.

nanimrod · 2017-08-20T08:54:37Z

src/main/java/net/greghaines/jesque/worker/Watchdog.java

+            try {
+                jedis.evalsha(this.LightKeeperScriptHash.get(), 1, getServerName());
+            } catch (RuntimeException e) {
+                e.printStackTrace();


Please... never swallow an exception unless you have a very good reason to do so (almost never!).

You are right, will fix it. I made quick fix in order to see error message and forgot to re-throw exception

So please commit a fix, so the PR will be cleaner :)

nanimrod · 2017-08-20T08:55:47Z

src/main/java/net/greghaines/jesque/worker/Watchdog.java

+    }
+
+    public void activateLightKeeper() {
+        jedis.evalsha(this.RequeueJobsScriptHash.get(), 1, getServerName());


Got it. Cool!!

nanimrod · 2017-08-20T09:01:48Z

src/main/java/net/greghaines/jesque/worker/Watchdog.java

+    protected final Jedis jedis;
+    private final ScheduledExecutorService scheduler =
+            Executors.newScheduledThreadPool(POOL_SIZE);
+    private final AtomicReference<String> LightKeeperScriptHash = new AtomicReference<>(null);


Why do we need the AtomicReference?

While i can't see real need for AtomicReference in this case, I've followed the way scripts are loaded in WorkerImpl only as precaution and in order to save on consistency.

Since it doesn't really have an affect, it's only confusing the reader.
I suggest to remove it.

nanimrod · 2017-08-20T09:02:58Z

src/main/java/net/greghaines/jesque/worker/WorkerImpl.java

@@ -615,7 +618,8 @@ private void removeInFlight(final String curQueue) {
        if (SHUTDOWN_IMMEDIATE.equals(this.state.get())) {
            lpoplpush(key(INFLIGHT, this.name, curQueue), key(QUEUE, curQueue));
        } else {
-            this.jedis.lpop(key(INFLIGHT, this.name, curQueue));
+            this.jedis.evalsha(this.removeInFlightHash.get(), 2, key(INFLIGHT, this.name, curQueue),key("log"));


remove "this"

Referencing instance variable thru this is the convention in this project, there are few other occurrences of this.jedis in this class. Why should we remove this exact reference that even was not introduced by us?

OK, makes sense :)

nanimrod · 2017-08-20T09:45:20Z

src/main/resources/workerScripts/light_keeper.lua

+end
+
+
+local function inspectLiveKeeper(currentTime)


Why are you getting the currentTime from outside?

Ohh !!! I'll give another look on it. Generally, I was not able to use any global variable unless it was defined before function that is going to use it . And I didn't wanted to call redis.time again and again

nanimrod · 2017-08-20T09:47:41Z

src/main/java/net/greghaines/jesque/worker/WorkerImpl.java

@@ -219,6 +221,7 @@ public void run() {
                this.lpoplpushScriptHash.set(this.jedis.scriptLoad(ScriptUtils.readScript(LPOPLPUSH_LUA)));
                this.multiPriorityQueuesScriptHash
                        .set(this.jedis.scriptLoad(ScriptUtils.readScript(POP_FROM_MULTIPLE_PRIO_QUEUES)));
+                this.removeInFlightHash.set(this.jedis.scriptLoad(ScriptUtils.readScript(REMOVE_INFLIGHT_LUA)));;


Load the script only once.

remove in flight cache is loaded only once ...

nanimrod · 2017-08-20T09:50:00Z

src/main/resources/workerScripts/light_keeper.lua

+
+inspectLiveKeeper(currentTime);
+
+redis.call('SET', isAliveKey,currentTime)


What is that?

Updating is alive key every 2 sec on every server

nanimrod · 2017-08-20T09:50:25Z

src/main/resources/workerScripts/light_keeper.lua

+
+local currentTime = getCurrentTime()
+
+inspectLiveKeeper(currentTime);


When are we scheduling this to run every 5 seconds?

Will be fixed

nanimrod · 2017-08-21T05:37:12Z

src/main/java/net/greghaines/jesque/worker/Watchdog.java

+    protected final Jedis jedis;
+    private final ScheduledExecutorService scheduler =
+            Executors.newScheduledThreadPool(POOL_SIZE);
+    private final AtomicReference<String> LightKeeperScriptHash = new AtomicReference<>(null);


Since it doesn't really have an affect, it's only confusing the reader.
I suggest to remove it.

nanimrod · 2017-08-21T05:38:09Z

src/main/java/net/greghaines/jesque/worker/Watchdog.java

+            try {
+                jedis.evalsha(this.LightKeeperScriptHash.get(), 1, getServerName());
+            } catch (RuntimeException e) {
+                e.printStackTrace();


So please commit a fix, so the PR will be cleaner :)

nanimrod · 2017-08-21T05:39:33Z

src/main/java/net/greghaines/jesque/worker/WorkerImpl.java

@@ -615,7 +618,8 @@ private void removeInFlight(final String curQueue) {
        if (SHUTDOWN_IMMEDIATE.equals(this.state.get())) {
            lpoplpush(key(INFLIGHT, this.name, curQueue), key(QUEUE, curQueue));
        } else {
-            this.jedis.lpop(key(INFLIGHT, this.name, curQueue));
+            this.jedis.evalsha(this.removeInFlightHash.get(), 2, key(INFLIGHT, this.name, curQueue),key("log"));


OK, makes sense :)

nanimrod · 2017-08-21T05:39:44Z

src/main/java/net/greghaines/jesque/worker/WorkerImpl.java

@@ -219,6 +221,7 @@ public void run() {
                this.lpoplpushScriptHash.set(this.jedis.scriptLoad(ScriptUtils.readScript(LPOPLPUSH_LUA)));
                this.multiPriorityQueuesScriptHash
                        .set(this.jedis.scriptLoad(ScriptUtils.readScript(POP_FROM_MULTIPLE_PRIO_QUEUES)));
+                this.removeInFlightHash.set(this.jedis.scriptLoad(ScriptUtils.readScript(REMOVE_INFLIGHT_LUA)));;


nanimrod · 2017-08-21T05:46:13Z

src/main/java/net/greghaines/jesque/worker/Watchdog.java

+        }
+    }
+
+    public void activateLightKeeper() {


move to constructor...

Refactor lua scripts into one script providing all watchdog related functionality

nanimrod · 2017-08-23T06:01:11Z

src/main/java/net/greghaines/jesque/worker/Watchdog.java

+        } catch (RuntimeException e) {
+            LOG.error("Failed to run " + REQUEUE_JOBS + " job " + WATCHDOG_LUA + " script", e);
+        }
+


This method is doing too much and is a bit long... please break it down.

Add recover from exception functionality to watchdog

dvizelman · 2017-08-23T11:00:42Z

@nanimrod I feel like Watchdog needs to expose start method . just creating watchdog and expecting it to run automatically looks very unusual in code :


for (int i = 0; i < NUMBER_OF_WORKER_THREADS; i++) {
            final WorkerImpl worker = new WorkerImpl(
                    config,
                    Arrays.asList(HIGH.name(), LOW.name()),
                    redisJobService.getMapJobFactory(),
                    createJedis(config),
                    NextQueueStrategy.RESET_TO_HIGHEST_PRIORITY);
            final Thread workerThread = new Thread(worker);
            workerThread.start();
            workers.put(worker, workerThread);
        }

        new Watchdog(config, createJedis(config));

dvizelman · 2017-08-23T12:15:56Z

I think the best would be to have start method just like Thread has.

nanimrod · 2017-08-23T13:41:26Z

I don't like a solution that forces the client to run 2 methods when none of them has any meaning without the other.

dvizelman · 2017-08-23T13:52:50Z

What 2 methods are you talking about? 1 method start , that's all . And that's the standard behaviour for Java as you can see from the Thread example.

dvizelman · 2017-08-23T14:34:17Z

If I need to choose between 2 bad solutions: having everything executed in constructor or having static method executing watchdog initialization logic, I will go with the constructor. At least in my tests I could extend watchdog with class having empty constructor and set it instead the original one. Just in case, few links bellow are discussing what's wrong with static methods in java

https://testing.googleblog.com/2008/12/static-methods-are-death-to-testability.html
https://simpleprogrammer.com/2010/01/29/static-methods-will-shock-you/
https://mrtnrbrts.wordpress.com/2012/01/18/static-methods-in-java-are-evil-mostly/

lua based ack implementation

d85fc13

dvizelman requested a review from nanimrod August 20, 2017 07:29

nanimrod reviewed Aug 20, 2017

View reviewed changes

nanimrod reviewed Aug 21, 2017

View reviewed changes

Use separate schedulers for isAlive and requeueInactive jobs.

cb3e738

Refactor lua scripts into one script providing all watchdog related functionality

dvizelman requested a review from mosheeshel August 23, 2017 05:41

nanimrod suggested changes Aug 23, 2017

View reviewed changes

dvizelman added 2 commits August 23, 2017 10:46

Break down watchdog acivate() to smaller functions

fab0091

Add recover from exception functionality to watchdog

Allow watchdog to use 1.8 features

f4fb112

dvizelman merged commit 45f07bf into master Aug 24, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lua based ack implementation #3

lua based ack implementation #3

dvizelman commented Aug 20, 2017

nanimrod Aug 20, 2017

dvizelman Aug 20, 2017

nanimrod Aug 21, 2017

nanimrod Aug 20, 2017

nanimrod Aug 20, 2017

dvizelman Aug 20, 2017

nanimrod Aug 21, 2017

nanimrod Aug 20, 2017

dvizelman Aug 20, 2017

nanimrod Aug 21, 2017

nanimrod Aug 20, 2017

dvizelman Aug 20, 2017

nanimrod Aug 20, 2017

dvizelman Aug 20, 2017

nanimrod Aug 21, 2017

nanimrod Aug 20, 2017

dvizelman Aug 20, 2017

nanimrod Aug 20, 2017

dvizelman Aug 20, 2017

nanimrod Aug 21, 2017

nanimrod Aug 21, 2017

nanimrod Aug 21, 2017

nanimrod Aug 21, 2017

nanimrod Aug 21, 2017

nanimrod Aug 23, 2017

dvizelman Aug 23, 2017

dvizelman commented Aug 23, 2017 •

edited

Loading

dvizelman commented Aug 23, 2017

nanimrod commented Aug 23, 2017

dvizelman commented Aug 23, 2017

dvizelman commented Aug 23, 2017


		inspectLiveKeeper(currentTime);

		redis.call('SET', isAliveKey,currentTime)


		local currentTime = getCurrentTime()

		inspectLiveKeeper(currentTime);

lua based ack implementation #3

lua based ack implementation #3

Conversation

dvizelman commented Aug 20, 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

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

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvizelman commented Aug 23, 2017 • edited Loading

dvizelman commented Aug 23, 2017

nanimrod commented Aug 23, 2017

dvizelman commented Aug 23, 2017

dvizelman commented Aug 23, 2017

dvizelman commented Aug 23, 2017 •

edited

Loading