From 6fdd30d920355c6eef15546a8fd091c2874f7814 Mon Sep 17 00:00:00 2001 From: Vignesh Raja Date: Wed, 11 Jan 2017 18:16:05 -0800 Subject: [PATCH] Persist experiment and variation IDs instead of keys in UserProfile --- .../com/optimizely/ab/bucketing/Bucketer.java | 31 ++++++++++--------- .../optimizely/ab/bucketing/UserProfile.java | 20 ++++++------ .../optimizely/ab/bucketing/BucketerTest.java | 22 +++++++------ 3 files changed, 38 insertions(+), 35 deletions(-) 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 index 7200600d5..71f6eab84 100644 --- a/core-api/src/main/java/com/optimizely/ab/bucketing/Bucketer.java +++ b/core-api/src/main/java/com/optimizely/ab/bucketing/Bucketer.java @@ -114,17 +114,18 @@ private Variation bucketToVariation(@Nonnull Experiment experiment, // If a user profile instance is present then check it for a saved variation if (userProfile != null) { - String variationKey = userProfile.lookup(userId, experimentKey); - if (variationKey != null) { + String variationId = userProfile.lookup(userId, experimentId); + if (variationId != null) { + Variation savedVariation = projectConfig + .getExperimentIdMapping() + .get(experimentId) + .getVariationIdToVariationMap() + .get(variationId); logger.info("Returning previously activated variation \"{}\" of experiment \"{}\" " + "for user \"{}\" from user profile.", - variationKey, experimentKey, userId); + savedVariation.getKey(), experimentKey, userId); // A variation is stored for this combined bucket id - return projectConfig - .getExperimentIdMapping() - .get(experimentId) - .getVariationKeyToVariationMap() - .get(variationKey); + return savedVariation; } else { logger.info("No previously activated variation of experiment \"{}\" " + "for user \"{}\" found in user profile.", @@ -142,18 +143,18 @@ private Variation bucketToVariation(@Nonnull Experiment experiment, if (bucketedVariationId != null) { Variation bucketedVariation = experiment.getVariationIdToVariationMap().get(bucketedVariationId); String variationKey = bucketedVariation.getKey(); - logger.info("User \"{}\" is in variation \"{}\" of experiment \"{}\".", userId, variationKey, + logger.info("User \"{}\" is in variation \"{}\" of experiment \"{}\".", userId, variationKey, experimentKey); // If a user profile is present give it a variation to store if (userProfile != null) { - boolean saved = userProfile.save(userId, experiment.getKey(), variationKey); + boolean saved = userProfile.save(userId, experimentId, bucketedVariationId); if (saved) { logger.info("Saved variation \"{}\" of experiment \"{}\" for user \"{}\".", - variationKey, experimentKey, userId); + bucketedVariationId, experimentId, userId); } else { logger.warn("Failed to save variation \"{}\" of experiment \"{}\" for user \"{}\".", - variationKey, experimentKey, userId); + bucketedVariationId, experimentId, userId); } } @@ -237,10 +238,10 @@ public void cleanUserProfiles() { Map> records = userProfile.getAllRecords(); if (records != null) { for (Map.Entry> record : records.entrySet()) { - for (String experimentKey : record.getValue().keySet()) { - Experiment experiment = projectConfig.getExperimentKeyMapping().get(experimentKey); + for (String experimentId : record.getValue().keySet()) { + Experiment experiment = projectConfig.getExperimentIdMapping().get(experimentId); if (experiment == null || !experiment.isRunning()) { - userProfile.remove(record.getKey(), experimentKey); + userProfile.remove(record.getKey(), experimentId); } } } diff --git a/core-api/src/main/java/com/optimizely/ab/bucketing/UserProfile.java b/core-api/src/main/java/com/optimizely/ab/bucketing/UserProfile.java index 152bc479b..c5405d9ae 100644 --- a/core-api/src/main/java/com/optimizely/ab/bucketing/UserProfile.java +++ b/core-api/src/main/java/com/optimizely/ab/bucketing/UserProfile.java @@ -31,20 +31,20 @@ public interface UserProfile { * 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 + * @param experimentId the experiment ID of the activation + * @param variationId the variation ID of the activation * @return true if saving of the record was successful */ - boolean save(String userId, String experimentKey, String variationKey); + boolean save(String userId, String experimentId, String variationId); /** * 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 + * @param experimentId the experiment ID of the next activation + * @return the variation ID of the next activation, or null if no record exists */ - String lookup(String userId, String experimentKey); + String lookup(String userId, String experimentId); /** * Called when user profile should be removed @@ -53,18 +53,18 @@ public interface UserProfile { * deleted. * * @param userId the user id of the record to remove - * @param experimentKey the experiment key of the record to remove + * @param experimentId the experiment ID of the record to remove * @return true if the record was removed */ - boolean remove(String userId, String experimentKey); + boolean remove(String userId, String experimentId); /** * Called by bucketer to get a mapping of all stored records * * 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 + * @return a map of user IDs to a map of experiment IDs to variation IDs */ - Map> getAllRecords(); + Map> getAllRecords(); } 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 index 1ba38bebb..1e571945e 100644 --- a/core-api/src/test/java/com/optimizely/ab/bucketing/BucketerTest.java +++ b/core-api/src/test/java/com/optimizely/ab/bucketing/BucketerTest.java @@ -460,14 +460,15 @@ public void bucketUserNotInOverlappingGroupExperiment() throws Exception { Experiment groupExperiment = groupExperiments.get(0); final Variation variation = groupExperiment.getVariations().get(0); - when(userProfile.save("blah", groupExperiment.getKey(), variation.getKey())).thenReturn(true); + when(userProfile.save("blah", groupExperiment.getId(), variation.getId())).thenReturn(true); assertThat(algorithm.bucket(groupExperiment, "blah"), is(variation)); logbackVerifier.expectMessage(Level.INFO, - "Saved variation \"e2_vtag1\" of experiment \"group_etag2\" for user \"blah\"."); + String.format("Saved variation \"%s\" of experiment \"%s\" for user \"blah\".", variation.getId(), + groupExperiment.getId())); - verify(userProfile).save("blah", groupExperiment.getKey(), variation.getKey()); + verify(userProfile).save("blah", groupExperiment.getId(), variation.getId()); } /** @@ -485,14 +486,15 @@ public void bucketUserNotInOverlappingGroupExperiment() throws Exception { Experiment groupExperiment = groupExperiments.get(0); final Variation variation = groupExperiment.getVariations().get(0); - when(userProfile.save("blah", groupExperiment.getKey(), variation.getKey())).thenReturn(false); + when(userProfile.save("blah", groupExperiment.getId(), variation.getId())).thenReturn(false); assertThat(algorithm.bucket(groupExperiment, "blah"), is(variation)); logbackVerifier.expectMessage(Level.WARN, - "Failed to save variation \"e2_vtag1\" of experiment \"group_etag2\" for user \"blah\"."); + String.format("Failed to save variation \"%s\" of experiment \"%s\" for user \"blah\".", + variation.getId(), groupExperiment.getId())); - verify(userProfile).save("blah", groupExperiment.getKey(), variation.getKey()); + verify(userProfile).save("blah", groupExperiment.getId(), variation.getId()); } /** @@ -510,7 +512,7 @@ public void bucketUserNotInOverlappingGroupExperiment() throws Exception { Experiment groupExperiment = groupExperiments.get(0); final Variation variation = groupExperiment.getVariations().get(0); - when(userProfile.lookup("blah", groupExperiment.getKey())).thenReturn(variation.getKey()); + when(userProfile.lookup("blah", groupExperiment.getId())).thenReturn(variation.getId()); assertThat(algorithm.bucket(groupExperiment, "blah"), is(variation)); @@ -518,7 +520,7 @@ public void bucketUserNotInOverlappingGroupExperiment() throws Exception { "Returning previously activated variation \"e2_vtag1\" of experiment \"group_etag2\"" + " for user \"blah\" from user profile."); - verify(userProfile).lookup("blah", groupExperiment.getKey()); + verify(userProfile).lookup("blah", groupExperiment.getId()); } /** @@ -536,13 +538,13 @@ public void bucketUserNotInOverlappingGroupExperiment() throws Exception { Experiment groupExperiment = groupExperiments.get(0); final Variation variation = groupExperiment.getVariations().get(0); - when(userProfile.lookup("blah", groupExperiment.getKey())).thenReturn(null); + when(userProfile.lookup("blah", groupExperiment.getId())).thenReturn(null); assertThat(algorithm.bucket(groupExperiment, "blah"), is(variation)); logbackVerifier.expectMessage(Level.INFO, "No previously activated variation of experiment " + "\"group_etag2\" for user \"blah\" found in user profile."); - verify(userProfile).lookup("blah", groupExperiment.getKey()); + verify(userProfile).lookup("blah", groupExperiment.getId()); } /**