feat: Add blocking timeout in polling manager by rashidsp · Pull Request #211 · optimizely/python-sdk

Rashid Siddique (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.

Owais Akbani (oakbani)

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

Owais Akbani (oakbani) · 2019-09-24T04:34:34Z

        Returns:
            ProjectConfig. None if not set.
        """
+        self._condition.acquire()


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

Owais Akbani (oakbani) · 2019-09-24T04:36:33Z

        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.

Owais Akbani (oakbani) · 2019-09-24T04:45:28Z

+        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

Owais Akbani (oakbani) · 2019-09-24T04:46:11Z

+        """
+        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

Owais Akbani (oakbani) · 2019-09-24T04:51:45Z

+            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

Owais Akbani (oakbani) · 2019-09-24T04:53:45Z

+            )
+
+        # 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.

Owais Akbani (oakbani) · 2019-09-24T05:04:44Z

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

Coveralls (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 (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 (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.

Ali Abbas Rizvi (aliabbasrizvi) · 2019-09-26T22:57:11Z

                                                  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?

Ali Abbas Rizvi (aliabbasrizvi) · 2019-09-26T22:58:38Z

+        """
+        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

Ali Abbas Rizvi (aliabbasrizvi) · 2019-09-26T22:59:40Z

+            )
+
+        # 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.

Ali Abbas Rizvi (aliabbasrizvi) · 2019-09-26T23:00:56Z

            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.

Ali Abbas Rizvi (aliabbasrizvi) · 2019-09-26T23:01:52Z

 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

Ali Abbas Rizvi (aliabbasrizvi)

LGTM

Added blocking timeout in polling manager

4296539

Rashid Siddique (rashidsp) requested a review from Owais Akbani (oakbani) September 23, 2019 16:38

Owais Akbani (oakbani) previously requested changes Sep 24, 2019

View reviewed changes

check tests

4bb2cfb

revise

c9e5ef4

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

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

Owais Akbani (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

Ali Abbas Rizvi (aliabbasrizvi) suggested changes Sep 26, 2019

View reviewed changes

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

optimizely-codereview-bot assigned Rashid Siddique (rashidsp) Sep 26, 2019

Ali Abbas Rizvi (aliabbasrizvi) reviewed Sep 26, 2019

View reviewed changes

address comments

a1925bf

Owais Akbani (oakbani) requested a review from Ali Abbas Rizvi (aliabbasrizvi) September 27, 2019 05:06

Owais Akbani (oakbani) and others added 2 commits September 27, 2019 10:09

add unit test

1e4ace4

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

5cbf39d

Owais Akbani (oakbani) mentioned this pull request Oct 9, 2019

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

Merged

Ali Abbas Rizvi (aliabbasrizvi) approved these changes Oct 9, 2019

View reviewed changes

Ali Abbas Rizvi (aliabbasrizvi) merged commit 22437a7 into master Oct 9, 2019

Ali Abbas Rizvi (aliabbasrizvi) deleted the rashid/dfm-blocking-timeout branch October 9, 2019 18:19

Conversation

Rashid Siddique (rashidsp) commented Sep 23, 2019 • edited by oakbani Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

Owais Akbani (oakbani) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Owais Akbani (oakbani) commented Sep 24, 2019

Uh oh!

Coveralls (coveralls) commented Sep 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Coveralls (coveralls) commented Sep 26, 2019

Uh oh!

Coveralls (coveralls) commented Sep 26, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ali Abbas Rizvi (aliabbasrizvi) left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Rashid Siddique (rashidsp) commented Sep 23, 2019 •

edited by oakbani

Loading

Coveralls (coveralls) commented Sep 26, 2019 •

edited

Loading