-
Notifications
You must be signed in to change notification settings - Fork 83
feat(ats): [FSSDK-8690] fix configurable ODP request timeouts #807
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
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.
Just a minor suggestion, LGTM otherwise.
const browserOdpManager = new BrowserOdpManager({}); | ||
|
||
// @ts-ignore | ||
expect(browserOdpManager.segmentManager.odpSegmentApiManager.requestHandler.timeout).toBe(10000); |
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.
can we use REQUEST_TIMEOUT_ODP_SEGMENTS_MS
here? same for the other test.
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 agree that comparing against the variable will improve test reliability when adjusting the timeout. In this case, I intentionally check against the number to protect from unintended constant changes :)
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!
Merging master back in to be synced with the jae/8793-odp-flush
changes and will force merge this in afterwards.
Summary
Support configurable timeouts for ODP segment and event requests.
Use default timeout of 10 secs if not configured.
Test plan
Issues