feat(ATS): update OptimizelyManager/OptimizelyClient for ODP integration #444
Conversation
optimizely-codereview-bot
added
🤖 4 hrs
🤖 8 hrs
🤖 12 hrs
🤖 16 hrs
and removed
🤖 4 hrs
🤖 8 hrs
🤖 12 hrs
labels
| @Nullable private OptimizelyStartListener optimizelyStartListener; | ||
|
|
||
| @Nullable private final List<OptimizelyDecideOption> defaultDecideOptions; | ||
| @NonNull private final List<OptimizelyDecideOption> defaultDecideOptions; |
There was a problem hiding this comment.
Why did we change it to Nonnull, because I am still seeing it nullable at other places?
There was a problem hiding this comment.
A good catch. I might be confused. It has been restored back to Nullable.
| @Nullable private DatafileConfig datafileConfig = null; | ||
| @Nullable private List<OptimizelyDecideOption> defaultDecideOptions = null; | ||
|
|
||
| private long odpSegmentCacheSize = 100L; |
There was a problem hiding this comment.
I think instead of long we should use int data type because eventually we are parsing it to int. Same goes for odpSegmentCacheTimeoutInSecs
There was a problem hiding this comment.
I changed all odp numbers to int.
| } | ||
|
|
||
| @Test | ||
| public void testBuildWithODP_defautSegmentFetchTimeout() throws Exception { |
There was a problem hiding this comment.
| public void testBuildWithODP_defautSegmentFetchTimeout() throws Exception { | |
| public void testBuildWithODP_defaultSegmentFetchTimeout() throws Exception { |
| } | ||
|
|
||
| @Test | ||
| public void testBuildWithODP_defaultCache() throws Exception { |
There was a problem hiding this comment.
| public void testBuildWithODP_defaultCache() throws Exception { | |
| public void testBuildWithODP_defaultCacheSize() throws Exception { |
| verify(mockDatafileHandler).stopBackgroundUpdates(any(), any()); | ||
| verify(mockDatafileHandler, never()).startBackgroundUpdates(any(), any(), any(), any()); | ||
| } | ||
|
|
There was a problem hiding this comment.
Update the header of this file.
| companion object { | ||
| // easy way to set the connection timeout | ||
| // configurable connection timeout | ||
| var CONNECTION_TIMEOUT = 10 * 1000 |
There was a problem hiding this comment.
can we add comment here to show its unit, "ms".
| */ | ||
|
|
||
| if (isValid()) { | ||
| if (this.vuid != null) { |
There was a problem hiding this comment.
Are we only going to trigger sendOdpEvent for client_intialized when vuid is given instead of user id or is it for both cases? Also if the vuid will be inserted by java sdk then what will this check ensure?
There was a problem hiding this comment.
A good catch. I remove vuid null checking.
@mnoman09 it's not expected that android-sdk clients will customize the cache (other than size and timeout), so not supported until we see demands. |
|
@mnoman09 all suggestions addressed. Please review again. |
Summary
Extends top level (OptimizelyManager/OptimizelyClient) to provide public APIs and connect to the core ATS features provided by the integrated Java-SDK.
Add new APIs:
ODP Configuration (OptimizelyManager.Builder)
Test plan
Issues