Changes persistent bucketer to user experiment record

Author: Josh Deffibaugh

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

@@ -18,7 +18,7 @@
 
 import com.optimizely.ab.annotations.VisibleForTesting;
 import com.optimizely.ab.bucketing.Bucketer;
-import com.optimizely.ab.bucketing.PersistentBucketer;
+import com.optimizely.ab.bucketing.UserExperimentRecord;
 import com.optimizely.ab.config.Attribute;
 import com.optimizely.ab.config.EventType;
 import com.optimizely.ab.config.Experiment;
@@ -94,6 +94,13 @@ private Optimizely(@Nonnull ProjectConfig projectConfig,
         this.errorHandler = errorHandler;
     }
 
+    // Do work here that should be done once per Optimizely lifecycle
+    @VisibleForTesting void initialize() {
+        // Give users of ExperimentRegistry a chance to clean out old state for experiments
+        // that are no longer in the data file.
+        bucketer.cleanUserExperimentRecord();
+    }
+
     //======== activate calls ========//
 
     public @Nullable Variation activate(@Nonnull String experimentKey,
@@ -420,7 +427,7 @@ public static class Builder {
 
         private String datafile;
         private Bucketer bucketer;
-        private PersistentBucketer persistentBucketer;
+        private UserExperimentRecord userExperimentRecord;
         private ErrorHandler errorHandler;
         private EventHandler eventHandler;
         private EventBuilder eventBuilder;
@@ -437,8 +444,8 @@ public Builder withErrorHandler(ErrorHandler errorHandler) {
             return this;
         }
 
-        public Builder withPersistentBucketer(PersistentBucketer persistentBucketer) {
-            this.persistentBucketer = persistentBucketer;
+        public Builder withUserExperimentRecord(UserExperimentRecord userExperimentRecord) {
+            this.userExperimentRecord = userExperimentRecord;
             return this;
         }
 
@@ -467,7 +474,7 @@ public Optimizely build() {
 
             // use the default bucketer and event builder, if no overrides were provided
             if (bucketer == null) {
-                bucketer = new Bucketer(projectConfig, persistentBucketer);
+                bucketer = new Bucketer(projectConfig, userExperimentRecord);
             }
 
             if (eventBuilder == null) {
@@ -478,7 +485,9 @@ public Optimizely build() {
                 errorHandler = new NoOpErrorHandler();
             }
 
-            return new Optimizely(projectConfig, bucketer, eventHandler, eventBuilder, errorHandler);
+            Optimizely optimizely = new Optimizely(projectConfig, bucketer, eventHandler, eventBuilder, errorHandler);
+            optimizely.initialize();
+            return optimizely;
         }
     }
 }

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

@@ -47,7 +47,7 @@ public class Bucketer {
 
     private final ProjectConfig projectConfig;
 
-    @Nullable private final PersistentBucketer persistentBucketer;
+    @Nullable private final UserExperimentRecord userExperimentRecord;
 
     private static final Logger logger = LoggerFactory.getLogger(Bucketer.class);
 
@@ -63,9 +63,9 @@ public Bucketer(ProjectConfig projectConfig) {
         this(projectConfig, null);
     }
 
-    public Bucketer(ProjectConfig projectConfig, @Nullable PersistentBucketer persistentBucketer) {
+    public Bucketer(ProjectConfig projectConfig, @Nullable UserExperimentRecord userExperimentRecord) {
         this.projectConfig = projectConfig;
-        this.persistentBucketer = persistentBucketer;
+        this.userExperimentRecord = userExperimentRecord;
     }
 
     private String bucketToEntity(int bucketValue, List<TrafficAllocation> trafficAllocations) {
@@ -111,12 +111,12 @@ private Variation bucketToVariation(@Nonnull Experiment experiment,
         String combinedBucketId = userId + experiment.getId();
 
         // If a persistent bucketer instance is present then check it for a persisted variation
-        if (persistentBucketer != null) {
-            Variation variation = persistentBucketer.restoreActivation(projectConfig, userId, experiment);
-            if (variation != null) {
-                logger.info("Returning previously activated variation \"{}\" from persistent bucketer.", variation.getKey());
+        if (userExperimentRecord != null) {
+            String variationKey = userExperimentRecord.lookup(userId, experiment.getKey());
+            if (variationKey != null) {
+                logger.info("Returning previously activated variation \"{}\" from user experiment registry.", variationKey);
                 // A variation is stored for this combined bucket id
-                return variation;
+                return projectConfig.getExperimentIdMapping().get(experiment.getId()).getVariationKeyToVariationMap().get(variationKey);
             } else {
                 logger.info("No previously activated variation returned from persistent bucketer.");
             }
@@ -137,8 +137,8 @@ private Variation bucketToVariation(@Nonnull Experiment experiment,
                         experimentKey);
 
             // If a persistent bucketer instance is present give it a variation to store
-            if (persistentBucketer != null) {
-                boolean saved = persistentBucketer.saveActivation(userId, experiment, bucketedVariation);
+            if (userExperimentRecord != null) {
+                boolean saved = userExperimentRecord.save(userId, experiment.getKey(), bucketedVariation.getKey());
                 if (saved) {
                     logger.info("Persisted variation \"{}\" of experiment \"{}\".", bucketedVariation.getKey(), experimentKey);
                 } else {
@@ -213,7 +213,23 @@ int generateBucketValue(int hashCode) {
     }
 
     @Nullable
-    public PersistentBucketer getPersistentBucketer() {
-        return persistentBucketer;
+    public UserExperimentRecord getUserExperimentRecord() {
+        return userExperimentRecord;
+    }
+
+    public void cleanUserExperimentRecord() {
+        if (userExperimentRecord != null) {
+            Map<String,Map<String,String>> records = userExperimentRecord.records();
+            if (records != null) {
+                for (Map.Entry<String,Map<String,String>> record : records.entrySet()) {
+                    for (String experimentKey : record.getValue().keySet()) {
+                        Experiment experiment = projectConfig.getExperimentKeyMapping().get(experimentKey);
+                        if (experiment == null || !experiment.isRunning()) {
+                            userExperimentRecord.remove(record.getKey(), experimentKey);
+                        }
+                    }
+                }
+            }
+        }
     }
 }

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

@@ -0,0 +1,53 @@
+package com.optimizely.ab.bucketing;
+
+import java.util.Map;
+
+/**
+ * Gives implementors a chance to override {@link Bucketer} bucketing on subsequent activations.
+ *
+ * Overriding bucketing for subsequent activations is useful in order to prevent changes to
+ * user experience after changing traffic allocations.  Also, this interface gives users
+ * a hook to keep track of activation history.
+ */
+public interface UserExperimentRecord {
+
+    /**
+     * Called when implementors should save an activation
+     *
+     * @param userId the user id of the activation
+     * @param experimentKey the experiment key of the activation
+     * @param variationKey the variation key of the activation
+     * @return true if saving of the record was successful
+     */
+    boolean save(String userId, String experimentKey, String variationKey);
+
+    /**
+     * Called by the bucketer to check for a record of the previous activation
+     * @param userId the user is id of the next activation
+     * @param experimentKey the experiment id of the next activation
+     * @return the variation key of the next activation, or null if no record exists
+     */
+    String lookup(String userId, String experimentKey);
+
+    /**
+     * Called when user experiment record should be removed
+     *
+     * Records should be removed when an experiment is not running or when an experiment has been
+     * deleted.
+     *
+     * @param userId the user id of the record to remove
+     * @param experimentKey the experiment key of the record to remove
+     * @return true if the record was removed
+     */
+    boolean remove(String userId, String experimentKey);
+
+    /**
+     * Called by bucketer to get a mapping of all stored records
+     *
+     * This mapping is needed so that the bucketer can {@link this#remove(String, String)} outdated
+     * records.
+     * @return a map of userIds to a map of experiment keys to variation keys
+     */
+    Map<String,Map<String,String>> records();
+
+}

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

@@ -16,7 +16,7 @@
  */
 package com.optimizely.ab;
 
-import com.optimizely.ab.bucketing.PersistentBucketer;
+import com.optimizely.ab.bucketing.UserExperimentRecord;
 import com.optimizely.ab.config.ProjectConfigTestUtils;
 import com.optimizely.ab.error.ErrorHandler;
 import com.optimizely.ab.error.NoOpErrorHandler;
@@ -89,11 +89,11 @@ public void withDefaultErrorHandler() throws Exception {
 
     @Test
     public void withPersistentBucketer() throws Exception {
-        PersistentBucketer persistentBucketer = mock(PersistentBucketer.class);
+        UserExperimentRecord userExperimentRecord = mock(UserExperimentRecord.class);
         Optimizely optimizelyClient = Optimizely.builder(validConfigJson(), mockEventHandler)
-                .withPersistentBucketer(persistentBucketer)
+                .withUserExperimentRecord(userExperimentRecord)
                 .build();
 
-        assertThat(optimizelyClient.bucketer.getPersistentBucketer(), is(persistentBucketer));
+        assertThat(optimizelyClient.bucketer.getUserExperimentRecord(), is(userExperimentRecord));
     }
 }

core-api/src/test/java/com/optimizely/ab/OptimizelyBuilderTest.java

@@ -138,6 +138,26 @@ public void activateEndToEnd() throws Exception {
         verify(mockEventHandler).dispatchEvent("test_url", testParams);
     }
 
+    /**
+     * Verify that bucketer initialization happens after building the Optimizely object
+     * @throws Exception
+     */
+    @Test
+    public void initializationOccursForBucketerWhenBuildingOptly() throws Exception {
+        String datafile = validConfigJson();
+        ProjectConfig projectConfig = validProjectConfig();
+        EventBuilder mockEventBuilder = mock(EventBuilder.class);
+
+        Optimizely.builder(datafile, mockEventHandler)
+            .withBucketing(mockBucketer)
+            .withEventBuilder(mockEventBuilder)
+            .withConfig(projectConfig)
+            .withErrorHandler(mockErrorHandler)
+            .build();
+
+        verify(mockBucketer).cleanUserExperimentRecord();
+    }
+
     /**
      * Verify that the {@link Optimizely#activate(Experiment, String, Map)} DOES NOT dispatch an impression event
      * when the user isn't bucketed to a variation.