Issue #4879: Add service for running migration in background. #5142
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5142 +/- ##
===========================================
- Coverage 82.97% 80.1% -2.87%
+ Complexity 5596 3839 -1757
===========================================
Files 534 487 -47
Lines 24331 17153 -7178
Branches 3629 2529 -1100
===========================================
- Hits 20189 13741 -6448
+ Misses 2771 2386 -385
+ Partials 1371 1026 -345
Continue to review full report at Codecov.
|
| stopSelf() | ||
| } | ||
|
|
||
| private fun ensureChannelExists(): String { |
There was a problem hiding this comment.
I would expect this sort of thing to live elsewhere, but i see that we already have similar ensureNotificationGroupAndChannelExists methods elsewhere already... Anyway, that's cleanup for a later date.
There was a problem hiding this comment.
Yeah, we could think of a helper maybe. But in this case I hope this code will not stick around forever, so I like it to not spread too much.
| override fun onBind(intent: Intent?): IBinder? = null | ||
|
|
||
| private suspend fun migrate() { | ||
| val results = migrator.migrateAsync().await() |
There was a problem hiding this comment.
We'll need to emit a glean ping out of this; is that something we can do in a service? or would we need to communicate w/ the host app? It'd be great to not have to worry about making our results Parcelable...
But I guess we can deal with this later.
There was a problem hiding this comment.
That should just work. We are already using Glean from some other services (e.g. media).
| @@ -236,28 +239,52 @@ class FennecMigrator private constructor( | |||
| * @return A deferred [MigrationResults], describing which migrations were performed and if they succeeded. | |||
| */ | |||
| fun migrateAsync(): Deferred<MigrationResults> = synchronized(migrationLock) { | |||
There was a problem hiding this comment.
this could be internal now, right?
There was a problem hiding this comment.
The Gecko migration piece is still called from the app code directly.
| } | ||
| } | ||
|
|
||
| private fun getMigrationsToRun(): List<VersionedMigration> { |
There was a problem hiding this comment.
I'm a little worried that this is now not protected by the migrationLock; we shouldn't have overlapping migrations running, of course...
There was a problem hiding this comment.
Yeah, overlapping migrations could cause a bunch of other issues.
It's private and still protected by the lock when called from migrateAsync(). I think I'll add the same protection to startMigrationServiceIfNeeded().
There was a problem hiding this comment.
Or maybe not! If a migration is currently running then we do not want to block here - because that can take a while. 🤔
There was a problem hiding this comment.
I think I want to land as-is. Once we have #4872 it will be easier to communicate the current state.
|
bors r=grigoryk |
Build succeeded
|
This is the AC part of the background service. Next step is to open the Fenix PR after this is in the next snapshot.