feat: Added ODPSegmentManager #484
Conversation
optimizely-codereview-bot
added
🤖 4 hrs
🤖 8 hrs
🤖 12 hrs
and removed
🤖 4 hrs
🤖 8 hrs
labels
| } | ||
|
|
||
| public void setApiKey(String apiKey) { | ||
| this.apiKey = apiKey; |
There was a problem hiding this comment.
thread-safety for all props?
There was a problem hiding this comment.
Made all getters and setters synchronized
| logger.debug("ODP Cache Miss. Making a call to ODP Server"); | ||
|
|
||
| ResponseJsonParser parser = ResponseJsonParserFactory.getParser(); | ||
| String qualifiedSegmentsResponse = apiManager.fetchQualifiedSegments(odpConfig.getApiKey(), odpConfig.getApiHost() + SEGMENT_URL_PATH, userKey.getKeyString(), userValue, odpConfig.getAllSegments()); |
There was a problem hiding this comment.
Can we add filtering for cases when odpConfig.getAllSegments() is empty?
We should not send this request to the ODP server.
There was a problem hiding this comment.
Added a check and a unit test.
| logger.debug("ODP Cache Miss. Making a call to ODP Server"); | ||
|
|
||
| ResponseJsonParser parser = ResponseJsonParserFactory.getParser(); | ||
| String qualifiedSegmentsResponse = apiManager.fetchQualifiedSegments(odpConfig.getApiKey(), odpConfig.getApiHost() + SEGMENT_URL_PATH, userKey.getKeyString(), userValue, odpConfig.getAllSegments()); |
There was a problem hiding this comment.
what if odpConfig is not ready (apiKey and apiHost null)?
There was a problem hiding this comment.
Added a check and a unit test.
|
|
||
| public List<String> getQualifiedSegments(ODPUserKey userKey, String userValue, List<ODPSegmentOption> options) { | ||
| if (!odpConfig.isReady()) { | ||
| logger.warn("ODP Config not ready. apiHost and/or apiKey null. Returning Empty list"); |
There was a problem hiding this comment.
Can we change all error messages to the standard format (see TDD)?
| ODPSegmentManager segmentManager = new ODPSegmentManager(odpConfig, mockApiManager, mockCache); | ||
| List<String> segments = segmentManager.getQualifiedSegments(ODPUserKey.FS_USER_ID, "testId"); | ||
|
|
||
| // Cache lookup called with correct key |
There was a problem hiding this comment.
This and next comments misleading.
There was a problem hiding this comment.
Removed the comments
| public List<String> getQualifiedSegments(ODPUserKey userKey, String userValue, List<ODPSegmentOption> options) { | ||
| if (!odpConfig.isReady()) { | ||
| logger.warn("ODP Config not ready. apiHost and/or apiKey null. Returning Empty list"); | ||
| logger.error("ODP is not enabled."); |
There was a problem hiding this comment.
I mean all segments error should come with "Audience segments fetch failed ()". So in this case, "Audience segments fetch failed (ODP is not enabled)". These common messages will be used for all SDKs.
3 participants
Summary
Added an ODPSegmentManager which provides functionality to fetch segments from ODP server and cache and re-use them.
Test plan
JIRA
OASIS-8383