feat: fetch timeout made configurable #411

ozayr-zaviar · 2022-11-23T15:14:23Z

Summary

Fetch segment and send opd event timeout made configurable through sdk_settings option

Test plan

All unit tests and FSC should pass.

Ticket

FSSDK-8693

andrewleap-optimizely

Looks good! A couple minor suggestions.

Also can we add a test confirming the new settings get passed in appropriately?

andrewleap-optimizely · 2022-12-01T15:43:37Z

optimizely/odp/odp_event_api_manager.py

+                        api_key: str,
+                        api_host: str,
+                        events: list[OdpEvent],
+                        odp_event_timeout: Optional[int] = None) -> bool:


Suggested change

odp_event_timeout: Optional[int] = None) -> bool:

timeout: Optional[int] = None) -> bool:

Seems like odp_event prefix is unnecessary in this context?

andrewleap-optimizely · 2022-12-01T15:58:28Z

optimizely/odp/odp_segment_manager.py

-    def fetch_qualified_segments(self, user_key: str, user_value: str, options: list[str]
-                                 ) -> Optional[list[str]]:
+    def fetch_qualified_segments(self, user_key: str, user_value: str, options: list[str],
+                                 fetch_segments_timeout: Optional[int]) -> Optional[list[str]]:


Suggested change

fetch_segments_timeout: Optional[int]) -> Optional[list[str]]:

timeout: Optional[int]) -> Optional[list[str]]:

fetch_segments is redundant in this context.

andrewleap-optimizely · 2022-12-01T15:59:25Z

optimizely/odp/odp_segment_api_manager.py

    def fetch_segments(self, api_key: str, api_host: str, user_key: str,
-                       user_value: str, segments_to_check: list[str]) -> Optional[list[str]]:
+                       user_value: str, segments_to_check: list[str],
+                       fetch_segments_timeout: Optional[int] = None) -> Optional[list[str]]:


Suggested change

fetch_segments_timeout: Optional[int] = None) -> Optional[list[str]]:

timeout: Optional[int] = None) -> Optional[list[str]]:

andrewleap-optimizely · 2022-12-01T18:22:15Z

optimizely/odp/odp_segment_api_manager.py

+                       fetch_segments_timeout: Optional[int] = None) -> Optional[list[str]]:
        """
        Fetch segments from ODP GraphQL API.



Missing doc string for the new arg

andrewleap-optimizely · 2022-12-01T18:29:42Z

optimizely/odp/odp_manager.py

            self.segment_manager = OdpSegmentManager(segments_cache, logger=self.logger)

        self.event_manager = self.event_manager or OdpEventManager(self.logger)
+        self.event_manager.odp_event_timeout = odp_event_timeout


How about we pass this in on initialization of the OdpEventManager?

+1 Pass it down all the way to OdpEventApiManager and OdpSegmentApiManager?

jaeopt

A suggestion to pass down timeout while initialization. Other than that, it looks good.

jaeopt · 2022-12-03T00:18:25Z

optimizely/odp/odp_event_api_manager.py

        self.logger = logger or optimizely_logger.NoOpLogger()

-    def send_odp_events(self, api_key: str, api_host: str, events: list[OdpEvent]) -> bool:
+    def send_odp_events(self,


What about passing the timeout value when initializing? We do not need to send timeout for every event. It's not expected we need event-by-event control. Also a custom OdpEventApiManager can have own timeout control.

jaeopt · 2022-12-03T00:20:26Z

optimizely/odp/odp_segment_api_manager.py


    def fetch_segments(self, api_key: str, api_host: str, user_key: str,
-                       user_value: str, segments_to_check: list[str]) -> Optional[list[str]]:
+                       user_value: str, segments_to_check: list[str],


same here. we may keep the timeout constant locally instead of passing timeout for each api call.

jaeopt · 2022-12-03T00:24:13Z

optimizely/odp/odp_manager.py

            self.segment_manager = OdpSegmentManager(segments_cache, logger=self.logger)

        self.event_manager = self.event_manager or OdpEventManager(self.logger)
+        self.event_manager.odp_event_timeout = odp_event_timeout


+1 Pass it down all the way to OdpEventApiManager and OdpSegmentApiManager?

andrewleap-optimizely

Just a couple more small suggestions. Also can we add one more test for Optimizely initialization with OptimizelySdkSettings and the two new parameters?

andrewleap-optimizely · 2022-12-06T20:51:28Z

optimizely/odp/odp_event_api_manager.py

-    def __init__(self, logger: Optional[optimizely_logger.Logger] = None):
+    def __init__(self, logger: Optional[optimizely_logger.Logger] = None, timeout: Optional[int] = None):
        self.logger = logger or optimizely_logger.NoOpLogger()
+        self.timeout = timeout


Suggested change

self.timeout = timeout

self.timeout = timeout or OdpEventApiConfig.REQUEST_TIMEOUT

Might as well just do this once.

andrewleap-optimizely · 2022-12-07T14:32:03Z

optimizely/odp/odp_segment_api_manager.py

-    def __init__(self, logger: Optional[optimizely_logger.Logger] = None):
+    def __init__(self, logger: Optional[optimizely_logger.Logger] = None, timeout: Optional[int] = None):
        self.logger = logger or optimizely_logger.NoOpLogger()
+        self.timeout = timeout


Suggested change

self.timeout = timeout

self.timeout = timeout or OdpSegmentApiConfig.REQUEST_TIMEOUT

jaeopt

LGTM

jaeopt · 2022-12-07T16:45:33Z

optimizely/odp/odp_event_api_manager.py

-    def __init__(self, logger: Optional[optimizely_logger.Logger] = None):
+    def __init__(self, logger: Optional[optimizely_logger.Logger] = None, timeout: Optional[int] = None):
        self.logger = logger or optimizely_logger.NoOpLogger()
+        self.timeout = timeout


andrewleap-optimizely

lgtm

fetch timeout made configurable

d68bec0

optimizely-codereview-bot assigned ozayr-zaviar Nov 23, 2022

ozayr-zaviar added 5 commits November 23, 2022 20:49

fix

7c9a7ca

lint fix

c23b314

fix

da97899

lint fix

6e5c853

odp event timeout added

7ff3e3d

ozayr-zaviar closed this Nov 28, 2022

ozayr-zaviar reopened this Nov 28, 2022

ozayr-zaviar added 8 commits November 28, 2022 19:46

lint fixes

5b1951a

Update odp_event_manager.py

4c8ad4c

fix testcases

8ad739f

fetch odp segment testcases fixed

535ba82

test fixes

db878c7

timeout made optional

75f08ca

Update odp_segment_manager.py

c83c966

lint fix

32618ed

msohailhussain marked this pull request as ready for review December 2, 2022 06:26

msohailhussain requested a review from a team as a code owner December 2, 2022 06:26

msohailhussain requested review from andrewleap-optimizely and jaeopt December 2, 2022 06:26

andrewleap-optimizely suggested changes Dec 2, 2022

View reviewed changes

jaeopt requested changes Dec 3, 2022

View reviewed changes

ozayr-zaviar added 2 commits December 6, 2022 16:42

comments addressed

48bcbe3

Update odp_manager.py

7546a40

ozayr-zaviar requested review from andrewleap-optimizely and jaeopt December 7, 2022 06:11

andrewleap-optimizely suggested changes Dec 7, 2022

View reviewed changes

jaeopt approved these changes Dec 7, 2022

View reviewed changes

comment addressed

1c0778d

msohailhussain requested a review from andrewleap-optimizely December 9, 2022 06:04

andrewleap-optimizely approved these changes Dec 9, 2022

View reviewed changes

msohailhussain closed this Dec 14, 2022

msohailhussain reopened this Dec 14, 2022

msohailhussain merged commit f67a0cc into master Dec 20, 2022

msohailhussain deleted the uzair/fetch-timeout-config branch December 20, 2022 06:05

	odp_event_timeout: Optional[int] = None) -> bool:
	timeout: Optional[int] = None) -> bool:

	fetch_segments_timeout: Optional[int]) -> Optional[list[str]]:
	timeout: Optional[int]) -> Optional[list[str]]:

	fetch_segments_timeout: Optional[int] = None) -> Optional[list[str]]:
	timeout: Optional[int] = None) -> Optional[list[str]]:

	self.timeout = timeout
	self.timeout = timeout or OdpEventApiConfig.REQUEST_TIMEOUT

	self.timeout = timeout
	self.timeout = timeout or OdpSegmentApiConfig.REQUEST_TIMEOUT

feat: fetch timeout made configurable #411

feat: fetch timeout made configurable #411

Uh oh!

Conversation

ozayr-zaviar commented Nov 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Ticket

Uh oh!

andrewleap-optimizely 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!

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

andrewleap-optimizely 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!

jaeopt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewleap-optimizely left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ozayr-zaviar commented Nov 23, 2022 •

edited

Loading