Skip to content

Handling update_interval values. #185

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
aliabbasrizvi merged 1 commit into from
Jul 2, 2019
Merged

Handling update_interval values. #185

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 21 additions & 7 deletions optimizely/config_manager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,15 +212,24 @@ def set_update_interval(self, update_interval):
Args:
update_interval: Time in seconds after which to update datafile.
"""
self.update_interval = update_interval or enums.ConfigManager.DEFAULT_UPDATE_INTERVAL
if not update_interval:
update_interval = enums.ConfigManager.DEFAULT_UPDATE_INTERVAL
self.logger.debug('Set config update interval to default value {}.'.format(update_interval))

if not isinstance(update_interval, (int, float)):
raise optimizely_exceptions.InvalidInputException(
'Invalid update_interval "{}" provided.'.format(update_interval)
)

# If polling interval is less than minimum allowed interval then set it to default update interval.
if self.update_interval < enums.ConfigManager.MIN_UPDATE_INTERVAL:
self.logger.debug('Invalid update_interval {} provided. Defaulting to {}'.format(
if update_interval < enums.ConfigManager.MIN_UPDATE_INTERVAL:
self.logger.debug('update_interval value {} too small. Defaulting to {}'.format(
update_interval,
enums.ConfigManager.DEFAULT_UPDATE_INTERVAL)
)
self.update_interval = enums.ConfigManager.DEFAULT_UPDATE_INTERVAL
update_interval = enums.ConfigManager.DEFAULT_UPDATE_INTERVAL

self.update_interval = update_interval

def set_last_modified(self, response_headers):
""" Looks up and sets last modified time based on Last-Modified header in the response.
Expand Down Expand Up @@ -269,9 +278,14 @@ def is_running(self):

def _run(self):
""" Triggered as part of the thread which fetches the datafile and sleeps until next update interval. """
while self.is_running:
self.fetch_datafile()
time.sleep(self.update_interval)
try:
while self.is_running:
self.fetch_datafile()
time.sleep(self.update_interval)
except (OSError, OverflowError) as err:
self.logger.error('Error in time.sleep. '
'Provided update_interval value may be too big. Error: {}'.format(str(err)))
raise
Copy link
Contributor

@mikeproeng37 mikeproeng37 Jul 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we still be raising in this case or just pass it to the error handler? Not sure if raising from another thread might be an issue

Copy link
Contributor Author

@aliabbasrizvi aliabbasrizvi Jul 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if we hit either of these errors we should just raise so that the user sees this and fixes it. We cannot leave it silent.

Copy link
Contributor

@mikeproeng37 mikeproeng37 Jul 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't this crash the application though? The job of the error handler in this case would be raise visibility (if an error handler is provided), otherwise it'll just show up in the logs (again, if a logger is provided)

Copy link
Contributor Author

@aliabbasrizvi aliabbasrizvi Jul 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the value (which is provided the first time) is too high to cause OverflowError or OSError the SDK won't be initialized in the first place.

Copy link
Contributor

@mikeproeng37 mikeproeng37 Jul 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Wouldn't that make this check redundant then?

Copy link
Contributor Author

@aliabbasrizvi aliabbasrizvi Jul 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it may seem that way, but unfortunately time.sleep has different limits. See here: https://github.com/python/cpython/blob/2.7/Modules/timemodule.c#L1030-L1155

We are not imposing any limit at init after our discussion last week. I was in favor on imposing a 1 day limit, but may be it is ok for time.sleep to fail to let people know that they should change it to a reasonable value. That way they can adjust for their platform accordingly.

Copy link
Contributor

@mikeproeng37 mikeproeng37 Jul 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. Thanks for the explanation.

Copy link
Contributor Author

@aliabbasrizvi aliabbasrizvi Jul 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍


def start(self):
""" Start the config manager and the thread to periodically fetch datafile. """
Expand Down
4 changes: 2 additions & 2 deletions optimizely/helpers/validator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ def is_finite_number(value):
greater than absolute limit of 2^53 else False.
"""
if not isinstance(value, (numbers.Integral, float)):
# numbers.Integral instead of int to accomodate long integer in python 2
# numbers.Integral instead of int to accommodate long integer in python 2
return False

if isinstance(value, bool):
Expand All @@ -244,7 +244,7 @@ def are_values_same_type(first_val, second_val):

Args:
first_val: Value to validate.
second_Val: Value to validate.
second_val: Value to validate.

Returns:
Boolean: True if both values belong to same type. Otherwise False.
Expand Down
5 changes: 5 additions & 0 deletions tests/test_config_manager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,11 @@ def test_set_update_interval(self, _):
""" Test set_update_interval with different inputs. """
project_config_manager = config_manager.PollingConfigManager(sdk_key='some_key')

# Assert that if invalid update_interval is set, then exception is raised.
with self.assertRaisesRegexp(optimizely_exceptions.InvalidInputException,
'Invalid update_interval "invalid interval" provided.'):
project_config_manager.set_update_interval('invalid interval')

# Assert that update_interval cannot be set to less than allowed minimum and instead is set to default value.
project_config_manager.set_update_interval(0.42)
self.assertEqual(enums.ConfigManager.DEFAULT_UPDATE_INTERVAL, project_config_manager.update_interval)
Expand Down