Introduce redis recovery listener #6

dvizelman · 2017-09-07T09:07:14Z

No description provided.

nanimrod · 2017-09-07T11:41:45Z

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

+        isDebug = LOG.isDebugEnabled() ? Boolean.TRUE.toString() : Boolean.FALSE.toString();
+        serverName = getServerName();
+        this.listener = listener;
+        redisRestartRecoveryOn = (listener != null);


Looks like we can avoid this and look at the listener instead.

Here I went for best readability (IMO, of course). redisRestartRecoveryOn is calculated one time , but used many time. I don't like checking whether some variable is null, I prefer it to be done only on initialization. I would like to use Optional instead, but optional is recommended only for method return value

nanimrod · 2017-09-07T11:43:39Z

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

+        }
+    }
+
+    private String getRecoveryStatus() {


Might be nicer to return the enum and transform to String only when you actually need it.
It will cause you to write the .name() part only once.

nanimrod · 2017-09-07T11:48:11Z

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

+    private Runnable listener;
+    private String serverName;
+    private String isDebug;
+    private Future<?> recovering = null;


We need a better name here...

Once again agree :-)

nanimrod · 2017-09-07T11:48:27Z

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

@@ -41,28 +48,39 @@
            Executors.newScheduledThreadPool(POOL_SIZE);
    private final AtomicReference<String> watchdogScriptHash = new AtomicReference<>(null);
    private Pool<Jedis> jedisPool;
+    private boolean redisRestartRecoveryOn;
+    private Runnable listener;


Better naming...

You are right, will change it

nanimrod · 2017-09-07T11:49:07Z

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

+    private boolean redisRestartRecoveryOn;
+    private Runnable listener;
+    private String serverName;
+    private String isDebug;


Can we save this one?
If not - can we make it boolean?

We can , but then we will need to convert it to string every time we call lua script. I went for performance , not for readability , but it can be changed easily ...

nanimrod · 2017-09-07T11:53:20Z

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

@@ -124,6 +143,40 @@ private void activateIsAliveService(final String serverName) throws RuntimeExcep
        scheduler.scheduleAtFixedRate(isAlive, INITIAL_DELAY, IS_ALIVE_PERIOD, SECONDS);
    }

+    private void heartbeatIsAlive(final Jedis jedis) {
+        String recoveringStatus = getRecoveryStatus();


I don't quite understand why we're passing this to the script.
When you come back - I'll catch you to discuss this.

Once jesque invokes listener, it needs to know whether the listener is running , finished or failed. This information is saved in recoveringStatus variable

…inishs/fails , remove this update from heart beat process

dvizelman · 2017-09-08T07:22:28Z

@ofirbnd

nanimrod · 2017-09-08T20:34:00Z

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

+            redisRestartRecoveryListener.run();
+            doWorkInPool(this.jedisPool, (PoolUtils.PoolWork<Jedis, Void>) jedis -> {
+                jedis.evalsha(watchdogScriptHash.get(), WATCHDOG_SCRIPT_KEYS_NUMBER, serverName, IS_ALIVE, getCurrTime(), isDebug, RecoveryStatus.FINISHED.name(), String.valueOf(LIGHT_KEEPER_PERIOD));
+                ;


What is this line with only ;?

nanimrod · 2017-09-08T20:34:48Z

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

+            try {
+                doWorkInPool(this.jedisPool, (PoolUtils.PoolWork<Jedis, Void>) jedis -> {
+                    jedis.evalsha(watchdogScriptHash.get(), WATCHDOG_SCRIPT_KEYS_NUMBER, serverName, IS_ALIVE, getCurrTime(), isDebug, RecoveryStatus.FAILED.name(), String.valueOf(LIGHT_KEEPER_PERIOD));
+                    ;


nanimrod · 2017-09-08T20:38:12Z

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

 import java.util.concurrent.Executors;
 import java.util.concurrent.ScheduledExecutorService;
 import java.util.concurrent.atomic.AtomicReference;

 import static java.util.concurrent.TimeUnit.SECONDS;
 import static net.greghaines.jesque.utils.PoolUtils.doWorkInPool;

+enum RecoveryStatus {
+    HEARTBEAT,


This is not a status.

nanimrod · 2017-09-08T20:43:27Z

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

@@ -124,6 +143,17 @@ private void activateIsAliveService(final String serverName) throws RuntimeExcep
        scheduler.scheduleAtFixedRate(isAlive, INITIAL_DELAY, IS_ALIVE_PERIOD, SECONDS);
    }

+    private void heartbeatIsAlive(final Jedis jedis) {
+
+        final String recoveryStatus = redisRestartRecoveryOn ? RecoveryStatus.HEARTBEAT.name() : RecoveryStatus.DISABLED.name();


We aren't sending here the recovery-status but we're sending is the recovery mechanism is enabled or not.
Let's rename accordingly and avoid using the enum which is related to the status.

nanimrod · 2017-09-08T20:47:09Z

src/main/resources/workerScripts/watchdog.lua

+                runRecovery = runRecoveryProcess();
+            end
+        end
+    elseif(recoveringStatus == "FINISHED") then


We should extract this code from this context.
I.e. different job/script.

nanimrod · 2017-09-08T20:47:17Z

src/main/resources/workerScripts/watchdog.lua

+        -- mark recovery as finished
+        debugMessage("Redis restart recovery process has finished on server "..serverName)
+        redis.call('SET', redisRecoveryKey,"READY")
+    elseif (recoveringStatus== "FAILED") then


We should extract this code from this context.
I.e. different job/script.

nanimrod · 2017-09-08T20:48:22Z

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

+        try {
+            redisRestartRecoveryListener.run();
+            doWorkInPool(this.jedisPool, (PoolUtils.PoolWork<Jedis, Void>) jedis -> {
+                jedis.evalsha(watchdogScriptHash.get(), WATCHDOG_SCRIPT_KEYS_NUMBER, serverName, IS_ALIVE, getCurrTime(), isDebug, RecoveryStatus.FINISHED.name(), String.valueOf(LIGHT_KEEPER_PERIOD));


Call a different script/job. Calling the same exact script & job is weird and not helping to the readability.
We're not notifying is_alive here. We're just updating the status to FINISHED.

nanimrod · 2017-09-08T20:48:38Z

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

+            LOG.error("Recovery process has failed ", e);
+            try {
+                doWorkInPool(this.jedisPool, (PoolUtils.PoolWork<Jedis, Void>) jedis -> {
+                    jedis.evalsha(watchdogScriptHash.get(), WATCHDOG_SCRIPT_KEYS_NUMBER, serverName, IS_ALIVE, getCurrTime(), isDebug, RecoveryStatus.FAILED.name(), String.valueOf(LIGHT_KEEPER_PERIOD));


different script/job

…ed and relaunch it if needed

nanimrod · 2017-09-12T14:56:08Z

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

@@ -541,7 +547,7 @@ protected void recoverFromException(final String curQueue, final Exception ex) {
                end(false);
            } else {
                authenticateAndSelectDB();
-                LOG.info("Reconnected to Redis");
+                LOG.info("Reconnected to Redis " + this);


Is this have toString?
How will this print look like?

This way

18:52:24.922 [Worker-6 Jesque-DEVELOPMENT: RUNNING] INFO net.greghaines.jesque.worker.WorkerImpl - Reconnected to Redis resque:worker:dimav-e7470:23990-6:JAVA_DYNAMIC_QUEUES,highPriorityQueue,lowPriorityQueue
18:52:24.983 [Worker-10 Jesque-DEVELOPMENT: RUNNING] INFO net.greghaines.jesque.worker.WorkerImpl - Reconnected to Redis resque:worker:dimav-e7470:23990-10:JAVA_DYNAMIC_QUEUES,highPriorityQueue,lowPriorityQueue

instead of

18:52:24.922 [Worker-6 Jesque-DEVELOPMENT: RUNNING] INFO net.greghaines.jesque.worker.WorkerImpl - Reconnected to Redis
18:52:24.983 [Worker-10 Jesque-DEVELOPMENT: RUNNING] INFO net.greghaines.jesque.worker.WorkerImpl - Reconnected to Redis

Probably you are right , as we have specific worker related information in the thread name :-)

What bothered me is that the message says Redis and than we print the worker. Also believe that the worker is not really relevant so expected the Redis to be printed.

I wanted to improve original message and now I just want to revert my change and leave the message intact.

BTW , worker is relevant ,as every worker contains dedicated jedis client and we are reconnecting this jedis to restarted redis

nanimrod · 2017-09-12T14:56:55Z

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

+
+        if(ex instanceof JedisNoScriptException){
+            try {
+                loadScripts();


getRecoveryStrategy is loading scripts? This is weird and not expected.

Agree, my fault

nanimrod · 2017-09-12T14:58:47Z

src/main/resources/workerScripts/watchdog.lua



 if jobName=='watchdog' then
+    timeToRequeueJobsOnInactiveServer = getMillis(ARGV[5])
+    lightKeeperPeriod = getMillis(ARGV[6])
+
    inspectLightKeeper()
 elseif jobName=='requeueJobs' then
    requeueJobs(serverName)
 elseif jobName=='isAlive' then


We should probably use a verb here :)

I've copied it from Thread.isAlive() method. I don't mind changing the name, but can't find better name

What about reportAlive?

…ker's scripts after redis has been restarted; rename isAlive service to reportAlive

nanimrod · 2017-09-13T06:08:48Z

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

+            return recoveryStrategy;
+        }
+    }
+


Why did we touch the WorkerImpl class as part of this PR? Doesn't look related to what we're trying to achieve...

Without reloading script after redis restart workers will not work. And we are trying to achieve situation when listener will not only be called, but will have a chance to requeue aggregations :-)

OK, cool :)

Introduce redis restart recovery

a141b7e

dvizelman requested a review from nanimrod September 7, 2017 09:07

nanimrod requested a review from ofirbnd September 7, 2017 11:40

nanimrod reviewed Sep 7, 2017

View reviewed changes

Update FINISHED/FAILED recovery statuses immidiately after listener f…

118615d

…inishs/fails , remove this update from heart beat process

nanimrod suggested changes Sep 8, 2017

View reviewed changes

On workers restart check whether redis recovery process was interrupt…

fddf9b9

…ed and relaunch it if needed

dvizelman requested a review from matandavid September 10, 2017 05:25

dvizelman added 2 commits September 11, 2017 18:17

On redis restart watchdog script should be reloaded

8005759

on redis restart reload worker scripts

0767d1d

nanimrod suggested changes Sep 12, 2017

View reviewed changes

nanimrod reviewed Sep 12, 2017

View reviewed changes

Make WorkerImpl use dedicated ExceptionHandler in order to reload Wor…

2e0dc4b

…ker's scripts after redis has been restarted; rename isAlive service to reportAlive

nanimrod reviewed Sep 13, 2017

View reviewed changes

nanimrod approved these changes Sep 13, 2017

View reviewed changes

dvizelman merged commit 6066f01 into master Sep 13, 2017

dvizelman deleted the redis_restart_recovery_listener branch September 13, 2017 06:48

bp-FLN pushed a commit to uberall/jesque that referenced this pull request Dec 21, 2017

Syncing with kenshoo/pull/6

f12f23d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce redis recovery listener #6

Introduce redis recovery listener #6

dvizelman commented Sep 7, 2017

nanimrod Sep 7, 2017

dvizelman Sep 7, 2017

nanimrod Sep 7, 2017

nanimrod Sep 7, 2017

dvizelman Sep 7, 2017

nanimrod Sep 7, 2017

dvizelman Sep 7, 2017

nanimrod Sep 7, 2017

dvizelman Sep 7, 2017

nanimrod Sep 7, 2017 •

edited

Loading

dvizelman Sep 7, 2017

dvizelman commented Sep 8, 2017

nanimrod Sep 8, 2017

nanimrod Sep 8, 2017

nanimrod Sep 8, 2017

nanimrod Sep 8, 2017

nanimrod Sep 8, 2017

nanimrod Sep 8, 2017

nanimrod Sep 8, 2017

nanimrod Sep 8, 2017

nanimrod Sep 12, 2017

dvizelman Sep 12, 2017

nanimrod Sep 12, 2017

dvizelman Sep 12, 2017

dvizelman Sep 12, 2017

nanimrod Sep 12, 2017

dvizelman Sep 12, 2017

nanimrod Sep 12, 2017

dvizelman Sep 12, 2017

nanimrod Sep 12, 2017

dvizelman Sep 12, 2017

nanimrod Sep 13, 2017

dvizelman Sep 13, 2017

nanimrod Sep 13, 2017

Introduce redis recovery listener #6

Introduce redis recovery listener #6

Conversation

dvizelman commented Sep 7, 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

nanimrod Sep 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvizelman commented Sep 8, 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

nanimrod Sep 7, 2017 •

edited

Loading