fix(blocking-api): fixed issue related to default blocking-timeout and blocking API methods. #202

rashidsp · 2019-09-11T14:53:44Z

Summary

Fixed bug when blocking_timeout is set to default.
Block all APIs for the blocking_timeout period or datafile is not loaded successfulsy.
Updated doc string.

coveralls · 2019-09-11T20:24:54Z

Coverage decreased (-0.02%) to 99.432% when pulling 3570990 on rashid/dfm-blocking-timeout into 7ab4650 on master.

msohailhussain · 2019-09-11T20:26:44Z

lib/optimizely/config_manager/static_project_config_manager.rb

@@ -39,5 +38,9 @@ def initialize(datafile, logger, error_handler, skip_json_validation)
        skip_json_validation
      )
    end
+
+    def get_config


I think, this should be defined in the projectconfigmanager as an abstract method. right now i can see config is defined in the projectconfig manager.

msohailhussain

please respond to get_config, rest of the stuff will review after that.

mjc1283 · 2019-09-12T17:50:43Z

lib/optimizely/config_manager/http_project_config_manager.rb

      # Get Project Config.

      # If the background datafile polling thread is running. and config has been initalized,
      # we simply return config.
      # If it is not, we wait and block maximum for @blocking_timeout.
      # If thread is not running, we fetch the datafile and update config.
+      return nil if @closed == true


Why do we have to return nil in this case? Is there a problem with just returning the current @config? Also, why @closed == true instead of @closed?

mjc1283 · 2019-09-12T17:56:01Z

lib/optimizely/config_manager/http_project_config_manager.rb

@@ -243,7 +245,7 @@ def blocking_timeout(blocking_timeout)
          Logger::DEBUG,
          "Blocking timeout is not provided. Defaulting to #{Helpers::Constants::CONFIG_MANAGER['DEFAULT_BLOCKING_TIMEOUT']} seconds."
        )
-        @polling_interval = Helpers::Constants::CONFIG_MANAGER['DEFAULT_BLOCKING_TIMEOUT']
+        @blocking_timeout = Helpers::Constants::CONFIG_MANAGER['DEFAULT_BLOCKING_TIMEOUT']


Please ensure that there is test coverage that would have revealed these places where it should have been @blocking_timeout, but was actually @polling_interval.

msohailhussain · 2019-09-12T18:36:19Z

lib/optimizely/config_manager/http_project_config_manager.rb

@@ -105,13 +105,15 @@ def stop!
      @closed = true
    end

-    def get_config
+    def config


why you are defining config not get_config, unable to understand.

Following ruby's convention.
Please view rubocop's rule https://www.rubydoc.info/gems/rubocop/RuboCop/Cop/Naming/AccessorMethodName

I stumpled upon this, too. Not even your own StaticProjectConfigManager is using get_config which makes it unusable here:

ruby-sdk/lib/optimizely.rb

Lines 85 to 88 in 31e93c7

@config_manager = if config_manager.respond_to?(:get_config)

config_manager

elsif sdk_key

HTTPProjectConfigManager.new(

mjc1283 · 2019-09-17T16:27:30Z

@msohailhussain Could you please review and approve if this looks good to you?

msohailhussain

lgtm. please address minor nits.

msohailhussain · 2019-09-17T19:05:52Z

lib/optimizely/config_manager/http_project_config_manager.rb

      # Get Project Config.

      # If the background datafile polling thread is running. and config has been initalized,
      # we simply return config.
      # If it is not, we wait and block maximum for @blocking_timeout.
      # If thread is not running, we fetch the datafile and update config.
+      return @config if @closed


@mjc1283 I think, better attribute name is stopped, since function name is stop. Do you suggest to rename it?
@rashidsp can you please add a comment here, if closed is true, then config is set to nil.

@msohailhussain I agree that stopped is a better attribute name.

msohailhussain · 2019-09-17T19:08:17Z

README.md

@@ -44,7 +44,7 @@ You can initialize the Optimizely instance in two ways: directly with a datafile

   When the `datafile` is given then it will be used initially before any update.

- 2. Initialize Optimizely by providing a Config Manager that implements a 'get_config' method. You can customize our `HTTPConfigManager` as needed.
+ 2. Initialize Optimizely by providing a Config Manager that implements a 'config' method. You can customize our `HTTPConfigManager` as needed.


Please use config

msohailhussain · 2019-09-17T19:09:29Z

lib/optimizely/helpers/constants.rb

@@ -362,7 +362,7 @@ module Constants

      CONFIG_MANAGER = {
        'DATAFILE_URL_TEMPLATE' => 'https://cdn.optimizely.com/datafiles/%s.json',
-        # Default time in seconds to block the get_config call until config has been initialized.
+        # Default time in seconds to block the config method call until config has been initialized.


config or 'config'

msohailhussain · 2019-09-17T19:11:42Z

lib/optimizely/optimizely_factory.rb

@@ -29,7 +29,7 @@ def self.default_instance(sdk_key, datafile = nil)

    # Returns a new optimizely instance.
    #
-    # @param config_manager - Required ConfigManagerInterface Responds to get_config.
+    # @param config_manager - Required ConfigManagerInterface Responds to config method.


please single quote. am saying config should be more focused as method name.

fix(blocking-timeout): fixed issue related to default blocking-timeout

424847e

rashidsp requested a review from a team as a code owner September 11, 2019 14:53

optimizely-codereview-bot assigned rashidsp Sep 11, 2019

rashidsp requested a review from msohailhussain September 11, 2019 14:53

msohailhussain added the WIP Work in progress label Sep 11, 2019

msohailhussain changed the title ~~fix(blocking-timeout): fixed issue related to default blocking-timeout.~~ fix(blocking-timeout): fixed issue related to default blocking-timeout. - WIP Sep 11, 2019

rashidsp added 2 commits September 12, 2019 00:55

updated get_config

7f249b0

fixes: unit tests

8ff0660

msohailhussain reviewed Sep 11, 2019

View reviewed changes

msohailhussain requested review from mjc1283 and mikeproeng37 September 11, 2019 20:28

msohailhussain removed the WIP Work in progress label Sep 11, 2019

msohailhussain changed the title ~~fix(blocking-timeout): fixed issue related to default blocking-timeout. - WIP~~ fix(blocking-timeout): fixed issue related to default blocking-timeout. Sep 11, 2019

msohailhussain changed the title ~~fix(blocking-timeout): fixed issue related to default blocking-timeout.~~ fix(blocking-api): fixed issue related to default blocking-timeout and blocking API methods. Sep 11, 2019

rashidsp added 2 commits September 12, 2019 18:06

refact: config call

22fa6c7

updates: rubocop rule

63b6061

mjc1283 suggested changes Sep 12, 2019

View reviewed changes

msohailhussain reviewed Sep 12, 2019

View reviewed changes

addressed review

bfcca8f

rashidsp added WIP Work in progress and removed WIP Work in progress labels Sep 13, 2019

mjc1283 approved these changes Sep 16, 2019

View reviewed changes

msohailhussain approved these changes Sep 17, 2019

View reviewed changes

Addressed feedback

ba562a7

rashidsp requested a review from msohailhussain September 18, 2019 11:23

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

3570990

msohailhussain approved these changes Sep 19, 2019

View reviewed changes

mjc1283 closed this Sep 19, 2019

mjc1283 reopened this Sep 19, 2019

mjc1283 merged commit 2d0b8b3 into master Sep 20, 2019

rashidsp deleted the rashid/dfm-blocking-timeout branch September 23, 2019 07:38

	@config_manager = if config_manager.respond_to?(:get_config)
	config_manager
	elsif sdk_key
	HTTPProjectConfigManager.new(

fix(blocking-api): fixed issue related to default blocking-timeout and blocking API methods. #202

fix(blocking-api): fixed issue related to default blocking-timeout and blocking API methods. #202

Uh oh!

Conversation

rashidsp commented Sep 11, 2019 • edited by msohailhussain Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msohailhussain 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!

mjc1283 commented Sep 17, 2019

Uh oh!

msohailhussain 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!

Uh oh!

rashidsp commented Sep 11, 2019 •

edited by msohailhussain

Loading

coveralls commented Sep 11, 2019 •

edited

Loading