Fix: Removed null check from getDefaultRequestConfigWithTimeout to create separate request configs #498
Conversation
…ding odpEvent upon creating just a copy of UserContext
jaeopt
left a comment
jaeopt
left a comment
There was a problem hiding this comment.
A few clarifications. We can continue discussion in Teams if I miss the points.
| this.odpManager = odpManager; | ||
|
|
||
| if (odpManager != null) { | ||
| if (odpManager != null && odpManager.getEventManager() != null) { |
There was a problem hiding this comment.
Do we need this extra null checking? ODPManager has NonNull segmentManager and eventManager.
| if (odpManager != null) { | ||
| synchronized (odpManager) { | ||
| return odpManager.getSegmentManager().getQualifiedSegments(userId, segmentOptions); | ||
| return odpManager.getSegmentManager() != null ? |
There was a problem hiding this comment.
Same NonNull as EventManager
| } | ||
|
|
||
| private ODPManager(@Nonnull ODPSegmentManager segmentManager, @Nonnull ODPEventManager eventManager, boolean disable) { | ||
| if (!disable) { |
There was a problem hiding this comment.
Java integrates ODP with builder pattern. ODPManager is expected to pass for initialization only when enabled.
I think we may not need this redundant "disable" control here.
enabled:
OptimizelyBuilder().withOdpManager(...).build()
disabled:
OptimizelyBuilder().build()
There was a problem hiding this comment.
That is correct. I added it to keep it consistent but for now I reverted adding odpDisable option in odpManager
| .build(); | ||
|
|
||
| public static RequestConfig getDefaultRequestConfigWithTimeout(int timeoutMillis) { | ||
| if (requestConfigWithTimeout == null) { |
There was a problem hiding this comment.
I understand we try to re-use RequestConfig once created. Curious why we do not want to reuse?
There was a problem hiding this comment.
this function is getting used at multiple places (for setting request config with timeout of SegmentAPIManager as well as for EventAPIManager). If we want to use different timeout for different Request config then this check will not allow that as it will return the first object that was initialized.
mnoman09
changed the title
Summary
Test plan
All test should pass.