Skip to content

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

Merged
merged 3 commits into from
Mar 24, 2023

Conversation

jaeopt
Copy link
Contributor

@jaeopt jaeopt commented Mar 23, 2023

Summary

Support configurable timeouts for ODP segment and event requests.
Use default timeout of 10 secs if not configured.

Test plan

  • Add unit tests for configured and default timeout values

Issues

@jaeopt jaeopt requested a review from a team as a code owner March 23, 2023 19:29
@jaeopt jaeopt removed their assignment Mar 23, 2023
@jaeopt jaeopt requested a review from opti-jnguyen March 23, 2023 20:24
Copy link
Contributor

@yasirfolio3 yasirfolio3 left a 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);
Copy link
Contributor

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.

Copy link
Contributor Author

@jaeopt jaeopt Mar 23, 2023

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 :)

Copy link
Contributor

@opti-jnguyen opti-jnguyen left a 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.

@opti-jnguyen opti-jnguyen merged commit 70bdda4 into master Mar 24, 2023
@opti-jnguyen opti-jnguyen deleted the jae/8690-config-timeout branch March 24, 2023 19:27
@coveralls
Copy link

Coverage Status

Coverage: 89.615% (+0.03%) from 89.589% when pulling 5a2072c on jae/8690-config-timeout into 790e154 on master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants