Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reviewer: migrate requested files immediately #13346

Merged
merged 3 commits into from
Feb 26, 2023

Conversation

david-allison
Copy link
Member

@david-allison david-allison commented Feb 25, 2023

Purpose / Description

During a storage migration, we have some media in an old folder and others in the collection media folder.

Instead of handling the possibility of 2 folders, we immediately migrate requested files when requested by the reviewer/previewers

Fixes

Approach

When an image/sound file is requested, and is:

  • not found
  • inside the collection
  • available for migration

Add a new task to prioritise it in the storage migration

Image migration: needs to be performed in shouldInterceptRequest as WebClient error handlers do not provide enough control to recover from

Sound: performed in the error handler to avoid impacting the fast path

How Has This Been Tested?

API 31/33 emulator.

The migration performance hit is not noticeable in my testing

Learning (optional, can help others)

Using CountDownLatch - constructor is very light

Checklist

  • You have a descriptive commit message with a short title (first line, max 50 chars).
  • You have commented your code, particularly in hard-to-understand areas
  • You have performed a self-review of your own code
  • UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
  • UI Changes: You have tested your change using the Google Accessibility Scanner

The methods for onConnect/disconnect do not need
to have functionality
During a storage migration, we have some media in an old folder
and others in the collection media folder.

Instead of handling the possibility of 2 folders, we immediately
migrate requested files when requested by the reviewer/previewers

When an image/sound file is requested, and is:
* not found
* inside the collection
* available for migration

Add a new task to prioritise it in the storage migration

Much better UX than missing images

Image migration: needs to be performed in `shouldInterceptRequest`
  as WebClient error handlers do not provide enough control to
  recover from

Sound: performed in the error handler to avoid impacting the fast path

The migration performance hit is not noticeable in my testing

Issue 5304
Copy link
Member

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

LGTM

@mikehardy mikehardy added the Needs Second Approval Has one approval, one more approval to merge label Feb 25, 2023
@mikehardy mikehardy merged commit 3f251cf into ankidroid:main Feb 26, 2023
@github-actions github-actions bot added this to the 2.16 release milestone Feb 26, 2023
@github-actions github-actions bot removed Needs Second Approval Has one approval, one more approval to merge Review High Priority Request for high priority review labels Feb 26, 2023
@david-allison david-allison deleted the migrate-images branch February 26, 2023 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

3 participants