-
Notifications
You must be signed in to change notification settings - Fork 26
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.
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
FSSDK-8970: Adds support for odp options in config. #366
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a suggestion for grouping all odp configurations. Let me know if it makes sense.
Other than that, all looks good to me.
config.yaml
Outdated
## Disable odp | ||
disableOdp: false | ||
## Max number of items that can be stored in odp segments cache. | ||
segmentsCacheSize: 10000 | ||
## Timeout in seconds after which an item stored in odp segments cache will be deleted. | ||
segmentsCacheTimeout: 600s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about grouping all these and future odp options under "odp". I think "odpCache" is also can be a part of this group. We can also clarify "odpCache" will override these values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about:
- odp_segment_request_timeout
- odp_event_request_timeout
- odp_event_flush_interval
We use these in Python sdk ODP sdk settings, but don't see them exposed in go-sdk. Are we including them in agent config?
In fact we have more in py: https://github.com/optimizely/python-sdk/blob/f000c6e98d9d21ae6ce338e687f5a42f506cbdd3/optimizely/helpers/sdk_settings.py#L57-L65
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have only added those params which are directly available through the main factory constructor. Should we be adding these internal properties as well? @jaeopt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Mat001 request timeout's and flush interval can also be updated in go-sdk but for that we need to supply instances of both the event manager and segment manager to the odp manager.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Mat001 a good call! I think it's valuable to provide those options (segment_request_timeout, event_request_timeout, and probably event_flush_interval as well) to agent customers. @yasirfolio3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made a comment about config options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! A name change suggested. Can we also add timeouts and flush-interval to odp options as @Mat001 suggested?
segmentsCacheSize: 10000 | ||
## Timeout in seconds after which an item stored in odp segments cache will be deleted. | ||
segmentsCacheTimeout: 600s | ||
## This will override all other settings | ||
cache: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! What about "segmentsCache" instead of "cache" to be consistent with "segmentsCacheSize" and "segmentsCacheTimeout"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, will update this in the parent PR for odp-cache, this is branched off of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good with the additional config options.
Summary
disableOdp
,eventsRequestTimeout
,eventsFlushInterval
,segmentsCacheSize
,segmentsRequestTimeout
andsegmentsCacheTimeout
options through config.yamlIssues
Tests