[FSSDK-11170] update: decision service methods for cmab #583

FarhanAnjum-opti · 2025-09-24T15:46:52Z

Summary

Decision Service methods to handle CMAB

Test plan

Added unit tests

Issues

FSSDK-11170

…vice

…e to support CMAB UUID handling

…and include CMAB UUIDs in responses

…ils for CMAB configuration

…ations over CMAB service decisions in DecisionService

…rvice failures

…information

…tly verify error state

…izely and DecisionService

…out CMAB logic

…text and related fetcher classes

…serContextTest

… documentation

muzahidul-opti

Changes look good to me. Added few comments.

core-api/src/main/java/com/optimizely/ab/Optimizely.java

muzahidul-opti · 2025-09-25T17:03:12Z

core-api/src/main/java/com/optimizely/ab/optimizelydecision/DecisionResponse.java

+    private String cmabUUID;

-    public DecisionResponse(@Nullable T result, @Nonnull DecisionReasons reasons) {
+    public DecisionResponse(@Nullable T result, @Nonnull DecisionReasons reasons, @Nonnull boolean error, @Nullable String cmabUUID) {


Does it impact the existing user?

No, existing users can still use this class as they were. In that case cmabUUID will be set null and error will be false. Class behaviour will be same.

core-api/src/main/java/com/optimizely/ab/bucketing/Bucketer.java

core-api/src/main/java/com/optimizely/ab/bucketing/FeatureDecision.java

core-api/src/main/java/com/optimizely/ab/optimizelydecision/AsyncDecisionsFetcher.java

…aking logic

…ecision responses

…ent key retrieval

…atureDecision

…eation in CMAB client

…hance decision handling

…lientHelper

core-api/src/main/java/com/optimizely/ab/cmab/client/CmabClientHelper.java

muzahidul-opti

LGTM

jaeopt

A few changes in API definitions. Can you refactor them first?
I need more time to review the cmab logic.

jaeopt · 2025-10-08T17:36:16Z

core-api/src/main/java/com/optimizely/ab/Optimizely.java

            String flagKey = flagsWithoutForcedDecision.get(i).getKey();
+
+            if (error) {
+                OptimizelyDecision optimizelyDecision = OptimizelyDecision.newErrorDecision(flagKey, user, DecisionMessage.CMAB_ERROR.reason(experimentKey));


Do we always report CMAB error for any decision errors? Is this safe?

As far as I understand, we get error from decision service only when cmab fails. So this error flag is only true for cmab errors. @raju-opti can verify.

core-api/src/main/java/com/optimizely/ab/Optimizely.java

jaeopt · 2025-10-13T15:35:18Z

core-api/src/main/java/com/optimizely/ab/Optimizely.java

+                                                                    @Nonnull List<String> keys,
+                                                                    @Nonnull List<OptimizelyDecideOption> options,
+                                                                    boolean ignoreDefaultOptions) {
+        Map<String, OptimizelyDecision> decisionMap = new HashMap<>();


We have full separate copies of decideForKeys and decideForKeysSync
It won't be easy to maintain these identical copies with small changes.
Can we consider refactor decideForKeysSync to reuse decideForKeys (or the other way). We can pass "needAsync=True" param to skip async ops (including CMAB).

Same for decideSync and decideAllSync?

core-api/src/main/java/com/optimizely/ab/OptimizelyUserContext.java

jaeopt · 2025-10-13T15:47:54Z

core-api/src/main/java/com/optimizely/ab/bucketing/DecisionService.java

                                                    @Nullable UserProfileTracker userProfileTracker,
-                                                    @Nullable DecisionReasons reasons) {
+                                                    @Nullable DecisionReasons reasons,
+                                                    @Nonnull boolean useCmab) {


can we change this param useCmab to needAsync (or similar)?
I understand the purpose of this is differentiate sync vs async not specific for cmab.
We can keep more it more generatic for future extension.

We have three cases:

Sync + useCmab (java default)

Async + useCmab (for android)

Sync + witoutCmab (for android)

Using needAsync will be confusing in that case I think.

useCmab is for differentiating flow in sync path only. Not related to async.

jaeopt · 2025-10-13T15:49:44Z

core-api/src/main/java/com/optimizely/ab/bucketing/DecisionService.java

        if (decisionMeetAudience.getResult()) {
            String bucketingId = getBucketingId(user.getUserId(), user.getAttributes());
+            String cmabUUID = null;
+            if (useCmab && isCmabExperiment(experiment)) {


We can consider to bring useCmab (needAsync flag) to decouple from cmab so can use to cover other async ops too.

core-api/src/main/java/com/optimizely/ab/bucketing/Bucketer.java

core-api/src/main/java/com/optimizely/ab/bucketing/DecisionService.java

… backward compatibility for mobile apps

… decisions in OptimizelyUserContext

…rContextTest for consistency

…od for consistency

…ecision handling

jaeopt

A few more suggestions at the top level.

jaeopt · 2025-10-17T00:43:59Z

core-api/src/main/java/com/optimizely/ab/Optimizely.java

                                                          @Nonnull List<OptimizelyDecideOption> options,
-                                                          boolean ignoreDefaultOptions) {
+                                                          boolean ignoreDefaultOptions,
+                                                          boolean useCmab) {


I still like to change this useCmab to more generic name to async. Let's discuss :)

core-api/src/main/java/com/optimizely/ab/Optimizely.java

jaeopt · 2025-10-17T00:59:17Z

core-api/src/main/java/com/optimizely/ab/OptimizelyUserContext.java

+     * @param options A list of options for decision-making.
+     * @return A decision result.
+     */
+    public OptimizelyDecision decideSync(@Nonnull String key,


I think we better remove this and all "sync" functions from OptimizelyUserContext.
These are only for android-sdk, so must be hidden from java-sdk users.
Can we change "private final Optimizely optimizely;" in line 53 to "final Optimizely optimizely;" so android-sdk can directly access "optimizely.decideSync()"?

jaeopt · 2025-10-17T00:59:34Z

core-api/src/main/java/com/optimizely/ab/OptimizelyUserContext.java

+     * @param options A list of options for decision-making.
+     * @return All decision results mapped by flag keys.
+     */
+    public Map<String, OptimizelyDecision> decideForKeysSync(@Nonnull List<String> keys,


Same - we should remove this.

jaeopt · 2025-10-17T00:59:46Z

core-api/src/main/java/com/optimizely/ab/OptimizelyUserContext.java

+     * @param options A list of options for decision-making.
+     * @return All decision results mapped by flag keys.
+     */
+    public Map<String, OptimizelyDecision> decideAllSync(@Nonnull List<OptimizelyDecideOption> options) {


Same - we should remove this too.

jaeopt · 2025-10-17T15:48:54Z

core-api/src/main/java/com/optimizely/ab/Optimizely.java

+            int DEFAULT_MAX_SIZE = 1000;
+            int DEFAULT_CMAB_CACHE_TIMEOUT = 30 * 60 * 1000;
+            DefaultLRUCache<CmabCacheValue> cmabCache = new DefaultLRUCache<>(DEFAULT_MAX_SIZE, DEFAULT_CMAB_CACHE_TIMEOUT);


Suggested change

int DEFAULT_MAX_SIZE = 1000;

int DEFAULT_CMAB_CACHE_TIMEOUT = 30 * 60 * 1000;

DefaultLRUCache<CmabCacheValue> cmabCache = new DefaultLRUCache<>(DEFAULT_MAX_SIZE, DEFAULT_CMAB_CACHE_TIMEOUT);

int DEFAULT_CMAB_CACHE_TIMEOUT_IN_SECS = 30 * 60;

``

I believe DefaultLRUCache accepts timeout in secs not in millisecs. Can we add a test case for DefaultLRUCache to validate this error?

I understand cmabCacheSize and cmabCacheTimeout not configurable for now?

jaeopt · 2025-10-17T15:56:00Z

core-api/src/main/java/com/optimizely/ab/Optimizely.java

+            int DEFAULT_CMAB_CACHE_TIMEOUT = 30 * 60 * 1000;
+            DefaultLRUCache<CmabCacheValue> cmabCache = new DefaultLRUCache<>(DEFAULT_MAX_SIZE, DEFAULT_CMAB_CACHE_TIMEOUT);
+            CmabServiceOptions cmabServiceOptions = new CmabServiceOptions(logger, cmabCache, cmabClient);
+            DefaultCmabService defaultCmabService = new DefaultCmabService(cmabServiceOptions);


cmabClient should not be an "option"?
We can consider moving it to DefaultCmabService(cmabClient, cmabServiceOptions).

Co-authored-by: Jae Kim <[email protected]>

jaeopt · 2025-10-17T16:46:43Z

core-api/src/main/java/com/optimizely/ab/Optimizely.java

            return this;
        }

+        public Builder withCmabClient(CmabClient cmabClient) {


@FarhanAnjum-opti It looks like we need to build withCmabService(), so others like android-sdk can inject full-featured CmabService with client + cacheSize + cacheTTL.
We can provide Builder pattern to CmabService like OdpManager -

CmabServiceBuilder() .withCmabCacheSize() .withCmabCachTimeoutInSecs() .withClient() .build()

…larity and maintainability

Co-authored-by: Jae Kim <[email protected]>

…ckage-private and add copyright notice to DecisionPath

FarhanAnjum-opti added 17 commits September 19, 2025 16:18

update: add CmabService to Optimizely class and builder

26e6393

update: integrate CMAB service into OptimizelyFactory

ad63201

update: change CmabService field to non-nullable in Optimizely class

fbed362

update: add CmabService to DecisionService and its tests

53d754a

update: implement CMAB traffic allocation in Bucketer and DecisionSer…

9905026

…vice

update: enhance DecisionService, FeatureDecision, and DecisionRespons…

78f45bf

…e to support CMAB UUID handling

update: enhance DecisionService and DecisionMessage to handle errors …

9757d49

…and include CMAB UUIDs in responses

update: add validConfigJsonCMAB method to DatafileProjectConfigTestUt…

ecf9199

…ils for CMAB configuration

update: add tests to verify precedence of whitelisted and forced vari…

5e0808f

…ations over CMAB service decisions in DecisionService

update: add test to verify error handling in getVariation for CMAB se…

36d2b4c

…rvice failures

update: modify DecisionResponse to include additional error handling …

5796cb7

…information

update: fix error handling assertion in DecisionServiceTest to correc…

d8b0134

…tly verify error state

update: add tests for CMAB experiment variations in DecisionService

b2f270f

update: implement decision-making methods to skip CMAB logic in Optim…

a4c3f1c

…izely and DecisionService

update: add methods to OptimizelyUserContext for decision-making with…

e4fe788

…out CMAB logic

update: add asynchronous decision-making methods in OptimizelyUserCon…

e75693d

…text and related fetcher classes

update: add decision-making methods without CMAB logic in OptimizelyU…

af210d8

…serContextTest

optimizely-codereview-bot assigned FarhanAnjum-opti Sep 24, 2025

update: remove unused parameter 'useCmab' from DecisionService method…

42053e4

… documentation

muzahidul-opti requested changes Sep 25, 2025

View reviewed changes

FarhanAnjum-opti added 10 commits September 26, 2025 03:00

update: rename methods to use 'Sync' suffix for clarity in decision-m…

9a12d72

…aking logic

update: return cmab error decision whenever found

416bcbd

update: enhance error handling by specifying CMAB error messages in d…

64f378f

…ecision responses

update: improve error handling by checking for null values in experim…

8539166

…ent key retrieval

update: fix CMAB error handling by providing a valid Experiment in Fe…

3cee65c

…atureDecision

update: add Javadoc comments for async decision methods and config cr…

47c65b5

…eation in CMAB client

update: refactor build to use cmabClient instead of default service

fe75a85

update: refactor DefaultCmabClient to utilize CmabClientHelper

b0d5090

update: refactor AsyncDecisionsFetcher to AsyncDecisionFetcher and en…

6db2e88

…hance decision handling

update: add missing copyright notice and license information to CmabC…

6fc6446

…lientHelper

pvcraven requested changes Oct 3, 2025

View reviewed changes

core-api/src/main/java/com/optimizely/ab/cmab/client/CmabClientHelper.java Show resolved Hide resolved

muzahidul-opti approved these changes Oct 8, 2025

View reviewed changes

jaeopt requested changes Oct 13, 2025

View reviewed changes

jaeopt reviewed Oct 13, 2025

View reviewed changes

core-api/src/main/java/com/optimizely/ab/bucketing/Bucketer.java Outdated Show resolved Hide resolved

jaeopt reviewed Oct 13, 2025

View reviewed changes

core-api/src/main/java/com/optimizely/ab/bucketing/DecisionService.java Outdated Show resolved Hide resolved

FarhanAnjum-opti added 5 commits October 15, 2025 18:28

update: enhance CMAB handling in bucketing and decision services, add…

a80c0d3

… backward compatibility for mobile apps

update: add backward compatibility support for Android sync and async…

a9ae805

… decisions in OptimizelyUserContext

update: add empty list parameter to decision methods in OptimizelyUse…

f25f824

…rContextTest for consistency

update: replace null with empty list parameter in async decision meth…

7363a2f

…od for consistency

update: add useCmab parameter to decideForKeys methods for enhanced d…

1c52366

…ecision handling

jaeopt requested changes Oct 17, 2025

View reviewed changes

Update core-api/src/main/java/com/optimizely/ab/Optimizely.java

b2dcf9e

Co-authored-by: Jae Kim <[email protected]>

jaeopt reviewed Oct 17, 2025

View reviewed changes

FarhanAnjum-opti and others added 4 commits October 23, 2025 20:54

update: refactor decision-making logic to use DecisionPath enum for c…

89771bc

…larity and maintainability

Update core-api/src/main/java/com/optimizely/ab/Optimizely.java

73d5673

Co-authored-by: Jae Kim <[email protected]>

Update core-api/src/main/java/com/optimizely/ab/Optimizely.java

690379c

Co-authored-by: Jae Kim <[email protected]>

update: modify OptimizelyUserContext to change optimizely field to pa…

9c8dd8f

…ckage-private and add copyright notice to DecisionPath

Uh oh!

[FSSDK-11170] update: decision service methods for cmab #583

Are you sure you want to change the base?

[FSSDK-11170] update: decision service methods for cmab #583

Conversation

FarhanAnjum-opti commented Sep 24, 2025 • edited by atlassian bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Issues

Uh oh!

muzahidul-opti left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

muzahidul-opti left a comment

Choose a reason for hiding this comment

Uh oh!

jaeopt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FarhanAnjum-opti Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jaeopt Oct 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jaeopt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

FarhanAnjum-opti commented Sep 24, 2025 •

edited by atlassian bot

Loading

FarhanAnjum-opti Oct 15, 2025 •

edited

Loading

jaeopt Oct 13, 2025 •

edited

Loading

jaeopt Oct 17, 2025 •

edited

Loading