Skip to content

Josh/persistent bucketer #1

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 15 commits into from
Aug 29, 2016
Merged

Josh/persistent bucketer #1

merged 15 commits into from
Aug 29, 2016

Conversation

jophde
Copy link
Contributor

@jophde jophde commented Aug 8, 2016 •
edited
Loading

This adds a StickyBucketer interface and hooks it up to the bucketer.

EventHandler eventHandler = OptlyEventHandler.getInstance(context);
        PersistentBucketer persistentBucketer = AndroidPersistentBucketer.newInstance(context);
        Optimizely.builder(dataFile, eventHandler)
            .withPersistentBucketer(persistentBucketer)
            .build();

I also had to bump the gradle version to get things to build on my version of IntelliJ. I am still unable to get the project building from IntelliJ but i can I build and run tests from the command line. All the tests are passing. I am not sure if the issue on my machine or in the build. It was like this on the HEAD of master.

@jophde jophde mentioned this pull request Aug 9, 2016
Merged
aliabbasrizvi
aliabbasrizvi reviewed Aug 11, 2016
View reviewed changes
.gitignore
@@ -14,6 +14,9 @@
.classpath
.project

# android studio config files
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dont think it makes sense for Java SDK. Probably for Android SDK.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will need to expense IntelliJ Ultimate then. Android Studio adds the file automatically. It's also useful to work on the android modules and java module at the same time in Android Studio.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Makes sense.

aliabbasrizvi
aliabbasrizvi reviewed Aug 15, 2016
View reviewed changes
core-api/src/main/java/com/optimizely/ab/Optimizely.java
@@ -93,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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit. Comment needs updating.

aliabbasrizvi
aliabbasrizvi reviewed Aug 15, 2016
View reviewed changes
gradle.properties
@@ -1,5 +1,5 @@
# Maven version
version = 0.1.68-SNAPSHOT
version = 0.1.70-SNAPSHOT
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vraja2 FYI

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not 0.1.69?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since there public API changes shouldn't it go to 0.1.70 per semantic versioning? I don't care either way let me know.

vraja2
vraja2 reviewed Aug 16, 2016
View reviewed changes
core-api/src/test/java/com/optimizely/ab/bucketing/BucketerTest.java
@@ -440,6 +447,153 @@ public void bucketUserNotInOverlappingGroupExperiment() throws Exception {
assertNull(algorithm.bucket(groupExperiment, "blah"));
}

@Test public void bucketUserSaveActivationWithPersistentBucketer() throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add docstrings describing the new tests?

@mikeproeng37
Copy link
Contributor

LGTM, there are mostly just minor things to address, but once those are done, merge it!

@jophde
Copy link
Contributor Author

jophde commented Aug 25, 2016

I addressed everyone's comments. The only outstanding thing is changing the name of UserExperimentRecord#records() to something else. I was thinking UserExperimentRecords#getAllRecords(). Also, let me know if you want to bump to 0.1.70 or go to 0.1.69. There is no breaking API but there is new API. @vraja2 @aliabbasrizvi @mikeng13

vraja2
vraja2 reviewed Aug 26, 2016
View reviewed changes
core-api/src/main/java/com/optimizely/ab/bucketing/Bucketer.java

// If a user experiment record is present give it a variation to store
if (userExperimentRecord != null) {
boolean saved = userExperimentRecord.save(userId, experiment.getKey(), variationKey);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use experimentKey here

@vraja2
Copy link
Contributor

vraja2 commented Aug 26, 2016

Looks great, just a couple of nits. Can you run "./gradlew check" locally before merging also? Thanks for addressing everything!

aliabbasrizvi
aliabbasrizvi reviewed Aug 26, 2016
View reviewed changes
README.md
@@ -1,6 +1,6 @@
Optimizely Java SDK
===================
[![Build Status](https://travis-ci.com/optimizely/optimizely-ab-java-sdk.svg?token=qN2PbasvLidLamENwDAs&branch=master)](https://travis-ci.com/optimizely/java-sdk)
[![Build Status](https://travis-ci.org/optimizely/java-sdk.svg?branch=master)](https://travis-ci.org/optimizely/java-sdk)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jophde you may have to rebase.

cc @vraja2

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

those are the latest links, public repos use .org not .com for Travis

@jophde jophde force-pushed the josh/persistent-bucketer branch from 91531a0 to 91e0427 Compare August 26, 2016 20:57
@jophde jophde merged commit 45c775e into master Aug 29, 2016
@jophde jophde deleted the josh/persistent-bucketer branch August 29, 2016 16:07
mikeproeng37 pushed a commit that referenced this pull request Jan 11, 2019
@jamesneimanconsulting
Merge pull request #1 from optimizely/master
f06b197 
merge master into fork
loganlinn pushed a commit that referenced this pull request Mar 29, 2019
Extract ProjectConfig interface and create DatafileProjectConfig. (#1)
255fa25 
ProjectConfig is the current access point into the configuration of experiments, rollouts, audiences, etc. Currently it is basically a wrapper around the datafile, but can potentially be backed by other implementations. Extracting out the interface now will make it easier to distance ourselves from being tightly coupled to the current datafile concepts.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jophde @mikeproeng37 @vraja2 @aliabbasrizvi