Skip to content

Conversation

@CDRussell
Copy link
Member

Task/Issue URL: https://app.asana.com/0/488551667048375/1147610969828115
Tech Design URL:
CC:

Description:
Migrates the app configuration sync to WorkManager and stops using JobScheduler APIs directly for it.

Steps to test this PR:

  1. Checkout develop, and modify the times in JobBuilder to use ridiculously short times for periodic and backoffCriteria (like, a few milliseconds kind of short)
  2. Install and launch. ensure the initial syncs finishes, so that a Job is scheduled with JobScheduler
  3. Checkout this branch. Similar to before, set short values for periodic and backoff times in AppConfigurationSyncWorkRequestBuilder
  4. Install and launch. Verify initial sync still happens when app first launches
  5. Verify after app sync finishes, work is scheduled with WorkManager.
  6. Wait for sync to happen again. Depending on the OS, this might take ~15mins ☕☕. Now is a good time to get Charles ready. We want to test failure path and success path, so you can set a breakpoint on one of the sync requests in Charles.
  7. When the sync happens, kill a request in Charles to produce a failed sync. Verify it tries again, as per the retry policy
  8. Let all requests succeed, and verify sync completes successfully

Internal references:

Software Engineering Expectations
Technical Design Template

@malmstein
Copy link
Contributor

@CDRussell develop has been updated after this PR was opened and caused DB migration issues. I've just merged and pushed so we can continue the review and the steps to test.

return true
Timber.i("Deprecated AppConfigurationJobService running. Unscheduling future syncs using this job")
jobScheduler.cancel(LEGACY_APP_CONFIGURATION_JOB_ID)
return false
Copy link
Contributor

Choose a reason for hiding this comment

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

this is just here because we want to tell JobScheduler that we are done

@malmstein
Copy link
Contributor

We've had a call to discuss all the changes and run through the steps to validate this is working as expected.

Copy link
Contributor

@malmstein malmstein left a comment

Choose a reason for hiding this comment

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

We've had a call to discuss all the changes and run through the steps to validate this is working as expected.

@CDRussell CDRussell merged commit 084e859 into develop May 14, 2020
@CDRussell CDRussell deleted the feature/craig/migrate_sync_to_work_manager branch May 14, 2020 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants