Adds lazy donwnload for multimedia for App Updates #2243
Conversation
|
@ctsims Made the discussed changes in last few commits. |
ctsims
left a comment
ctsims
left a comment
There was a problem hiding this comment.
Hi Shubham,
Thanks for processing through the performance stuff. I reviewed the new commits and had a couple of questions and one or two suggestions.
In particular I'd like to understand why the model needed to be changed to support a worker per app version #, since that can have pretty significant runaway effects with multiple workers all reading/writing over similar tables / files at the same time.
| Logger.log(LogTypes.TYPE_MAINTENANCE, "An error occured while lazy downloading a media resource : " + it.message) | ||
| return handleRecoverResourceFailure(it).data | ||
| } | ||
| if (!HiddenPreferences.hasLazyMediaDownloaded()) { |
There was a problem hiding this comment.
I'm not sure why we'd ever been running lazy media installers for multiple app versions. Are we lazy installing the upgrade table as well as the installed version? or is this due to some behavior that happens after handing over between updates being applied??
There was a problem hiding this comment.
Only A new app update can introduce new lazy resources, so once we have successfully run this task for version x, we don't need to run it again for the same version until user updates to a different version.
Retrospectively, a better change here will be to just maintain a single app pref across all versions and reset that flag after an app update.
There was a problem hiding this comment.
Ah, got it. I was reviewing by commit, which was silly, and I missed this was old. Sorry for the noise.
| } | ||
| return Result.failure() | ||
| val result = MissingMediaDownloadHelper.downloadAllLazyMedia() | ||
| return if (result == AppInstallStatus.Installed) Result.success() else Result.retry() |
There was a problem hiding this comment.
Don't we only want to retry automatically on specific errors?
In the handling list above, 3 of the 4 errors wouldn't be good for us to run a retry on
ctsims
left a comment
ctsims
left a comment
There was a problem hiding this comment.
No feedback on the latest round, so unless I'm missing anything this should be good to go.
|
@ctsims thanks. Also want to make sure that you have had a chance to go through the cross-requested core PR. |
…show the toast when the button is visible
|
Made one more functional change to only show the audio missing toast when the audio button is visible. |
|
@ctsims I just realized that we don't actually rate limit media requests but only the profile files which is the starting point for an update. Therefore I am worried that these lazy mm requests won't get rate limited at all slowing down other parts of the system. Is it worth adding a flag to these request saying that these are not part of a blocking update and hence it's fine to rate limit these. |
|
@shubham1g5 I think that's a really good idea, and we can avoid setting that flag for Media that's actually on the screen when the user makes the direct request to download a file |
Spec: https://docs.google.com/document/d/1WWhgG6cyKm7nN2pUn8lty72F3ZN6D1xWqTZsIlHX4z8/edit#heading=h.xlb7kbytn7rw
resource definitionMissingMediaDownloadWorkeron login to initiate lazy media download. This task today takes help of our existing multimedia verification to find out the lazy resources and installs them one by one in backgroundcross-request: dimagi/commcare-core#899