(chore): Use the project config manager in java sdk with android. Also add listener. #276

thomaszurkan-optimizely · 2019-06-26T00:29:12Z

Summary

This implements the new project config. It also integrates with the android datafile handler polling. By using a FileObserver available in Android, we update the project config when the cached file is modified.

What this means is that now when a new datafile comes in for android, the config is updated on the fly.
I haven't added any tests to this yet. But, I wanted to get some comments more sooner than later.

Test plan

mikecdavis

I think we can remove all of the injectOptimizely logic since this is now happening internally within the ProjectConfigManager. Was this kept for backwards compatibility with custom datafile handlers?

mikecdavis · 2019-06-27T16:44:46Z

android-sdk/src/androidTest/java/com/optimizely/ab/android/sdk/OptimizelyManagerTest.java

+        try {
+            config = new DatafileProjectConfig.Builder().withDatafile(datafile).build();
+        }
+        catch (Exception e) {


Can we throw from the method signature?

mikecdavis · 2019-06-27T16:51:19Z

android-sdk/src/main/java/com/optimizely/ab/android/sdk/OptimizelyManager.java

-                    datafileHandler.startBackgroundUpdates(context, datafileConfig, datafileDownloadInterval);
+                    datafileHandler.startBackgroundUpdates(context, datafileConfig, datafileDownloadInterval, datafile1 -> {
+                        if (getOptimizely() != null && getOptimizely().getNotificationCenter() != null)
+                            getOptimizely().getNotificationCenter().send(new UpdateConfigNotification());


Can we use braces? "goto fail;"

mikecdavis · 2019-06-27T16:51:35Z

android-sdk/src/main/java/com/optimizely/ab/android/sdk/OptimizelyManager.java

+            datafileHandler.startBackgroundUpdates(context, datafileConfig, datafileDownloadInterval, datafile1 -> {
+                // fire
+                if (getOptimizely() != null && getOptimizely().getNotificationCenter() != null)
+                getOptimizely().getNotificationCenter().send(new UpdateConfigNotification());


braces please

mikecdavis · 2019-06-27T16:57:50Z

...handler/src/main/java/com/optimizely/ab/android/datafile_handler/DefaultDatafileHandler.java

 * interaction point to the datafile-handler module.
 */
 public class DefaultDatafileHandler implements DatafileHandler {
+    private Logger logger = LoggerFactory.getLogger("DefaultDatafileHandler");


private static final Logger logger = LoggerFactory.getLogger(DefaultDatafileHandler.class);

mikecdavis · 2019-06-27T17:01:04Z

...handler/src/main/java/com/optimizely/ab/android/datafile_handler/DefaultDatafileHandler.java

+        );
+
+        File filesFolder = context.getFilesDir();
+        fileObserver = new FileObserver(filesFolder.getPath()) {


should we check that fileObserver == null? Just curious what would happen if startBackgroundUpdates was called multiple times?

there is a stop in the start. the stop clears the fileObserver properly.

mikecdavis · 2019-06-27T17:10:28Z

...handler/src/main/java/com/optimizely/ab/android/datafile_handler/DefaultDatafileHandler.java

+    }
+
+    public DefaultDatafileHandler(DatafileConfig datafileConfig) {
+        this.datafileConfig = datafileConfig;


datafile handler can be instantiated by the use and passed into the constructor. this is meant for that. it is also used by the convenience constructor.

The question here, is should deprecate the old methods that passed in a project config and use the internal dataconfig created?

mikecdavis · 2019-06-27T17:12:23Z

...handler/src/main/java/com/optimizely/ab/android/datafile_handler/DatafileChangeListener.java

@@ -0,0 +1,5 @@
+package com.optimizely.ab.android.datafile_handler;
+
+public interface DatafileChangeListener {


Do we need this listener? Can't we leverage the existing DatafileLoadedListener

Didn't want to overload datafile load listener. Also, the one method per interface allows for lambdas.

Not sure it's overloading as semantically we are "loading" a datafile.
JavaDoc for DatafileLoadedListener:

Listens for new Optimizely datafiles

mikecdavis · 2019-06-27T17:18:57Z

...handler/src/main/java/com/optimizely/ab/android/datafile_handler/DefaultDatafileHandler.java

    }
+
+    public void setDatafile(String datafile) {
+        if (currentProjectConfig == null && datafile != null && !datafile.isEmpty()) {


Q: We only want to set this once?

Style thing, but I prefer to fail fast and to group logical conditions. I think it makes the code easier to grok, log and requires less nesting which helps readability.

public void setDatafile(String datafile) { if (currentProjectConfig == null) { logger.info("Datafile already set, ignoring update."); return; } if (datafile != null && !datafile.isEmpty()) { logger.info("datafile is empty, ignoring update"); return; } try { currentProjectConfig = new DatafileProjectConfig.Builder().withDatafile(datafile).build(); logger.info("Datafile successfully loaded with revision: {}", currentProjectConfig.getRevision()); } catch (ConfigParseException ex) { logger.error("Unable to parse the datafile", ex); logger.info("Datafile is invalid: {}", datafile); } }

my bad. the currentProjectConfig is not checked.

thanks. consider, removing the nesting by exiting early. Think of the empty check as a precondition.

if (datafile != null && !datafile.isEmpty()) { logger.info("datafile is empty, ignoring update"); return; }

this also adds visibility, etc.

I'm not sure I follow you. Are you saying that this check should be pulled out of the method? In general I think that if you are taking a more functional approach (not easy in java), you want functions to be self contained and not be potentially fatal because of bad data. Am I following this correctly?

mikecdavis · 2019-06-27T17:23:17Z

android-sdk/src/main/java/com/optimizely/ab/android/sdk/OptimizelyManager.java

            builder.withUserProfileService(userProfileService);
        }

+        if (datafileHandler != null) {


This is never null.

mikecdavis · 2019-06-27T17:28:19Z

android-sdk/src/main/java/com/optimizely/ab/android/sdk/OptimizelyManager.java

+        if (datafileHandler != null) {
+            if (datafileHandler instanceof DefaultDatafileHandler) {
+                DefaultDatafileHandler handler = (DefaultDatafileHandler)datafileHandler;
+                handler.setDatafile(datafile);


Shouldn't these defaults be part of the OptimizelyManager.Builder and not defaulted here?

aliabbasrizvi · 2019-06-27T17:11:10Z

.travis.yml

    - $HOME/.gradle/wrapper/
 before_install:
-    - yes | sdkmanager "platforms;android-28"
+    - yes | sdkmanager "platforms;android-29"


Isn't it possible to use ANDROID_API variable here?

aliabbasrizvi · 2019-06-27T17:12:39Z

test-app/src/main/java/com/optimizely/ab/android/test_app/SplashScreenActivity.java

               });
+               optimizelyManager.getOptimizely().getNotificationCenter().addNotificationHandler(UpdateConfigNotification.class, new NotificationHandler<UpdateConfigNotification>() {
+                   @Override
+                   public void handle(UpdateConfigNotification message) {


nit. message is not the right choice of variable name I feel. Perhaps updateNotification.

this is the test-app and it was what the notification defaults to. the change you are requesting would be at the java level.

yeah NotificationManager<T> is a generic class with handle(T message) as the signature.

aliabbasrizvi · 2019-06-27T17:14:27Z

test-app/src/main/java/com/optimizely/ab/android/test_app/SplashScreenActivity.java

+               optimizelyManager.getOptimizely().getNotificationCenter().addNotificationHandler(UpdateConfigNotification.class, new NotificationHandler<UpdateConfigNotification>() {
+                   @Override
+                   public void handle(UpdateConfigNotification message) {
+                        System.out.println("got datafile change ");


nit. Extraneous space at end.

aliabbasrizvi · 2019-06-27T17:16:10Z

android-sdk/src/androidTest/java/com/optimizely/ab/android/sdk/OptimizelyClientTest.java

        attributes.put(BUCKETING_ATTRIBUTE, bucketingId);
        Variation v = optimizelyClient.activate(FEATURE_ANDROID_EXPERIMENT_KEY, GENERIC_USER_ID, attributes);
-        verify(bucketer).bucket( experiment, bucketingId);
+        verify(bucketer).bucket( experiment, bucketingId, optimizely.getProjectConfig());


nit. Remove space between ( and experiment.

aliabbasrizvi · 2019-06-27T17:31:03Z

...handler/src/main/java/com/optimizely/ab/android/datafile_handler/DefaultDatafileHandler.java

 * The default implementation of {@link DatafileHandler} and the main
 * interaction point to the datafile-handler module.
 */
 public class DefaultDatafileHandler implements DatafileHandler {


Can we not remove reliance on DefaultDatafileHandler and move to an implementation of ProjectConfigManager

This is more for backward compatibility. We also have issues where mobile developers have wanted more control (i.e. look to see if there is a cached datafile).

I marked DatafileHandler interface as deprecated.

mikecdavis · 2019-06-27T23:55:57Z

datafile-handler/src/main/java/com/optimizely/ab/android/datafile_handler/DatafileHandler.java

 * overridden so that the sdk user can provide a override for the default DatafileHandler.
 */
-public interface DatafileHandler {
+public interface DatafileHandler extends ProjectConfigManager {


I think these changes break backwards compatibility with custom implementations.

…erface.

…default datafile handler. allow old handlers to work the same as before using datafile only. fix tests

mikecdavis

Some superficial recommendations, but I think we should clean up OptimizelyManager#buildOptimizely and leverage the OptimizelyManager#Builder to consolidate the default logic a bit more. As it stands now a customer could get themselves in a funky state since we're implicitly mutating the datafileHandler.

mikecdavis · 2019-07-08T17:15:12Z

android-sdk/src/androidTest/java/com/optimizely/ab/android/sdk/OptimizelyManagerTest.java

+        String datafile = optimizelyManager.getDatafile(InstrumentationRegistry.getTargetContext(), R.raw.datafile);
+        ProjectConfig config = null;
+
+        config = new DatafileProjectConfig.Builder().withDatafile(datafile).build();


Can you put the declaration and initialization on the same line?

ProjectConfig config = new DatafileProjectConfig.Builder().withDatafile(datafile).build();

mikecdavis · 2019-07-08T17:17:54Z

android-sdk/src/androidTest/java/com/optimizely/ab/android/sdk/OptimizelyManagerTest.java

        Context appContext = mock(Context.class);
        when(context.getApplicationContext()).thenReturn(appContext);
        when(appContext.getPackageName()).thenReturn("com.optly");
+        when(defaultDatafileHandler.getConfig()).thenReturn(null);


Surprised this mock works since we already mocked with:

when(defaultDatafileHandler.getConfig()).thenReturn(config);

in setup(). Should we reset(defaultDatafileHandler) first?

the when override works fine. these are unit tests. however, it might also be possible to just include the default datafile handler instead of a mock and achieve the same thing.

mikecdavis · 2019-07-08T17:26:41Z

android-sdk/src/main/java/com/optimizely/ab/android/sdk/OptimizelyManager.java

    }

+    private void startDatafileHandler(Context context) {
+        if (datafileDownloadInterval > 0) {


nit, can we add logging? we have some silent ignores that might be confusing to debug. Stylistically as well I prefer we fail fast and return and minimize nesting whenever possible:

if (datafileDownloadInterval <= 0) { logger.debug("Invalid download interval, ignoring background updates."); return; } datafileHandler.startBackgroundUpdates(context, datafileConfig, datafileDownloadInterval, datafile1 -> { NotificationCenter notificationCenter = getOptimizely().getNotificationCenter(); if (notificationCenter == null) { logger.debug("NotificationCenter null, not sending notification"); return; } notificationCenter.send(new UpdateConfigNotification()); });

mikecdavis · 2019-07-08T17:55:41Z

android-sdk/src/main/java/com/optimizely/ab/android/sdk/OptimizelyManager.java

-                .withClientEngine(clientEngine)
+        Optimizely.Builder builder = Optimizely.builder();
+
+        if (datafileHandler instanceof DefaultDatafileHandler) {


I think this logic should be part of the OptimizelyManager.build() and not this method. This would make this much cleaner.

At build time the developer has not called an initialize/start. It was decided that there would be explicit ways to initialize sync and async. So, moving it would break backward compatibility. However, this does bring up the point on whether we should also be be deprecating the initialize methods.

mikecdavis · 2019-07-08T17:57:06Z

datafile-handler/src/main/java/com/optimizely/ab/android/datafile_handler/DatafileLoader.java

                // we're done here. meaning, we have notified you of either the cache coming in or of a new file.
                // so, we are notifying you that the data file service has stopped.
-                datafileLoadedListener.onStop(datafileService.getApplicationContext());
+                //datafileLoadedListener.onStop(datafileService.getApplicationContext());


Can remove this line.

mikecdavis · 2019-07-08T17:57:37Z

...handler/src/main/java/com/optimizely/ab/android/datafile_handler/DefaultDatafileHandler.java

 */
-public class DefaultDatafileHandler implements DatafileHandler {
+public class DefaultDatafileHandler implements DatafileHandler, ProjectConfigManager {
+    private static final Logger logger = LoggerFactory.getLogger("DefaultDatafileHandler");


DefaultDatafileHandler.class as opposed to "DefaultDatafileHandler"

datafile-handler/src/main/java/com/optimizely/ab/android/datafile_handler/DatafileHandler.java

thomaszurkan-optimizely · 2019-07-09T18:03:02Z

...handler/src/main/java/com/optimizely/ab/android/datafile_handler/DefaultDatafileHandler.java

-    public void startBackgroundUpdates(Context context, DatafileConfig datafileConfig, Long updateInterval) {
+    public void startBackgroundUpdates(Context context, DatafileConfig datafileConfig, Long updateInterval, DatafileLoadedListener listener) {
        // if already running, stop it
        stopBackgroundUpdates(context, datafileConfig);


stop background updates clears out the previous update if there was any. It also stops the file observer and sets it to null.

aliabbasrizvi · 2019-07-11T18:02:54Z

...oid-sdk/src/androidTest/java/com/optimizely/ab/android/sdk/OptimizelyManagerBuilderTest.java

 import com.optimizely.ab.OptimizelyRuntimeException;
 import com.optimizely.ab.android.datafile_handler.DefaultDatafileHandler;
 import com.optimizely.ab.android.event_handler.DefaultEventHandler;
+import com.optimizely.ab.android.shared.DatafileConfig;


This import seems unnecessary.

aliabbasrizvi · 2019-07-11T18:04:33Z

android-sdk/src/androidTest/java/com/optimizely/ab/android/sdk/OptimizelyManagerTest.java

    public void initializeIntUseForcedVariation() {
        optimizelyManager.initialize(InstrumentationRegistry.getTargetContext(), R.raw.datafile);

+


Seems extraneous. Remove.

first attempt at using the project config manager with android

e729d75

thomaszurkan-optimizely requested review from aliabbasrizvi and mikecdavis June 26, 2019 00:29

thomaszurkan-optimizely added 2 commits June 26, 2019 12:09

add datafile change notification

61e7459

fix unit tests

d7aaa17

thomaszurkan-optimizely changed the title ~~(chore): first attempt at using the project config manager with android~~ (chore): Use the project config manager in java sdk with android. Also add listener. Jun 26, 2019

thomaszurkan-optimizely added 4 commits June 26, 2019 19:35

update to latest build versions

0f58881

trying to fix travis

289dab7

try to get travis working

30258fd

remove audio from emulator start

8034458

mikecdavis reviewed Jun 27, 2019

View reviewed changes

aliabbasrizvi suggested changes Jun 27, 2019

View reviewed changes

thomaszurkan-optimizely added 7 commits June 27, 2019 11:10

incorrect check for currentConfig

27d3202

fix travis yaml

cccaf11

use 3.2.0 java-sdk

eb6bba3

fall back to tools 28.0.3

a295664

cleanup travis yaml

c619ce6

trying to fix travis build

253ba75

respond to comments, slight refactor.

07db97b

mikecdavis reviewed Jun 27, 2019

View reviewed changes

thomaszurkan-optimizely added 4 commits July 1, 2019 16:45

use datafileloaded as datafile change. deprecate datafile handler int…

9999cd7

…erface.

one more slight refactor. remove datafile config from constructor of …

802c514

…default datafile handler. allow old handlers to work the same as before using datafile only. fix tests

slight refactor for setConfig

2ae390e

cleanup test-app a little

fe2d2d9

thomaszurkan-optimizely closed this Jul 3, 2019

thomaszurkan-optimizely reopened this Jul 3, 2019

mikecdavis suggested changes Jul 8, 2019

View reviewed changes

cleanup from PR comments

eaab9a4

thomaszurkan-optimizely commented Jul 9, 2019

View reviewed changes

mikecdavis approved these changes Jul 10, 2019

View reviewed changes

mnoman09 and others added 2 commits July 11, 2019 09:58

removed not from null datafile or empty datafile check (#279)

435681e

add a few more null checks

0847b7b

aliabbasrizvi approved these changes Jul 11, 2019

View reviewed changes

thomaszurkan-optimizely merged commit 3d0fe42 into master Jul 11, 2019

		@@ -0,0 +1,5 @@
		package com.optimizely.ab.android.datafile_handler;

		public interface DatafileChangeListener {

		public void initializeIntUseForcedVariation() {
		optimizelyManager.initialize(InstrumentationRegistry.getTargetContext(), R.raw.datafile);

(chore): Use the project config manager in java sdk with android. Also add listener. #276

(chore): Use the project config manager in java sdk with android. Also add listener. #276

Uh oh!

Conversation

thomaszurkan-optimizely commented Jun 26, 2019

Summary

Test plan

Uh oh!

mikecdavis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thomaszurkan-optimizely Jun 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikecdavis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thomaszurkan-optimizely Jun 27, 2019 •

edited

Loading