Changes persistent bucketer to user experiment record, fixes whitespace,

Josh Deffibaugh · Josh Deffibaugh · commit 91e042766f76 · 2016-08-26T13:56:36.000-07:00
typos, naming, etc from PR review
diff --git a/core-api/src/main/java/com/optimizely/ab/Optimizely.java b/core-api/src/main/java/com/optimizely/ab/Optimizely.java
@@ -96,7 +96,7 @@ private Optimizely(@Nonnull ProjectConfig projectConfig,
 
     // Do work here that should be done once per Optimizely lifecycle
     @VisibleForTesting void initialize() {
-        bucketer.cleanUserExperimentRecord();
+        bucketer.cleanUserExperimentRecords();
     }
 
     //======== activate calls ========//
@@ -457,9 +457,7 @@ protected Builder withEventBuilder(EventBuilder eventBuilder) {
             return this;
         }
 
-        /**
-         * Helper function for making testing easier
-         */
+        // Helper function for making testing easier
         protected Builder withConfig(ProjectConfig projectConfig) {
             this.projectConfig = projectConfig;
             return this;
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
@@ -108,22 +108,30 @@ private Experiment bucketToExperiment(@Nonnull Group group,
     private Variation bucketToVariation(@Nonnull Experiment experiment,
                                         @Nonnull String userId) {
         // "salt" the bucket id using the experiment id
-        String combinedBucketId = userId + experiment.getId();
+        String experimentId = experiment.getId();
+        String experimentKey = experiment.getKey();
+        String combinedBucketId = userId + experimentId;
 
-        // If a persistent bucketer instance is present then check it for a persisted variation
+        // If a user experiment record instance is present then check it for a saved variation
         if (userExperimentRecord != null) {
-            String variationKey = userExperimentRecord.lookup(userId, experiment.getKey());
+            String variationKey = userExperimentRecord.lookup(userId, experimentKey);
             if (variationKey != null) {
-                logger.info("Returning previously activated variation \"{}\" from user experiment registry.", variationKey);
+                logger.info("Returning previously activated variation \"{}\" of experiment \"{}\" "
+                            + "for user \"{}\" from user experiment record.",
+                            variationKey, experimentKey, userId);
                 // A variation is stored for this combined bucket id
-                return projectConfig.getExperimentIdMapping().get(experiment.getId()).getVariationKeyToVariationMap().get(variationKey);
+                return projectConfig
+                    .getExperimentIdMapping()
+                    .get(experimentId)
+                    .getVariationKeyToVariationMap()
+                    .get(variationKey);
             } else {
-                logger.info("No previously activated variation returned from persistent bucketer.");
+                logger.info("No previously activated variation of experiment \"{}\" "
+                            + "for user \"{}\" found in user experiment record.",
+                            experimentKey, userId);
             }
         }
 
-        String experimentKey = experiment.getKey();
-
         List<TrafficAllocation> trafficAllocations = experiment.getTrafficAllocation();
 
         int hashCode = MurmurHash3.murmurhash3_x86_32(combinedBucketId, 0, combinedBucketId.length(), MURMUR_HASH_SEED);
@@ -133,16 +141,19 @@ private Variation bucketToVariation(@Nonnull Experiment experiment,
         String bucketedVariationId = bucketToEntity(bucketValue, trafficAllocations);
         if (bucketedVariationId != null) {
             Variation bucketedVariation = experiment.getVariationIdToVariationMap().get(bucketedVariationId);
-            logger.info("User \"{}\" is in variation \"{}\" of experiment \"{}\".", userId, bucketedVariation.getKey(),
+            String variationKey = bucketedVariation.getKey();
+                logger.info("User \"{}\" is in variation \"{}\" of experiment \"{}\".", userId, variationKey,
                         experimentKey);
 
-            // If a persistent bucketer instance is present give it a variation to store
+            // If a user experiment record is present give it a variation to store
             if (userExperimentRecord != null) {
-                boolean saved = userExperimentRecord.save(userId, experiment.getKey(), bucketedVariation.getKey());
+                boolean saved = userExperimentRecord.save(userId, experiment.getKey(), variationKey);
                 if (saved) {
-                    logger.info("Persisted variation \"{}\" of experiment \"{}\".", bucketedVariation.getKey(), experimentKey);
+                    logger.info("Saved variation \"{}\" of experiment \"{}\" for user \"{}\".",
+                                variationKey, experimentKey, userId);
                 } else {
-                    logger.warn("Failed to persist variation \"{}\" of experiment \"{}\".", bucketedVariation.getKey(), experimentKey);
+                    logger.warn("Failed to save variation \"{}\" of experiment \"{}\" for user \"{}\".",
+                                variationKey, experimentKey, userId);
                 }
             }
 
@@ -221,9 +232,9 @@ public UserExperimentRecord getUserExperimentRecord() {
      * Gives implementations of {@link UserExperimentRecord} a chance to remove records
      * of experiments that are deleted or not running.
      */
-    public void cleanUserExperimentRecord() {
+    public void cleanUserExperimentRecords() {
         if (userExperimentRecord != null) {
-            Map<String,Map<String,String>> records = userExperimentRecord.records();
+            Map<String, Map<String,String>> records = userExperimentRecord.getAllRecords();
             if (records != null) {
                 for (Map.Entry<String,Map<String,String>> record : records.entrySet()) {
                     for (String experimentKey : record.getValue().keySet()) {
diff --git a/core-api/src/main/java/com/optimizely/ab/bucketing/UserExperimentRecord.java b/core-api/src/main/java/com/optimizely/ab/bucketing/UserExperimentRecord.java
@@ -1,3 +1,19 @@
+/**
+ *
+ *    Copyright 2016, Optimizely
+ *
+ *    Licensed under the Apache License, Version 2.0 (the "License");
+ *    you may not use this file except in compliance with the License.
+ *    You may obtain a copy of the License at
+ *
+ *        http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *    Unless required by applicable law or agreed to in writing, software
+ *    distributed under the License is distributed on an "AS IS" BASIS,
+ *    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *    See the License for the specific language governing permissions and
+ *    limitations under the License.
+ */
 package com.optimizely.ab.bucketing;
 
 import java.util.Map;
@@ -23,6 +39,7 @@ public interface UserExperimentRecord {
 
     /**
      * 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
@@ -44,10 +61,10 @@ public interface UserExperimentRecord {
     /**
      * 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
+     * This mapping is needed so that the bucketer can {@link #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();
+    Map<String, Map<String,String>> getAllRecords();
 
 }
diff --git a/core-api/src/main/java/com/optimizely/ab/event/EventHandler.java b/core-api/src/main/java/com/optimizely/ab/event/EventHandler.java
@@ -19,7 +19,7 @@
 import java.util.Map;
 
 /**
- * Implementations are responsible for dispatching {@link LogEvent}s to the Optimizely event end-point.
+ * Implementations are responsible for dispatching event's to the Optimizely event end-point.
  */
 public interface EventHandler {
 
diff --git a/core-api/src/test/java/com/optimizely/ab/OptimizelyBuilderTest.java b/core-api/src/test/java/com/optimizely/ab/OptimizelyBuilderTest.java
@@ -88,7 +88,7 @@ public void withDefaultErrorHandler() throws Exception {
     }
 
     @Test
-    public void withPersistentBucketer() throws Exception {
+    public void withUserExperimentRecord() throws Exception {
         UserExperimentRecord userExperimentRecord = mock(UserExperimentRecord.class);
         Optimizely optimizelyClient = Optimizely.builder(validConfigJson(), mockEventHandler)
                 .withUserExperimentRecord(userExperimentRecord)
diff --git a/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java b/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java
@@ -155,7 +155,7 @@ public void initializationOccursForBucketerWhenBuildingOptly() throws Exception
             .withErrorHandler(mockErrorHandler)
             .build();
 
-        verify(mockBucketer).cleanUserExperimentRecord();
+        verify(mockBucketer).cleanUserExperimentRecords();
     }
 
     /**
diff --git a/core-api/src/test/java/com/optimizely/ab/bucketing/BucketerTest.java b/core-api/src/test/java/com/optimizely/ab/bucketing/BucketerTest.java
@@ -252,8 +252,6 @@ public void bucketUserInForcedVariation() throws Exception {
         assertThat(algorithm.bucket(experiment, "testUser1").getKey(), is("var1"));
     }
 
-
-
     /**
      * Verify that {@link Bucketer#bucket(Experiment, String)} returns the proper variation when the user doesn't
      * have a forced variation mapping.
@@ -447,10 +445,14 @@ public void bucketUserNotInOverlappingGroupExperiment() throws Exception {
         assertNull(algorithm.bucket(groupExperiment, "blah"));
     }
 
-    @Test public void bucketUserSaveActivationWithPersistentBucketer() throws Exception {
+    /**
+     * Verify that {@link Bucketer#bucket(Experiment,String} saves a variation of an experiment for a user
+     * when a {@link UserExperimentRecord} is present.
+     */
+    @Test public void bucketUserSaveActivationWithUserExperimentRecord() throws Exception {
         final AtomicInteger bucketValue = new AtomicInteger();
         UserExperimentRecord userExperimentRecord = mock(UserExperimentRecord.class);
-        Bucketer algorithm = mockPersistentBucketAlgorithm(bucketValue, userExperimentRecord);
+        Bucketer algorithm = mockUserExperimentRecordAlgorithm(bucketValue, userExperimentRecord);
         bucketValue.set(3000);
 
         ProjectConfig projectConfig = validProjectConfig();
@@ -463,15 +465,19 @@ public void bucketUserNotInOverlappingGroupExperiment() throws Exception {
         assertThat(algorithm.bucket(groupExperiment, "blah"),  is(variation));
 
         logbackVerifier.expectMessage(Level.INFO,
-                "Persisted variation \"e2_vtag1\" of experiment \"group_etag2\".");
+                "Saved variation \"e2_vtag1\" of experiment \"group_etag2\" for user \"blah\".");
 
         verify(userExperimentRecord).save("blah", groupExperiment.getKey(), variation.getKey());
     }
 
-    @Test public void bucketUserSaveActivationFailWithPersistentBucketer() throws Exception {
+    /**
+     * Verify that {@link Bucketer#bucket(Experiment,String} logs correctly
+     * when a {@link UserExperimentRecord} is present and fails to save an activation.
+     */
+    @Test public void bucketUserSaveActivationFailWithUserExperimentRecord() throws Exception {
         final AtomicInteger bucketValue = new AtomicInteger();
         UserExperimentRecord userExperimentRecord = mock(UserExperimentRecord.class);
-        Bucketer algorithm = mockPersistentBucketAlgorithm(bucketValue, userExperimentRecord);
+        Bucketer algorithm = mockUserExperimentRecordAlgorithm(bucketValue, userExperimentRecord);
         bucketValue.set(3000);
 
         ProjectConfig projectConfig = validProjectConfig();
@@ -484,15 +490,19 @@ public void bucketUserNotInOverlappingGroupExperiment() throws Exception {
         assertThat(algorithm.bucket(groupExperiment, "blah"),  is(variation));
 
         logbackVerifier.expectMessage(Level.WARN,
-                "Failed to persist variation \"e2_vtag1\" of experiment \"group_etag2\".");
+                "Failed to save variation \"e2_vtag1\" of experiment \"group_etag2\" for user \"blah\".");
 
         verify(userExperimentRecord).save("blah", groupExperiment.getKey(), variation.getKey());
     }
 
-    @Test public void bucketUserRestoreActivationWithPersistentBucketer() throws Exception {
+    /**
+     * Verify that {@link Bucketer#bucket(Experiment,String)} returns a variation that is
+     * stored in the provided {@link userExperimentRecord}.
+     */
+    @Test public void bucketUserRestoreActivationWithUserExperimentRecord() throws Exception {
         final AtomicInteger bucketValue = new AtomicInteger();
         UserExperimentRecord userExperimentRecord = mock(UserExperimentRecord.class);
-        Bucketer algorithm = mockPersistentBucketAlgorithm(bucketValue, userExperimentRecord);
+        Bucketer algorithm = mockUserExperimentRecordAlgorithm(bucketValue, userExperimentRecord);
         bucketValue.set(3000);
 
         ProjectConfig projectConfig = validProjectConfig();
@@ -505,15 +515,20 @@ public void bucketUserNotInOverlappingGroupExperiment() throws Exception {
         assertThat(algorithm.bucket(groupExperiment, "blah"),  is(variation));
 
         logbackVerifier.expectMessage(Level.INFO,
-                "Returning previously activated variation \"e2_vtag1\" from user experiment registry.");
+                "Returning previously activated variation \"e2_vtag1\" of experiment \"group_etag2\""
+                                      + " for user \"blah\" from user experiment record.");
 
         verify(userExperimentRecord).lookup("blah", groupExperiment.getKey());
     }
 
-    @Test public void bucketUserRestoreActivationNullWithPersistentBucketer() throws Exception {
+    /**
+     * Verify {@link Bucketer#bucket(Experiment,String)} handles a present {@link UserExperimentRecord}
+     * returning null when looking up a variation.
+     */
+    @Test public void bucketUserRestoreActivationNullWithUserExperimentRecord() throws Exception {
         final AtomicInteger bucketValue = new AtomicInteger();
         UserExperimentRecord userExperimentRecord = mock(UserExperimentRecord.class);
-        Bucketer algorithm = mockPersistentBucketAlgorithm(bucketValue, userExperimentRecord);
+        Bucketer algorithm = mockUserExperimentRecordAlgorithm(bucketValue, userExperimentRecord);
         bucketValue.set(3000);
 
         ProjectConfig projectConfig = validProjectConfig();
@@ -525,73 +540,87 @@ public void bucketUserNotInOverlappingGroupExperiment() throws Exception {
 
         assertThat(algorithm.bucket(groupExperiment, "blah"),  is(variation));
 
-        logbackVerifier.expectMessage(Level.INFO,
-                "No previously activated variation returned from persistent bucketer.");
-
+        logbackVerifier.expectMessage(Level.INFO, "No previously activated variation of experiment " +
+                                      "\"group_etag2\" for user \"blah\" found in user experiment record.");
         verify(userExperimentRecord).lookup("blah", groupExperiment.getKey());
     }
 
+    /**
+     * Verify {@link Bucketer#cleanUserExperimentRecords()} handles a null {@link UserExperimentRecord}.
+     */
     @Test
     public void nullUserExperimentRecordWhenCleaning() {
         final AtomicInteger bucketValue = new AtomicInteger();
         Bucketer algorithm = mockBucketAlgorithm(bucketValue);
         bucketValue.set(3000);
         try {
-            algorithm.cleanUserExperimentRecord();
+            algorithm.cleanUserExperimentRecords();
         } catch (NullPointerException e) {
             fail();
         }
     }
 
+    /**
+     * Verify {@link Bucketer#cleanUserExperimentRecords()} handles a null returned from
+     * {@link UserExperimentRecord#getAllRecords()}.
+     */
     @Test
     public void nullUserExperimentRecords() {
         final AtomicInteger bucketValue = new AtomicInteger();
         UserExperimentRecord userExperimentRecord = mock(UserExperimentRecord.class);
-        Bucketer algorithm = mockPersistentBucketAlgorithm(bucketValue, userExperimentRecord);
+        Bucketer algorithm = mockUserExperimentRecordAlgorithm(bucketValue, userExperimentRecord);
         bucketValue.set(3000);
 
-        when(userExperimentRecord.records()).thenReturn(null);
+        when(userExperimentRecord.getAllRecords()).thenReturn(null);
         try {
-            algorithm.cleanUserExperimentRecord();
+            algorithm.cleanUserExperimentRecords();
         } catch (NullPointerException e) {
             fail();
         }
     }
 
+    /**
+     * Verify {@link Bucketer#cleanUserExperimentRecords()} removes experiments
+     * that are no longer in the {@link ProjectConfig}.
+     */
     @Test
     public void cleanRemovesRecordsOfExperimentsThatNoLongerExist() {
         final AtomicInteger bucketValue = new AtomicInteger();
         UserExperimentRecord userExperimentRecord = mock(UserExperimentRecord.class);
-        Bucketer algorithm = mockPersistentBucketAlgorithm(bucketValue, userExperimentRecord);
+        Bucketer algorithm = mockUserExperimentRecordAlgorithm(bucketValue, userExperimentRecord);
         bucketValue.set(3000);
 
         Map<String,Map<String,String>> records = new HashMap<String, Map<String, String>>();
         Map<String,String> activation = new HashMap<String, String>();
-        activation.put("57", "57");
+        activation.put("exp1", "var1");
         records.put("blah",  activation);
-        when(userExperimentRecord.records()).thenReturn(records);
+        when(userExperimentRecord.getAllRecords()).thenReturn(records);
 
-        algorithm.cleanUserExperimentRecord();
+        algorithm.cleanUserExperimentRecords();
 
-        verify(userExperimentRecord).remove("blah", "57");
+        verify(userExperimentRecord).remove("blah", "exp1");
     }
 
+    /**
+     * Verify {@link Bucketer#cleanUserExperimentRecords()} removes experiments
+     * that are paused in the {@link ProjectConfig}.
+     */
     @Test
     public void cleanRemovesRecordsOfExperimentsThatAreNotRunning() {
         final AtomicInteger bucketValue = new AtomicInteger();
         UserExperimentRecord userExperimentRecord = mock(UserExperimentRecord.class);
-        Bucketer algorithm = mockPersistentBucketAlgorithm(bucketValue, userExperimentRecord);
+        Bucketer algorithm = mockUserExperimentRecordAlgorithm(bucketValue, userExperimentRecord);
         bucketValue.set(3000);
 
         Map<String,Map<String,String>> records = new HashMap<String, Map<String, String>>();
         Map<String,String> activation = new HashMap<String, String>();
-        activation.put("118", "57");
+        activation.put("exp1", "var1");
         records.put("blah",  activation);
-        when(userExperimentRecord.records()).thenReturn(records);
+        when(userExperimentRecord.getAllRecords()).thenReturn(records);
 
-        algorithm.cleanUserExperimentRecord();
+        algorithm.cleanUserExperimentRecords();
 
-        verify(userExperimentRecord).remove("blah", "118");
+        verify(userExperimentRecord).remove("blah", "exp1");
     }
 
     //======== Helper methods ========//
@@ -618,12 +647,12 @@ int generateBucketValue(int hashCode) {
      * @param bucketValue the expected bucket value holder
      * @return the mock bucket algorithm
      */
-    private Bucketer mockPersistentBucketAlgorithm(final AtomicInteger bucketValue, final UserExperimentRecord userExperimentRecord) {
+    private Bucketer mockUserExperimentRecordAlgorithm(final AtomicInteger bucketValue, final UserExperimentRecord userExperimentRecord) {
         return new Bucketer(validProjectConfig(), userExperimentRecord) {
             @Override
             int generateBucketValue(int hashCode) {
                 return bucketValue.get();
             }
         };
     }
-}
+}

Original file line number	Diff line number	Diff line change
`@@ -88,7 +88,7 @@ public void withDefaultErrorHandler() throws Exception {`
`88`	`88`	`}`
`89`	`89`
`90`	`90`	`@Test`
`91`		`- public void withPersistentBucketer() throws Exception {`
	`91`	`+ public void withUserExperimentRecord() throws Exception {`
`92`	`92`	`UserExperimentRecord userExperimentRecord = mock(UserExperimentRecord.class);`
`93`	`93`	`Optimizely optimizelyClient = Optimizely.builder(validConfigJson(), mockEventHandler)`
`94`	`94`	`.withUserExperimentRecord(userExperimentRecord)`
Original file line number	Diff line number	Diff line change
`@@ -155,7 +155,7 @@ public void initializationOccursForBucketerWhenBuildingOptly() throws Exception`
`155`	`155`	`.withErrorHandler(mockErrorHandler)`
`156`	`156`	`.build();`
`157`	`157`
`158`		`- verify(mockBucketer).cleanUserExperimentRecord();`
	`158`	`+ verify(mockBucketer).cleanUserExperimentRecords();`
`159`	`159`	`}`
`160`	`160`
`161`	`161`	`/**`