Migrate contacts backup job to new background job manager API #5482
Conversation
|
IT test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/12705-IT |
ezaquarii
changed the title
|
Wow, this is big, a bit too big… ;-) Let me first answer your post New abstraction looks very very good! Lombok: Testing: Migration: EDIT: I will start reviewing it after rebasing |
ezaquarii
force-pushed
the
ezaquarii/migrate-contacts-backup-job-to-worker-api
branch
3 times, most recently
from
792ef79 to
12105e9
Compare
|
Screenshot test failed, but no output was generated. Maybe a preliminary stage failed. |
ezaquarii
force-pushed
the
ezaquarii/migrate-contacts-backup-job-to-worker-api
branch
2 times, most recently
from
0dbbb33 to
6ae080f
Compare
Codecov Report
@@ Coverage Diff @@
## master #5482 +/- ##
=========================================
Coverage ? 21.85%
Complexity ? 3
=========================================
Files ? 405
Lines ? 33794
Branches ? 4741
=========================================
Hits ? 7386
Misses ? 25217
Partials ? 1191
|
|
IT test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/12719-IT |
ezaquarii
force-pushed
the
ezaquarii/migrate-contacts-backup-job-to-worker-api
branch
from
6ae080f to
17b23b6
Compare
|
Screenshot test failed, but no output was generated. Maybe a preliminary stage failed. |
ezaquarii
force-pushed
the
ezaquarii/migrate-contacts-backup-job-to-worker-api
branch
2 times, most recently
from
04501fb to
0dc6ef2
Compare
|
@tobiasKaminsky I fixed lint and rebased. Robolectric is still enabled in this branch until I finish the implementation. I'll remove it at later stage. |
|
Those strings are visible in ETM screen. I think ETM is disabled in release builds (f-droid and gplay). ETM is mostly for developersand maybe Dev build users.
…On February 18, 2020 8:08:40 PM UTC, Andy Scherzinger ***@***.***> wrote:
AndyScherzinger commented on this pull request.
> @@ -887,6 +887,17 @@
<string name="etm_title">Engineering Test Mode</string>
<string name="etm_accounts">Accounts</string>
<string name="etm_preferences">Preferences</string>
+ <string name="etm_background_jobs">Background jobs</string>
+ <string name="etm_background_jobs_cancel_all">Cancel all
jobs</string>
+ <string name="etm_background_jobs_prune">Prune inactive
jobs</string>
+ <string name="etm_background_jobs_start_test_job">Start test
job</string>
@ezaquarii would they be visible somewhere in any build? (like the
nightly aka dev build which is published?)
--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#5482 (comment)
|
|
We should be fine with adding new strings imho. They get pushed to transifex and for the languages that are very actively maintained the translations come in quite quickly. |
|
I know. I was worried that it incurs extra cost and since those strings are development only, that might not be desired thing.
|
ezaquarii
force-pushed
the
ezaquarii/migrate-contacts-backup-job-to-worker-api
branch
from
0dc6ef2 to
69a0ecb
Compare
|
APK file: https://www.kaminsky.me/nc-dev/android-artifacts/13072.apk |
Codacy705Lint
SpotBugs (new)
SpotBugs (master)
|
|
IT test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/13072-IT |
|
APK file: https://www.kaminsky.me/nc-dev/android-artifacts/13073.apk |
Codacy705Lint
SpotBugs (new)
SpotBugs (master)
|
|
IT test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/13073-IT |
|
APK file: https://www.kaminsky.me/nc-dev/android-artifacts/13074.apk |
Codacy705Lint
SpotBugs (new)
SpotBugs (master)
|
|
Screenshot test failed, but no output was generated. Maybe a preliminary stage failed. |
Signed-off-by: Chris Narkiewicz <hello@ezaquarii.com>
ezaquarii
force-pushed
the
ezaquarii/migrate-contacts-backup-job-to-worker-api
branch
from
7bf8b23 to
500fdb9
Compare
|
Complexity decreasing per file
==============================
+ src/main/java/com/owncloud/android/jobs/NCJobCreator.java -1
See the complete overview on Codacy |
|
APK file: https://www.kaminsky.me/nc-dev/android-artifacts/13075.apk |
Codacy705Lint
SpotBugs (new)
SpotBugs (master)
|
|
Screenshot test failed, but no output was generated. Maybe a preliminary stage failed. |
|
APK file: https://www.kaminsky.me/nc-dev/android-artifacts/13077.apk |
Codacy705Lint
SpotBugs (new)
SpotBugs (master)
|
|
Screenshot test failed, but no output was generated. Maybe a preliminary stage failed. |
|
@tobiasKaminsky This is ready for final review. Screenshot tests are failing. The trouble is that those tests are failing 100% if time and I'm pretty sure it's not my code. I know the flakiness is a challenge. Can we revisit this solution and think about some kind of compromise that won't impect the daily development process? |
|
APK file: https://www.kaminsky.me/nc-dev/android-artifacts/13097.apk |
Codacy375Lint
SpotBugs (new)
SpotBugs (master)
|
|
IT test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/13097-IT |
|
Wow! |
delete-merged-branch
bot
deleted the
ezaquarii/migrate-contacts-backup-job-to-worker-api
branch
|
The trash bin should be tinted, but I merged it so that I do not delay further work :-) |
@tobiasKaminsky @AndyScherzinger @mario This is still WIP but a review would be handy nevertheless.
This is rather big PR containing contacts backup job migration to new
androidx.work.WorkerManager. To migrate it, I had to implement some missing infrastructure, hence the scope of this PR is much larger than I intitially anticipated. To make matters more interesting, it will get bigger (see last 2 sections).New abstraction over
WorkManagerBackgroundJobManageris our application-specific abstraction, exposing business-specific API to control running jobs. It hides all job handling machinery behind a very goal-oriented, verb-based API.Important difference between old job manager API is that all
WorkManagerAPI is asynchronous. Exposed API is based on JavaFutureprimitive andLiveData<T>. New semantics make some operations more tricky and porting of previously synchronous code needs some thinking, but no problems has been spotted so far.One limitation of
WorkManagerAPI is that it does not provide elegant way of storing job metadata - like the previously usedcom.evernote.android.job.JobRequest.getExtras()- and provides onlylimited API in form ofSet<String> WorkInfo.getTags(). The missing functionality has been implemented in asJobInfoabstraction that replacesWorkInfo.JobInfois created by parsing tags stored inWorkInfo. Type translation is handled by theBackgroundJobManagerImpland those complexities are not visible for the end user of our API.Observable collection of all jobs is exposed using
LiveData<T>interface, so it should play nice with the UI code.ETM Screen
Background Jobs
Viewing current background job status is quite important do debug any potential issues, so I added
EtmBackgroundJobsFragmentwith a list of all registered jobs and handy menu where we can dump development actions. It currently allows us to cancel all tasks and toggleTestJobthat demonstrates progress indicator and is used to experiment with platform API.Migrations
This PR provies a small screen with migrations status. It lists migrations and allows to clear migrations status. This function can be very useful when creating new migrations.
BackupContactsJobOld code has been copied almost verbatim from old job class, with some minor tweaks around dependencies and few cleanups. There is no unit test for it, but it seems to work ok.
ContentObserverWorkYikes! We have a bug in our current manager implementation and the observer job is added to the queue each time the app starts, which means that users are accumulating those jobs in their queues. This is not a big deal, but might eat up resources. Since the
ContentObserverWorkonly starts legacy job implementation, the damange is contained.This issue is addressed and the observer job is started only once.
Old job is cleared.
User.nameEqualsExtended user API with account name equality check, as we need to do a log of
Stringcomparisions.Missing things
masterMaybe refactorContactsBackupWorkto enable testingRemove legacyContactsBackupJobclassDesign migration moduleSee Migrations manager #5546Decide how to handle failing migrationsSee Migrations manager #5546Add migration module to DI and enable it inSee Migrations manager #5546MainApp