Add Reconnect delay #167

KohliDev · 2017-10-20T16:36:29Z

Adding reconnect random time so that every MQTT client connects randomly to the broker in case of disconnection.

gkmccready · 2017-10-20T20:55:17Z

liota/lib/transports/mqtt.py

+            log.info("Clean disconnection")
+        else:
+            wait_time = round(random.uniform(1.0, 10.0), 1)
+            log.info("Unexpected disconnection! Reconnecting in {0} seconds".format(str(wait_time)))


If we're going to log here, we should log the reason for the disconnect. Are some reasons fatal or do they all warrant a retry? Also, you shouldn't need to use .format(str(wait_time)) and {0}, right? "... {0:d}".format(wait_time) should be sufficient.

The RC code is included above which will inform the user about the reason for disconnection as per MQTT. I`ll refactor the log statement.

gkmccready · 2017-10-20T21:03:25Z

liota/lib/transports/mqtt.py

+        if self._disconnect_result_code == 0:
+            log.info("Clean disconnection")
+        else:
+            wait_time = round(random.uniform(1.0, 10.0), 1)


Please use random.randint(1, 10) as you do elsewhere, no need for floating point... and please add a comment explaining why we delay and how the upper bound of 10 seconds was chosen.

I added the delay so that every mqtt client have the different random delay(different gateway) before retrying to connect, thus reducing the load on the broker. I used float trying to make it more random. Will add the comment.

Being a float and rounding to a whole number doesn't make this any more random.

I wasn't rounding to complete whole number it still had once decimal place but agreed not added much random effect.

Python got me. :-) But I still disagree with using floats when they are not necessary.

gkmccready · 2017-10-20T21:16:32Z

liota/lib/transports/mqtt.py

@@ -271,6 +278,7 @@ def connect_soc(self):
        self._paho_client.connect(host=self.url, port=self.port, keepalive=self.keep_alive)

        # Start network loop to handle auto-reconnect
+        self._paho_client.reconnect_delay_set(random.randint(1, 15), random.randint(150, 250))


This is too random for us to understand the behaviour; isn't paho going to slowly move between min and max, anyway, so we should just say 1, 250? Otherwise we need a good comment explaining why we're making min & max random values.

When connection is lost, wait initially min_delay seconds and double this time every attempt. The wait is capped at max_delay as per the documentation in Paho MQTT Library.

I have added random delay so that after the max_delay limit is reached every client retries at the different max_delay time.

You don't think that the various starting times of trying to make the failed connection by each individual client will be random enough? They're not all operating in-sync...

As per QE testing, they figured out that the MQTT broker rejects client connection as all of them try to connect together (due to a large number of DB calls for ACL authorization, it wasn't occurring before ACL was implemented). This issue should be resolved at database front but I'm trying to add some randomness for every client as they try to re-connect.

Again, don't you think there will be enough randomness across clients from when they first try to connect (and fail), and how quickly every subsequent attempt occurs? (I think there is enough randomness there, especially as you back off to almost 5 minutes between retries!) This sounds like you're fixing the QE environment, and not the customer environment. I bet this is a scale test where everything starts very quickly and closely together? I agree we need to support that, but does that apply to a customer environment and should they be penalized with these long back offs?

I agree that's the reason I have asked for completing testing report from QE pointing out if this is the main cause. I'll remove the reconnect_delay_set temporarily and just have a random sleep if the disconnection happens unexpectedly.

pmasrani · 2017-10-23T16:12:48Z

liota-lite/requirements.txt

@@ -1,7 +1,5 @@
 aenum==1.4.5
 linux-metrics==0.1.4
-mock==2.0.0
-paho-mqtt==1.2
+paho-mqtt==1.3.1


I think there is something really wrong with the way liota-lite is designed. There should be a way to have a common file structure to describe the requirements and setup.py, with a flag being passed. This is a developer's nightmare to keep files in sync for liota and liota-lite.

You can raise a separate bug to fix this later, as it is not a concern related to this CL.

I don't believe this is required to be fixed as this approach has already been validated by Open Source team.

Can you add me tothe email chain where this is discussed, or point me to the person in Open Source team? I want to discuss this solution.

pmasrani · 2017-10-23T16:13:39Z

liota/lib/transports/mqtt.py

+            # time to reduce load
+            wait_time = random.randint(1, 5)
+            log.info("Unexpected disconnection! Reconnecting in {0:d} seconds".format(wait_time))
+            time.sleep(wait_time)


As per comments of this function, it can be called by a client disconnection as well. Why should we wait here if it is a client initiated disconnection?

This case is handled in the "if" loop above.

Do you mean if..else at line 82-87? Do you mean a client disconnection always leads to clean disconnection? It is a return code, not how the disconnect was initiated...

pmasrani · 2017-10-23T16:17:32Z

liota/lib/transports/mqtt.py

+        else:
+            # Added a random sleep so that every client tries out reconnection to broker at different
+            # time to reduce load
+            wait_time = random.randint(1, 5)


Why do we have two random functions, one with a wait_time here and another at line 283? Can't we be random enough just with one single function? Or at least have them in the same place?

pmasrani · 2017-10-23T16:18:55Z

liota/lib/transports/mqtt.py

@@ -271,6 +280,7 @@ def connect_soc(self):
        self._paho_client.connect(host=self.url, port=self.port, keepalive=self.keep_alive)

        # Start network loop to handle auto-reconnect
+        self._paho_client.reconnect_delay_set(random.randint(1, 15), random.randint(150, 250))


Why these numbers where chosen? How many clients did we try out with? Are we sure this will succeed for thousands of client?
We should make these parameters user configurable so that QE can test and set it to correct values, instead of some randomly chosen numbers.

pmasrani

Can you make various numbers which are used in the delay functions as configurable in liota.conf? I don't want QE to hit connectivity issues and come back for a code change. They can change the default configuration and retry.

pmasrani · 2017-10-26T08:56:41Z

liota/lib/transports/mqtt.py

+            # time to reduce load
+            wait_time = random.randint(1, 5)
+            log.info("Unexpected disconnection! Reconnecting in {0:d} seconds".format(wait_time))
+            time.sleep(wait_time)


Do you mean if..else at line 82-87? Do you mean a client disconnection always leads to clean disconnection? It is a return code, not how the disconnect was initiated...

pmasrani · 2017-10-26T08:57:51Z

liota-lite/requirements.txt

@@ -1,7 +1,5 @@
 aenum==1.4.5
 linux-metrics==0.1.4
-mock==2.0.0
-paho-mqtt==1.2
+paho-mqtt==1.3.1


Can you add me tothe email chain where this is discussed, or point me to the person in Open Source team? I want to discuss this solution.

KohliDev · 2017-10-31T15:00:04Z

We are putting this PR on hold after discussion with @gkmccready . We need concrete reports and exact reason if the MQTT Broker is rejecting connection due to various clients connecting at the same time because liota mqtt client has enough randomness before retrying the connection and are on different OS. Also, we can`t penalize Liota MQTT client with these long back offs.

Venkat2811 · 2017-10-31T15:47:15Z

Please find Randomized exponential back implementation here: diwanc@a7f229c

KohliDev added 2 commits October 20, 2017 22:00

Add Reconnect delay

fa8ed5c

Remove mock Library

9c9e8e3

KohliDev requested review from GregBollella, pmasrani and winniex1 October 20, 2017 19:49

gkmccready reviewed Oct 20, 2017

View reviewed changes

KohliDev added 2 commits October 21, 2017 17:06

Logging Exceptions

6020d6a

Added comments

a6b473d

pmasrani reviewed Oct 23, 2017

View reviewed changes

KohliDev added 2 commits October 26, 2017 01:47

Incorporating Review comments

90f78e7

Added Comment

891c8f4

pmasrani reviewed Oct 26, 2017

View reviewed changes

KohliDev added 2 commits October 30, 2017 09:50

Merge branch 'master' into reconnect-mqtt

a204255

Merge branch 'master' into reconnect-mqtt

5665b4f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Reconnect delay #167

Add Reconnect delay #167

KohliDev commented Oct 20, 2017

gkmccready Oct 20, 2017

KohliDev Oct 21, 2017

gkmccready Oct 20, 2017

KohliDev Oct 21, 2017

gkmccready Oct 21, 2017

KohliDev Oct 21, 2017

gkmccready Oct 21, 2017

gkmccready Oct 20, 2017

KohliDev Oct 21, 2017

gkmccready Oct 21, 2017

KohliDev Oct 21, 2017 •

edited

Loading

gkmccready Oct 21, 2017

KohliDev Oct 26, 2017

pmasrani Oct 23, 2017

pmasrani Oct 24, 2017

KohliDev Oct 26, 2017 •

edited

Loading

pmasrani Oct 26, 2017

pmasrani Oct 23, 2017

KohliDev Oct 26, 2017

pmasrani Oct 26, 2017

pmasrani Oct 23, 2017

pmasrani Oct 23, 2017

pmasrani left a comment

pmasrani Oct 26, 2017

pmasrani Oct 26, 2017

KohliDev commented Oct 31, 2017 •

edited

Loading

Venkat2811 commented Oct 31, 2017

Add Reconnect delay #167

Are you sure you want to change the base?

Add Reconnect delay #167

Conversation

KohliDev commented Oct 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

KohliDev Oct 21, 2017 • edited Loading

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

KohliDev Oct 26, 2017 • edited Loading

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

pmasrani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KohliDev commented Oct 31, 2017 • edited Loading

Venkat2811 commented Oct 31, 2017

KohliDev Oct 21, 2017 •

edited

Loading

KohliDev Oct 26, 2017 •

edited

Loading

KohliDev commented Oct 31, 2017 •

edited

Loading