feat: Add blocking timeout in polling manager #211

rashidsp · 2019-09-23T16:37:47Z

Summary

This introduces blocking timeout in Python Config Manager. Config Manager will block the main thread until the config is available or the timeout occurs.

https://github.com/optimizely/python-testapp/pull/63 should be merged to pass FSC tests for this PR.

oakbani

Nice pick threading.condition. Please address comments and add unit tests.

oakbani · 2019-09-24T04:34:34Z

optimizely/config_manager.py

@@ -145,6 +146,9 @@ def get_config(self):
        Returns:
            ProjectConfig. None if not set.
        """
+        self._condition.acquire()


We should only be doing this for the first time. See Ruby.

oakbani · 2019-09-24T04:36:33Z

optimizely/config_manager.py

        self.last_modified = None
        self._polling_thread = threading.Thread(target=self._run)
        self._polling_thread.setDaemon(True)
        self._polling_thread.start()
+        self._condition = threading.Condition()


You will have to put this inside static config manager since you use it in it's method.

oakbani · 2019-09-24T04:45:28Z

optimizely/config_manager.py

+        Args:
+          blocking_timeout: Time in seconds to block the config call.
+        """
+        if not blocking_timeout:


nit. Check if it's None. Otherwise, this will be true when blocking_timeout is 0

oakbani · 2019-09-24T04:46:11Z

optimizely/config_manager.py

+        """
+        if not blocking_timeout:
+            blocking_timeout = enums.ConfigManager.DEFAULT_BLOCKING_TIMEOUT
+            self.logger.debug('Set config blocking timeout to default value {}.'.format(blocking_timeout))


nit. See log in Ruby

nit. Setting

oakbani · 2019-09-24T04:51:45Z

optimizely/config_manager.py

+            blocking_timeout = enums.ConfigManager.DEFAULT_BLOCKING_TIMEOUT
+            self.logger.debug('Set config blocking timeout to default value {}.'.format(blocking_timeout))
+
+        if not isinstance(blocking_timeout, (int, float)):


use numbers.Integral instead of int. This will cover long for Python 2 as well

oakbani · 2019-09-24T04:53:45Z

optimizely/config_manager.py

+            )
+
+        # If blocking timeout is less than or equal to 0 then set it to default blocking timeout.
+        if blocking_timeout <= 0:


Why haven't you enforced MIN and MAX limit as in Ruby SDK?

I feel 0 should be allowed as a value if people are not interested in blocking.

oakbani · 2019-09-24T05:04:44Z

@rashidsp Use threading.Event instead. https://docs.python.org/2.7/library/threading.html?highlight=condition%20acquire#event-objects

coveralls · 2019-09-26T10:52:55Z

Coverage decreased (-0.09%) to 97.765% when pulling 5cbf39d on rashid/dfm-blocking-timeout into 3731be4 on master.

coveralls · 2019-09-26T10:52:56Z

Coverage decreased (-0.01%) to 98.282% when pulling 4bb2cfb on rashid/dfm-blocking-timeout into 6794260 on master.

coveralls · 2019-09-26T10:52:56Z

Coverage decreased (-0.01%) to 98.282% when pulling 4bb2cfb on rashid/dfm-blocking-timeout into 6794260 on master.

aliabbasrizvi · 2019-09-26T22:57:11Z

optimizely/config_manager.py

@@ -95,6 +96,7 @@ def __init__(self,
                                                  notification_center=notification_center)
        self._config = None
        self.validate_schema = not skip_json_validation
+        self._configReadyEvent = threading.Event()


Why is this camelCased when (almost) the entirety of this SDK is snake cased?

aliabbasrizvi · 2019-09-26T22:58:38Z

optimizely/config_manager.py

+        """
+        if not blocking_timeout:
+            blocking_timeout = enums.ConfigManager.DEFAULT_BLOCKING_TIMEOUT
+            self.logger.debug('Set config blocking timeout to default value {}.'.format(blocking_timeout))


nit. Setting

aliabbasrizvi · 2019-09-26T22:59:40Z

optimizely/config_manager.py

+            )
+
+        # If blocking timeout is less than or equal to 0 then set it to default blocking timeout.
+        if blocking_timeout <= 0:


I feel 0 should be allowed as a value if people are not interested in blocking.

aliabbasrizvi · 2019-09-26T23:00:56Z

optimizely/config_manager.py

@@ -133,6 +135,7 @@ def _set_config(self, datafile):
            return

        self._config = config
+        self._configReadyEvent.set()


What is the effect of calling set() twice

No effect. It will make the already 'true' internal flag to 'true' again. Once an event has been set, all the threads which wait for the events do not block until .clear() has been called on the event, which makes the internal flag false.

aliabbasrizvi · 2019-09-26T23:01:52Z

optimizely/helpers/enums.py

@@ -38,6 +38,8 @@ class AudienceEvaluationLogs(object):

 class ConfigManager(object):
  DATAFILE_URL_TEMPLATE = 'https://cdn.optimizely.com/datafiles/{sdk_key}.json'
+  # Default time in seconds to block the 'get_config' method call until 'config' instance has been initialized.
+  DEFAULT_BLOCKING_TIMEOUT = 15


Let's make this 10 seconds.

I am doing it now

aliabbasrizvi

LGTM

Added blocking timeout in polling manager

4296539

rashidsp requested a review from oakbani September 23, 2019 16:38

oakbani previously requested changes Sep 24, 2019

View reviewed changes

check tests

4bb2cfb

revise

c9e5ef4

oakbani changed the title ~~Added blocking timeout in polling manager~~ feat: Add blocking timeout in polling manager Sep 26, 2019

oakbani marked this pull request as ready for review September 26, 2019 12:36

oakbani requested a review from a team as a code owner September 26, 2019 12:36

optimizely-codereview-bot added 🤖 20 hrs 🤖 24 hrs and removed 🤖 20 hrs labels Sep 26, 2019

aliabbasrizvi suggested changes Sep 26, 2019

View reviewed changes

optimizely-codereview-bot removed the 🤖 24 hrs label Sep 26, 2019

optimizely-codereview-bot assigned rashidsp Sep 26, 2019

aliabbasrizvi reviewed Sep 26, 2019

View reviewed changes

address comments

a1925bf

oakbani requested a review from aliabbasrizvi September 27, 2019 05:06

oakbani and others added 2 commits September 27, 2019 10:09

add unit test

1e4ace4

Merge branch 'master' into rashid/dfm-blocking-timeout

5cbf39d

oakbani mentioned this pull request Oct 9, 2019

feat(event_processor): add forwarding event processor and integrate with optimizely #205

Merged

aliabbasrizvi approved these changes Oct 9, 2019

View reviewed changes

aliabbasrizvi merged commit 22437a7 into master Oct 9, 2019

aliabbasrizvi deleted the rashid/dfm-blocking-timeout branch October 9, 2019 18:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add blocking timeout in polling manager #211

feat: Add blocking timeout in polling manager #211

rashidsp commented Sep 23, 2019 •

edited by oakbani

Loading

oakbani left a comment

oakbani Sep 24, 2019

oakbani Sep 24, 2019

oakbani Sep 24, 2019

oakbani Sep 24, 2019

aliabbasrizvi Sep 26, 2019

oakbani Sep 24, 2019

oakbani Sep 24, 2019

aliabbasrizvi Sep 26, 2019

oakbani commented Sep 24, 2019

coveralls commented Sep 26, 2019 •

edited

Loading

coveralls commented Sep 26, 2019

coveralls commented Sep 26, 2019

aliabbasrizvi Sep 26, 2019

aliabbasrizvi Sep 26, 2019

aliabbasrizvi Sep 26, 2019

aliabbasrizvi Sep 26, 2019

oakbani Sep 27, 2019

aliabbasrizvi Sep 26, 2019

aliabbasrizvi left a comment

feat: Add blocking timeout in polling manager #211

feat: Add blocking timeout in polling manager #211

Conversation

rashidsp commented Sep 23, 2019 • edited by oakbani Loading

Summary

oakbani left a 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

oakbani commented Sep 24, 2019

coveralls commented Sep 26, 2019 • edited Loading

coveralls commented Sep 26, 2019

coveralls commented Sep 26, 2019

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

aliabbasrizvi left a comment

Choose a reason for hiding this comment

rashidsp commented Sep 23, 2019 •

edited by oakbani

Loading

coveralls commented Sep 26, 2019 •

edited

Loading