-
Notifications
You must be signed in to change notification settings - Fork 31
[FSSDK-11170] update: decision service methods for cmab #583
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
[FSSDK-11170] update: decision service methods for cmab #583
Conversation
…e to support CMAB UUID handling
…and include CMAB UUIDs in responses
…ils for CMAB configuration
…ations over CMAB service decisions in DecisionService
…tly verify error state
…izely and DecisionService
…text and related fetcher classes
muzahidul-opti
left a comment
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.
Changes look good to me. Added few comments.
core-api/src/main/java/com/optimizely/ab/optimizelydecision/DecisionResponse.java
Show resolved
Hide resolved
core-api/src/main/java/com/optimizely/ab/bucketing/Bucketer.java
Outdated
Show resolved
Hide resolved
core-api/src/main/java/com/optimizely/ab/bucketing/Bucketer.java
Outdated
Show resolved
Hide resolved
core-api/src/main/java/com/optimizely/ab/optimizelydecision/AsyncDecisionsFetcher.java
Outdated
Show resolved
Hide resolved
…ecision responses
…ent key retrieval
…eation in CMAB client
…hance decision handling
…ckage-private and add copyright notice to DecisionPath
…and OptimizelyUserContext with corresponding tests
…ndency and adjust related tests
…and enhance builder methods for cache configuration
jaeopt
left a comment
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 see it's now more aligned with android-sdk. I added more comments. Let's discuss.
core-api/src/main/java/com/optimizely/ab/bucketing/DecisionService.java
Outdated
Show resolved
Hide resolved
|
|
||
| if (decision != null) { | ||
| decisions.add(new DecisionResponse(decision, reasons)); | ||
| decisions.add(new DecisionResponse(decision, reasons, error, decision.cmabUUID)); |
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.
Do we have any case we need pass cmabUUID separately though it's already in decision?
core-api/src/main/java/com/optimizely/ab/bucketing/DecisionService.java
Outdated
Show resolved
Hide resolved
core-httpclient-impl/src/main/java/com/optimizely/ab/cmab/DefaultCmabClient.java
Outdated
Show resolved
Hide resolved
core-httpclient-impl/src/main/java/com/optimizely/ab/OptimizelyFactory.java
Outdated
Show resolved
Hide resolved
core-httpclient-impl/src/main/java/com/optimizely/ab/OptimizelyFactory.java
Outdated
Show resolved
Hide resolved
core-api/src/main/java/com/optimizely/ab/cmab/service/DefaultCmabService.java
Outdated
Show resolved
Hide resolved
…nService and adjust tests accordingly
…ervice to utilize it
…djust instance creation logic
…on and enhance cache handling logging
jaeopt
left a comment
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.
Bucket cleaning up looks good! A few more changes suggested.
core-api/src/test/java/com/optimizely/ab/OptimizelyUserContextTest.java
Outdated
Show resolved
Hide resolved
core-api/src/main/java/com/optimizely/ab/internal/CacheWithRemove.java
Outdated
Show resolved
Hide resolved
core-api/src/main/java/com/optimizely/ab/cmab/service/DefaultCmabService.java
Outdated
Show resolved
Hide resolved
core-api/src/main/java/com/optimizely/ab/cmab/service/DefaultCmabService.java
Outdated
Show resolved
Hide resolved
core-api/src/main/java/com/optimizely/ab/cmab/service/DefaultCmabService.java
Outdated
Show resolved
Hide resolved
core-api/src/main/java/com/optimizely/ab/cmab/service/DefaultCmabService.java
Outdated
Show resolved
Hide resolved
…ache size and timeout, and remove CacheWithRemove interface
…in OptimizelyFactory
…in newDefaultInstance method of OptimizelyFactory
jaeopt
left a comment
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.
All changes look good to me!
Added a few suggestions - it must be the final round :)
core-httpclient-impl/src/main/java/com/optimizely/ab/OptimizelyFactory.java
Outdated
Show resolved
Hide resolved
core-api/src/main/java/com/optimizely/ab/optimizelydecision/OptimizelyDecisionsCallback.java
Outdated
Show resolved
Hide resolved
core-api/src/main/java/com/optimizely/ab/optimizelydecision/AsyncDecisionFetcher.java
Outdated
Show resolved
Hide resolved
core-api/src/main/java/com/optimizely/ab/cmab/client/RetryConfig.java
Outdated
Show resolved
Hide resolved
core-api/src/main/java/com/optimizely/ab/cmab/client/CmabClientHelper.java
Show resolved
Hide resolved
core-api/src/main/java/com/optimizely/ab/cmab/client/CmabClientConfig.java
Show resolved
Hide resolved
core-api/src/main/java/com/optimizely/ab/bucketing/DecisionService.java
Outdated
Show resolved
Hide resolved
…timizelyDecisionsCallback.java Co-authored-by: Jae Kim <45045038+jaeopt@users.noreply.github.com>
…d add factory method
…dling and backward compatibility
jaeopt
left a comment
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
Summary
Decision Service methods to handle CMAB
Test plan
Added unit tests
Issues
FSSDK-11170
FSSDK-12013