This differential schedules blob deletion on background thread when large notification (notification containing metadata to an item on blob service) arrives. Deletion scheduling is implemented using WorkManager framework - native Android
library that handles persistence, optimal scheduling and retry policy of background tasks out of the box.
Details
Details
- Launch blob service locally
- Build android app substituting blob service URL in relevant places
- Send large notification (ex. 7000 'x' characters) to the device.
- Observe in blob service terminal that 30s after logging GET request, DELETE request is logged and reported successfull.
- Build the app again but change the line 38 of DeleteBlobWork.java to sth like "blobhash" (incorrect JSON structure).
- Send large notification (ex. 7000 'x' characters) to the device.
- Observe in blob service terminal that 30s after logging GET request, DELETE request is logged and reported unsuccessfull.
- Finally observe that DELETE request is retried several time - each time after longer period of time but don't wait for the final 10 retry since it will take 17 hours (retries are scheduled according to exponential backoff)
Additionally:
- Add line that logs blob holder when deletion is scheduled.
- Deploy the app to two different Android devices and send large notification.
- Open the app on each device and using console app ensure that each of them logs different holder.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- marcin/eng-4247
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| native/android/app/src/main/java/app/comm/android/notifications/CommAndroidNotificationsBlobServiceClient.java | ||
|---|---|---|
| 35 ↗ | (On Diff #30707) | I think it is nice not to have private and public variables mixed in subsequent lines so I moved this line here. |
| 95 ↗ | (On Diff #30707) | This enables us to schedule blob deletion upfront as soon as notification arrives. GET request already has time limit of 15 seconds so we can safely schedule blob deletion with 15 seconds delay as soon as notif arrives. |
| native/android/app/src/main/java/app/comm/android/notifications/DeleteBlobWork.java | ||
| 18 ↗ | (On Diff #30707) | According to: https://stackoverflow.com/questions/58433063/android-worker-class-as-nested-class it is impossible to have this class as private class in CommAndroidNotificationsBlobServiceClient without some serious workaround. However it still needs access to shared instance of OkHttpClient so it was made public. |
| 20 ↗ | (On Diff #30707) | This will take apprx. 17 hours to reach max number of retries assuming that each attempt fails: https://developer.android.com/guide/background/persistent/getting-started/define-work#retries_backoff |
| 22 ↗ | (On Diff #30707) | We can't pass OkHttpClient as parameters since it only accepts easily serializable types: https://developer.android.com/reference/androidx/work/WorkerParameters |
| 70 ↗ | (On Diff #30707) | This call is synchronous since all WorkManager tasks are executed on background threads. |
Comment Actions
Looks good!
I'm wondering if the new deps change the bundle size significantly.
| native/android/app/build.gradle | ||
|---|---|---|
| 713–714 | It isn't specified as optional in the docs, but I'm wondering if we need it | |
| native/android/app/src/main/java/app/comm/android/notifications/CommAndroidNotificationsBlobServiceClient.java | ||
| 35 ↗ | (On Diff #30707) | We can consider keeping it private and exposing a newCall method of it as a new method of CommAndroidNotificationsBlobServiceClient. Then in DeleteBlobWork we could simply call Response response =
CommAndroidNotificationsBlobServiceClient.newCall(request)
.execute(); |
| 95 ↗ | (On Diff #30707) | Can we instead schedule without a delay just after the download completes? Which option is better? |
| native/android/app/build.gradle | ||
|---|---|---|
| 713–714 | I will check. | |
| native/android/app/src/main/java/app/comm/android/notifications/CommAndroidNotificationsBlobServiceClient.java | ||
| 95 ↗ | (On Diff #30707) | My idea was that scheduling deletion with delay first is safer since if sth fails during the download part and the app crashes then we don't lose the ability to issue delete requests. It is possible that the crash occurs even before scheduling deletion, so we are not 100% safe - we just increase safety by scheduling deletion upfront. |
Comment Actions
This commit:
So we probably have already had guava in some other dependency since the size in MB didn't change at all. However please correct my if my method of checking the size was wrong.
I checked our app .apk file size twice: first time on a parent commit to this one and second time on this commit. In between I uninstalled the app and executed ./gradlew clean. My results are below:
Parent commit:
This commit:
So we probably have already had guava in some other dependency since the size in MB didn't change at all. However please correct my if my method of checking the size was wrong.
Comment Actions
When creating a blob, we're setting one holder for the content. Then we send the same hash and holder to multiple recipients. After the processing, the recipient schedules blob deletion which means that other recipients may be unable to download the blob - as it is already deleted.
Shouldn't we have a separate holder for each recipient?