From 831ea7b681659819f5bc9a4e62ad2b646d1f81a1 Mon Sep 17 00:00:00 2001
From: Ramya Krishnan <ramya.krishnan@airbnb.com>
Date: Thu, 13 Jun 2019 18:03:27 -0700
Subject: [PATCH] Avoid discovery_jitter sleep on first event

---
 Gemfile.lock                                  |  2 +-
 lib/synapse/service_watcher/zookeeper.rb      | 12 +++--
 lib/synapse/version.rb                        |  2 +-
 .../synapse/service_watcher_zookeeper_spec.rb | 46 +++++++++++++------
 4 files changed, 41 insertions(+), 21 deletions(-)

diff --git a/Gemfile.lock b/Gemfile.lock
index 920d24b9..6d409093 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -1,7 +1,7 @@
 PATH
   remote: .
   specs:
-    synapse (0.16.10)
+    synapse (0.16.11)
       aws-sdk (~> 1.39)
       docker-api (~> 1.7)
       dogstatsd-ruby (~> 3.3.0)
diff --git a/lib/synapse/service_watcher/zookeeper.rb b/lib/synapse/service_watcher/zookeeper.rb
index 3be450d7..57d867c9 100644
--- a/lib/synapse/service_watcher/zookeeper.rb
+++ b/lib/synapse/service_watcher/zookeeper.rb
@@ -280,17 +280,21 @@ def watcher_callback
         #
         # We call watch on every callback, but do not call zk.register => change callback, except the first time.
         #
-        # We sleep. We loose / do not get any events during this time for this service.
+        # We sleep if we have not slept in discovery_jitter seconds
+        # We loose / do not get any events during this time for this service.
         # This helps with throttling discover.
         #
         # We call watch and discover on every callback.
         # We call exists(watch), children(discover) and get(discover) with (:watch=>true)
         # This re-initializes callbacks just before get scan. So we do not miss any updates by sleeping.
         #
-        if @watcher && @discovery['discovery_jitter']
+        if @discovery['discovery_jitter']
           if @discovery['discovery_jitter'].between?(MIN_JITTER, MAX_JITTER)
-            log.info "synapse: sleeping for discovery_jitter=#{@discovery['discovery_jitter']} seconds for service:#{@name}"
-            sleep @discovery['discovery_jitter']
+            if @last_discovery && (Time.now - @last_discovery) < @discovery['discovery_jitter']
+              log.info "synapse: sleeping for discovery_jitter=#{@discovery['discovery_jitter']} seconds for service:#{@name}"
+              sleep @discovery['discovery_jitter']
+            end
+            @last_discovery = Time.now
           else
             log.warn "synapse: invalid discovery_jitter=#{@discovery['discovery_jitter']} for service:#{@name}"
           end
diff --git a/lib/synapse/version.rb b/lib/synapse/version.rb
index a074a49f..12e5f56e 100644
--- a/lib/synapse/version.rb
+++ b/lib/synapse/version.rb
@@ -1,3 +1,3 @@
 module Synapse
-  VERSION = "0.16.10"
+  VERSION = "0.16.11"
 end
diff --git a/spec/lib/synapse/service_watcher_zookeeper_spec.rb b/spec/lib/synapse/service_watcher_zookeeper_spec.rb
index 40eb4761..0f2f9f4c 100644
--- a/spec/lib/synapse/service_watcher_zookeeper_spec.rb
+++ b/spec/lib/synapse/service_watcher_zookeeper_spec.rb
@@ -172,22 +172,38 @@
       expect(subject.ping?).to be false
     end
 
-    it 'throttles discovery if discovery_jitter is set' do
-      discovery['discovery_jitter'] = 25
-      expect(subject).to receive(:watch)
-      expect(subject).to receive(:discover)
-      expect(subject).to receive(:sleep).with(25)
-      subject.instance_variable_set(:@watcher, instance_double(ZK::EventHandlerSubscription))
-      subject.send(:watcher_callback).call
-    end
+    context 'watcher_callback' do
+      let (:time_now) { Time.now }
+      before :each do
+        expect(subject).to receive(:watch)
+        expect(subject).to receive(:discover)
+        subject.instance_variable_set(:@watcher, instance_double(ZK::EventHandlerSubscription))
+        discovery['discovery_jitter'] = 25
+        allow(Time).to receive(:now).and_return(time_now)
+      end
 
-    it 'do not throttle on invalid discovery_jitter' do
-      discovery['discovery_jitter'] = Synapse::ServiceWatcher::ZookeeperWatcher::MAX_JITTER + 1
-      expect(subject).to receive(:watch)
-      expect(subject).to receive(:discover)
-      expect(subject).not_to receive(:sleep)
-      subject.instance_variable_set(:@watcher, instance_double(ZK::EventHandlerSubscription))
-      subject.send(:watcher_callback).call
+      it 'does not sleep if there was no discovery in last discovery_jitter seconds' do
+        subject.instance_variable_set(:@last_discovery, time_now - (discovery['discovery_jitter'] + 1))
+        expect(subject).not_to receive(:sleep)
+        subject.send(:watcher_callback).call
+      end
+
+      it 'does not sleep if @last_discovery is not set - first time' do
+        expect(subject).not_to receive(:sleep)
+        subject.send(:watcher_callback).call
+      end
+
+      it 'sleep if there was a discovery in last discovery_jitter seconds' do
+        subject.instance_variable_set(:@last_discovery, time_now - (discovery['discovery_jitter'] - 1))
+        expect(subject).to receive(:sleep).with(discovery['discovery_jitter'])
+        subject.send(:watcher_callback).call
+      end
+
+      it 'do not throttle on invalid discovery_jitter' do
+        discovery['discovery_jitter'] = Synapse::ServiceWatcher::ZookeeperWatcher::MAX_JITTER + 1
+        expect(subject).not_to receive(:sleep)
+        subject.send(:watcher_callback).call
+      end
     end
 
     context "generator_config_path" do