fix: update to latest build tools. slight refactor of datafile service. #335

thomaszurkan-optimizely · 2020-06-12T22:04:25Z

Summary

Eliminate piling up of datafile calls. Use latest tools.

Test plan

Lots of manual testing including taking the device offline for long periods of time.

jaeopt

A great catch!
I suggested to add one more sync for safety.
Also we need test cases to cover the changes.

jaeopt · 2020-06-12T23:19:43Z

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

@@ -228,7 +228,7 @@ public OptimizelyClient initialize(@NonNull Context context, @Nullable String da
            logger.error("Unable to build OptimizelyClient instance", e);
        }

-        if (downloadToCache) {
+        if (downloadToCache && !datafileDownloadEnabled()) {


Can we remove this extra flag check "datafileDownloadEnabled"?
Clients still want to see datafile immediately updated even when they turn on polling interval (15min+).
I see initial background fetch and periodic polling won't be called at the same time because of the big interval.

I think you found why we have a potential conflict in "startService" and setting the minLetency to one minute will fix it.

No. This causes a double download. It makes sense to have this here. Please use the test-app with master and you can easily see the double download.

I observed the double downloads. It looks like un-deterministic behavior of JobScheduler - polling download can be triggered any time between 0..15mins and in simulator, it's fired instantly (I think this will behave differently in real devices).

This code fix will remove the double download issue when app starts.
But in real devices, if clients expect immediate download with sync-initialization, it may behave differently (like 15min delayed) and can bring back the initialization issue we fixed before.

Can we find alt solution still supporting instant download even when polling enabled?

I think I don't see any issue here. Agree with Tom.

jaeopt · 2020-06-12T23:20:41Z

...le-handler/src/main/java/com/optimizely/ab/android/datafile_handler/DatafileRescheduler.java

@@ -95,11 +96,11 @@ void dispatch(Intent intent) {
            for (DatafileConfig datafileConfig : datafileConfigs) {
                // for scheduled jobs Android O and above, we use the JobScheduler and persistent periodic jobs
                // so, we don't need to do anything.
-               // if (android.os.Build.VERSION.SDK_INT < Build.VERSION_CODES.O) {


Remove this comment line

that's what the red means.

jaeopt · 2020-06-12T23:32:06Z

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

@@ -139,7 +140,7 @@ public void startBackgroundUpdates(Context context, DatafileConfig datafileConfi
        enableUpdateConfigOnNewDatafile(context, datafileConfig, listener);
    }

-    public void enableUpdateConfigOnNewDatafile(Context context, DatafileConfig datafileConfig, DatafileLoadedListener listener) {
+    synchronized public void enableUpdateConfigOnNewDatafile(Context context, DatafileConfig datafileConfig, DatafileLoadedListener listener) {


This sync looks good.
For safety, let's add a synchronizer for event handler as well (line 160-173).

No. Not relevant to this PR.

It's good to have sync on event handler but we already have sync on this method and disable. So chances for cross-thread is very low on event handler.

jaeopt · 2020-06-12T23:57:41Z

shared/src/main/java/com/optimizely/ab/android/shared/ServiceScheduler.java

@@ -109,7 +108,11 @@ private void setRepeating(long interval, PendingIntent pendingIntent, Intent int
                            ScheduledJobService.class.getName()));
            builder.setPeriodic(interval, interval);
            builder.setPersisted(true);
-            builder.setRequiredNetworkType(JobInfo.NETWORK_TYPE_ANY);


I think this is the one you mentioned in the slack - removed to avoid accumulated calls when network is back up.
Is this a bug in new Android OS or a new feature?

It has an unwanted side effect of just queuing up jobs rather than not running them. So, when a connection becomes available, it runs all queued.

jaeopt

I still have concern about changing init download timing. See my comments.
Also a question is how to test the cases we try to fix here:

it does not break when multiple downloads happen simultaneously
when network is down long time, it won't accumulate the request and cause out-of-memory issues.

It'll be great if we can invent good test cases for (2), which must be tough. We can add tests for (1).

jaeopt · 2020-06-16T01:31:09Z

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

@@ -228,7 +228,7 @@ public OptimizelyClient initialize(@NonNull Context context, @Nullable String da
            logger.error("Unable to build OptimizelyClient instance", e);
        }

-        if (downloadToCache) {
+        if (downloadToCache && !datafileDownloadEnabled()) {


I observed the double downloads. It looks like un-deterministic behavior of JobScheduler - polling download can be triggered any time between 0..15mins and in simulator, it's fired instantly (I think this will behave differently in real devices).

This code fix will remove the double download issue when app starts.
But in real devices, if clients expect immediate download with sync-initialization, it may behave differently (like 15min delayed) and can bring back the initialization issue we fixed before.

Can we find alt solution still supporting instant download even when polling enabled?

mnoman09 · 2020-06-16T19:13:23Z

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

+    @Test
+    public void initializeSyncWithUpdateOnNewDatafileDisabledWithPeriodicPollingDisabled() {
+        boolean downloadToCache = true;
+        boolean updateConfigOnNewDatafiel = false;


Nit: updateConfigOnNewDatafile

Suggested change

boolean updateConfigOnNewDatafiel = false;

boolean updateConfigOnNewDatafile = false;

mnoman09 · 2020-06-16T19:15:15Z

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

+                    }
+                }).when(manager.getDatafileHandler()).downloadDatafile(any(Context.class), any(DatafileConfig.class), any(DatafileLoadedListener.class));
+
+        OptimizelyClient client = manager.initialize(context, defaultDatafile, downloadToCache, updateConfigOnNewDatafiel);


Suggested change

OptimizelyClient client = manager.initialize(context, defaultDatafile, downloadToCache, updateConfigOnNewDatafiel);

OptimizelyClient client = manager.initialize(context, defaultDatafile, downloadToCache, updateConfigOnNewDatafile);

mnoman09 · 2020-06-16T20:01:28Z

shared/src/main/java/com/optimizely/ab/android/shared/ServiceScheduler.java

+            if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.P) {
+                builder.setPrefetch(true);
+            }
+            //builder.setRequiredNetworkType(JobInfo.NETWORK_TYPE_ANY);


Nit: remove?
Is there any reason to remove this line?

jaeopt

It looks good!
One small change and a new test case suggested

jaeopt · 2020-06-16T23:34:23Z

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

        // Execute tasks in order
        requestDatafileFromClientTask.executeOnExecutor(executor);
+        // set the download time and don't allow downloads to overlap less than a minute
+        saveDownloadTime(datafileUrl);


Let's move this to line 95 to maximize filtering.

I really don't see a reason to move this. Can you explain?

jaeopt · 2020-06-16T23:57:15Z

...dler/src/androidTest/java/com/optimizely/ab/android/datafile_handler/DatafileLoaderTest.java

@@ -170,4 +170,29 @@ public void warningsAreLogged() throws IOException {
        verify(logger).warn("Unable to save new datafile");
        verify(datafileLoadedListener, atMost(1)).onDatafileLoaded("{}");
    }
+
+    @Test
+    public void debugLogged() throws IOException {


Can we add one more test to check if back-to-back download works fine for both requests when they're apart 61secs?

I don't want to add another an extra minute to the tests to prove this point.

Yep it's not great to add a minute wait time for testing. But this test must be important to make sure we do not freeze polling permanently by error. We can change it to inject the required gap time so can test with a reasonable wait time of a few seconds.
I'm ok to release it as is for now and then add the extra test with the next coming release.

jaeopt · 2020-06-17T00:10:41Z

...dler/src/androidTest/java/com/optimizely/ab/android/datafile_handler/DatafileLoaderTest.java

+        }
+
+        verify(logger).debug("Last download happened under 1 minute ago. Throttled to be at least 1 minute apart.");
+        verify(datafileLoadedListener, atMost(2)).onDatafileLoaded("{}");


Let's also check if it's called at least once

Suggested change

verify(datafileLoadedListener, atMost(2)).onDatafileLoaded("{}");

verify(datafileLoadedListener, atMost(2)).onDatafileLoaded("{}");

verify(datafileLoadedListener, atLeast(1)).onDatafileLoaded("{}");

jaeopt

LGTM

…ged min via reflection.

I updated after your remarks. thanks! releasing now.

retool the datafile service.

fe953f7

thomaszurkan-optimizely requested a review from a team as a code owner June 12, 2020 22:04

optimizely-codereview-bot assigned thomaszurkan-optimizely Jun 12, 2020

thomaszurkan-optimizely requested a review from jaeopt June 12, 2020 22:04

fix tests

7665539

jaeopt requested changes Jun 12, 2020

View reviewed changes

thomaszurkan-optimizely added 4 commits June 15, 2020 10:23

update travis build tools

38dcfa4

change api as well as build tools

be4affd

cleanup gradle files

030b70c

remove unused import

1672dd9

jaeopt requested changes Jun 16, 2020

View reviewed changes

mnoman09 previously requested changes Jun 16, 2020

View reviewed changes

refactor to not allow more than one datafile download within a minute

31268df

jaeopt requested changes Jun 17, 2020

View reviewed changes

update to add another test for double download

0d98dbb

thomaszurkan-optimizely requested review from mnoman09 and jaeopt June 18, 2020 02:22

jaeopt approved these changes Jun 18, 2020

View reviewed changes

thomaszurkan-optimizely added 3 commits June 18, 2020 11:32

added a test for when two downloads come in longer than minimum. chan…

a41cba4

…ged min via reflection.

add a unit test for threading in datafile download

0a53320

remove unused imports

bfff4a0

thomaszurkan-optimizely merged commit a00a540 into master Jun 18, 2020

thomaszurkan-optimizely deleted the updateToLatestBuildTools branch June 18, 2020 20:35

	boolean updateConfigOnNewDatafiel = false;
	boolean updateConfigOnNewDatafile = false;

	OptimizelyClient client = manager.initialize(context, defaultDatafile, downloadToCache, updateConfigOnNewDatafiel);
	OptimizelyClient client = manager.initialize(context, defaultDatafile, downloadToCache, updateConfigOnNewDatafile);

	verify(datafileLoadedListener, atMost(2)).onDatafileLoaded("{}");
	verify(datafileLoadedListener, atMost(2)).onDatafileLoaded("{}");
	verify(datafileLoadedListener, atLeast(1)).onDatafileLoaded("{}");

fix: update to latest build tools. slight refactor of datafile service. #335

fix: update to latest build tools. slight refactor of datafile service. #335

Uh oh!

Conversation

thomaszurkan-optimizely commented Jun 12, 2020

Summary

Test plan

Uh oh!

jaeopt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thomaszurkan-optimizely Jun 15, 2020 • 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!

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 15, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

jaeopt 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!

jaeopt 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!

jaeopt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thomaszurkan-optimizely Jun 15, 2020 •

edited

Loading

thomaszurkan-optimizely Jun 15, 2020 •

edited

Loading