update: refactor bucketing logic to remove CMAB handling from DecisionService and adjust tests accordingly

FarhanAnjum-opti · FarhanAnjum-opti · commit e459321f2f12 · 2025-11-05T22:41:05.000+06:00
diff --git a/core-api/src/main/java/com/optimizely/ab/bucketing/Bucketer.java b/core-api/src/main/java/com/optimizely/ab/bucketing/Bucketer.java
@@ -128,49 +128,6 @@ private DecisionResponse<Variation> bucketToVariation(@Nonnull ExperimentCore ex
         return new DecisionResponse(null, reasons);
     }
 
-    /**
-     * Determines CMAB traffic allocation for a user based on hashed value from murmurhash3.
-     * This method handles bucketing users into CMAB (Contextual Multi-Armed Bandit) experiments.
-     */
-    @Nonnull
-    private DecisionResponse<String> bucketToEntityForCmab(@Nonnull Experiment experiment,
-                                                           @Nonnull String bucketingId) {
-        DecisionReasons reasons = DefaultDecisionReasons.newInstance();
-
-        // "salt" the bucket id using the experiment id
-        String experimentId = experiment.getId();
-        String experimentKey = experiment.getKey();
-        String combinedBucketId = bucketingId + experimentId;
-
-        // Handle CMAB traffic allocation
-        TrafficAllocation cmabTrafficAllocation = new TrafficAllocation("$", experiment.getCmab().getTrafficAllocation());
-        List<TrafficAllocation> trafficAllocations = java.util.Collections.singletonList(cmabTrafficAllocation);
-
-        String cmabMessage = reasons.addInfo("Using CMAB traffic allocation for experiment \"%s\".", experimentKey);
-        logger.debug(cmabMessage);
-
-        int hashCode = MurmurHash3.murmurhash3_x86_32(combinedBucketId, 0, combinedBucketId.length(), MURMUR_HASH_SEED);
-        int bucketValue = generateBucketValue(hashCode);
-        logger.debug("Assigned bucket {} to user with bucketingId \"{}\" when bucketing to a variation.", bucketValue, bucketingId);
-
-        String bucketedEntityId = bucketToEntity(bucketValue, trafficAllocations);
-        if (bucketedEntityId != null) {
-            if ("$".equals(bucketedEntityId)) {
-                String message = reasons.addInfo("User with bucketingId \"%s\" is bucketed into CMAB for experiment \"%s\".", bucketingId, experimentKey);
-                logger.info(message);
-            } else {
-                // This shouldn't happen in CMAB since we only have "$" entity, but handle gracefully
-                String message = reasons.addInfo("User with bucketingId \"%s\" is bucketed into entity \"%s\" for experiment \"%s\".", bucketingId, bucketedEntityId, experimentKey);
-                logger.info(message);
-            }
-        } else {
-            String message = reasons.addInfo("User with bucketingId \"%s\" is not bucketed into CMAB for experiment \"%s\".", bucketingId, experimentKey);
-            logger.info(message);
-        }
-
-        return new DecisionResponse<>(bucketedEntityId, reasons);
-    }
-
     /**
      * Assign a {@link Variation} of an {@link Experiment} to a user based on hashed value from murmurhash3.
      *
@@ -183,24 +140,6 @@ private DecisionResponse<String> bucketToEntityForCmab(@Nonnull Experiment exper
     public DecisionResponse<Variation> bucket(@Nonnull ExperimentCore experiment,
                                               @Nonnull String bucketingId,
                                               @Nonnull ProjectConfig projectConfig) {
-        
-        return bucket(experiment, bucketingId, projectConfig, false);
-    }
-
-    /**
-     * Assign a {@link Variation} of an {@link Experiment} to a user based on hashed value from murmurhash3.
-     *
-     * @param experiment  The Experiment in which the user is to be bucketed.
-     * @param bucketingId string A customer-assigned value used to create the key for the murmur hash.
-     * @param projectConfig      The current projectConfig
-     * @param useCmab     boolean flag to decide whether to handle cmab experiments.
-     * @return A {@link DecisionResponse} including the {@link Variation} that user is bucketed into (or null) and the decision reasons
-     */
-    @Nonnull
-    public DecisionResponse<Variation> bucket(@Nonnull ExperimentCore experiment,
-                                              @Nonnull String bucketingId,
-                                              @Nonnull ProjectConfig projectConfig,
-                                              @Nonnull boolean useCmab) {
         DecisionReasons reasons = DefaultDecisionReasons.newInstance();
 
         // ---------- Bucket User ----------
@@ -233,26 +172,9 @@ public DecisionResponse<Variation> bucket(@Nonnull ExperimentCore experiment,
             }
         }
 
-        if (useCmab){
-            if (experiment instanceof Experiment) {
-                DecisionResponse<String> decisionResponse = bucketToEntityForCmab((Experiment) experiment, bucketingId);
-                reasons.merge(decisionResponse.getReasons());
-                String entityId = decisionResponse.getResult();
-                if (entityId==null){
-                    return new DecisionResponse<>(null, reasons);
-                }
-                Variation variation = new Variation(entityId, entityId); //return dummy variation for cmab
-                return new DecisionResponse<>(variation, reasons);
-            } else {
-                String message = reasons.addInfo("ExperimentCore instance is not of type Experiment, cannot perform CMAB bucketing.");
-                logger.info(message);
-                return new DecisionResponse<>(null, reasons);
-            }
-        } else {
-            DecisionResponse<Variation> decisionResponse = bucketToVariation(experiment, bucketingId);
-            reasons.merge(decisionResponse.getReasons());
-            return new DecisionResponse<>(decisionResponse.getResult(), reasons);
-        }
+        DecisionResponse<Variation> decisionResponse = bucketToVariation(experiment, bucketingId);
+        reasons.merge(decisionResponse.getReasons());
+        return new DecisionResponse<>(decisionResponse.getResult(), reasons);
     }
 
     //======== Helper methods ========//
@@ -270,4 +192,4 @@ int generateBucketValue(int hashCode) {
         return (int) Math.floor(MAX_TRAFFIC_VALUE * ratio);
     }
 
-}
+}
diff --git a/core-api/src/main/java/com/optimizely/ab/bucketing/DecisionService.java b/core-api/src/main/java/com/optimizely/ab/bucketing/DecisionService.java
@@ -158,7 +158,10 @@ public DecisionResponse<Variation> getVariation(@Nonnull Experiment experiment,
         if (decisionMeetAudience.getResult()) {
             String bucketingId = getBucketingId(user.getUserId(), user.getAttributes());
             String cmabUUID = null;
-            if (decisionPath == DecisionPath.WITH_CMAB && isCmabExperiment(experiment)) {
+            decisionVariation = bucketer.bucket(experiment, bucketingId, projectConfig);
+            if (decisionPath == DecisionPath.WITH_CMAB && isCmabExperiment(experiment) && decisionVariation.getResult() != null) {  
+                // group-allocation and traffic-allocation checking passed for cmab  
+                // we need server decision overruling local bucketing for cmab  
                 DecisionResponse<CmabDecision> cmabDecision = getDecisionForCmabExperiment(projectConfig, experiment, user, bucketingId, options);
                 reasons.merge(cmabDecision.getReasons());
 
@@ -174,7 +177,6 @@ public DecisionResponse<Variation> getVariation(@Nonnull Experiment experiment,
                 }
             } else {
                 // Standard bucketing for non-CMAB experiments
-                decisionVariation = bucketer.bucket(experiment, bucketingId, projectConfig);
                 reasons.merge(decisionVariation.getReasons());
                 variation = decisionVariation.getResult();
             }
@@ -940,23 +942,6 @@ private DecisionResponse<CmabDecision> getDecisionForCmabExperiment(@Nonnull Pro
                                                                         @Nonnull List<OptimizelyDecideOption> options) {
         DecisionReasons reasons = DefaultDecisionReasons.newInstance();
 
-        // Check if user is in CMAB traffic allocation
-        DecisionResponse<Variation> bucketResponse = bucketer.bucket(experiment, bucketingId, projectConfig, true);
-        // DecisionResponse<String> bucketResponse = bucketer.bucketForCmab(experiment, bucketingId, projectConfig);
-        reasons.merge(bucketResponse.getReasons());
-
-        Variation bucketedVariation = bucketResponse.getResult();
-        String bucketedEntityId = bucketedVariation != null ? bucketedVariation.getId() : null;
-
-        if (bucketedEntityId == null) {
-            String message = String.format("User \"%s\" not in CMAB experiment \"%s\" due to traffic allocation.",
-                    userContext.getUserId(), experiment.getKey());
-            logger.info(message);
-            reasons.addInfo(message);
-
-            return new DecisionResponse<>(null, reasons);
-        }
-
         // User is in CMAB allocation, proceed to CMAB decision
         try {
             CmabDecision cmabDecision = cmabService.getDecision(projectConfig, userContext, experiment.getId(), options);
diff --git a/core-api/src/test/java/com/optimizely/ab/bucketing/DecisionServiceTest.java b/core-api/src/test/java/com/optimizely/ab/bucketing/DecisionServiceTest.java
@@ -1535,9 +1535,13 @@ public void getVariationCmabExperimentServiceError() {
             mockCmab // This makes it a CMAB experiment
         );
         
-        Bucketer bucketer = new Bucketer();
+        // Bucketer bucketer = new Bucketer();
+        Bucketer mockBucketer = mock(Bucketer.class);
+        Variation bucketedVariation = new Variation("$", "$");
+        when(mockBucketer.bucket(any(Experiment.class), anyString(), any(ProjectConfig.class)))
+            .thenReturn(DecisionResponse.responseNoReasons(bucketedVariation));
         DecisionService decisionServiceWithMockCmabService = new DecisionService(
-            bucketer,
+            mockBucketer,
             mockErrorHandler, 
             null, 
             mockCmabService
@@ -1599,7 +1603,7 @@ public void getVariationCmabExperimentServiceSuccess() {
         // Mock bucketer to return a variation (user is in CMAB traffic)
         Variation bucketedVariation = new Variation("$", "$");
         Bucketer mockBucketer = mock(Bucketer.class);
-        when(mockBucketer.bucket(any(Experiment.class), anyString(), any(ProjectConfig.class), eq(true)))
+        when(mockBucketer.bucket(any(Experiment.class), anyString(), any(ProjectConfig.class)))
             .thenReturn(DecisionResponse.responseNoReasons(bucketedVariation));
         
         DecisionService decisionServiceWithMockCmabService = new DecisionService(
@@ -1670,7 +1674,7 @@ public void getVariationCmabExperimentUserNotInTrafficAllocation() {
         
         // Mock bucketer to return null for CMAB allocation (user not in CMAB traffic)
         Bucketer mockBucketer = mock(Bucketer.class);
-        when(mockBucketer.bucket(any(Experiment.class), anyString(), any(ProjectConfig.class), eq(true)))
+        when(mockBucketer.bucket(any(Experiment.class), anyString(), any(ProjectConfig.class)))
             .thenReturn(DecisionResponse.nullNoReasons());
         
         DecisionService decisionServiceWithMockCmabService = new DecisionService(
@@ -1697,7 +1701,7 @@ public void getVariationCmabExperimentUserNotInTrafficAllocation() {
         verify(mockCmabService, never()).getDecision(any(), any(), any(), any());
         
         // Verify that bucketer was called for CMAB allocation
-        verify(mockBucketer, times(1)).bucket(any(Experiment.class), anyString(), any(ProjectConfig.class), eq(true));
+        verify(mockBucketer, times(1)).bucket(any(Experiment.class), anyString(), any(ProjectConfig.class));
     }
 
     private Experiment createMockCmabExperiment() {
