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

Permits to wait longer for a variant conversion #2092

Merged
merged 15 commits into from
Feb 11, 2025
Merged

Permits to wait longer for a variant conversion #2092

merged 15 commits into from
Feb 11, 2025

Conversation

idlira
Copy link
Contributor

@idlira idlira commented Feb 7, 2025

Description

The main purpose of this PR is to:

  • permit several hard-coded settings such as retry delay and number of attempts to be customized (same values used as before, so the standard config is non-breaking)
  • permits to wait longer for a conversion to finish. Previously we would return a HTTP 503 after around 2s for any successful conversion which could take longer than that. Now we support the /dasd/...?waitLonger=true parameter to makes the server retry a few more times (obviously customizable). Defaulting to 3x the normal amount.

One use case for this ALREADY IMPLEMENTED is the conversion for embedding in PDF (BlobPdfReplaceHandler).

To achieve this, the BlobStorageSpace has been refactored (thus BREAKING), in order to remove a god method which based on a single flag took various complex code paths. I now hope this is more cleaner to understand. By the way, the layer2's README.md of the storage framework has been enhanced with hopefully useful diagrams to better help understand how the whole thing fits together.

Note that the URLBuilder has been enhanced with a waitLonger builder-method in order to create URLs with the extra setting. Also note that this class should be refactored to use the kernel's URLBuilder, but this would require even more refactoring, so maybe another time.

BREAKING changes:

  • BlobStorageSpace

    • OLD: Optional<FileHandle> download(String blobKey, String variant);
    • NEW: Optional<FileHandle> download(String blobKey, String variant, boolean waitLonger);
  • BasicBlobStorageSpace

    • A protected method called tryResolvePhysicalKey has been removed, in case it has been overwritten (very unlikely).

Additional Notes

  • This PR fixes or works on following ticket(s): OX-11848

Checklist

  • Code change has been tested and works locally
  • Code was formatted via IntelliJ and follows SonarLint & best practices

idlira added 12 commits February 6, 2025 13:40
We want to ability to fine how long we want for a conversion. Either for it to start (optimistic lock) and once it has been started (conversion attempts + conversion delay)

Also uses the change to better document the usage of some of these settings

Fixes: OX-11845
Basically splits resolvePhysicalKey into tryFetchPhysicalKey (responsible for lookups) and tryCreatePhysicalKey (which actually converts a variant).

The previous method had a nonblocking parameter which decided between lookup and creation, and was passed down 4 levels below, making understanding of these calls quite complex.

On the way a few redundant methods got killed and JavaDoc updated

Fixes: OX-11845
Some use cases are ready to wait longer and obtain a converted file. Note that we choose to use a boolean here and control the amount of retries server-side, as leaving this to the caller can lead to misusage.

Fixes: OX-11845
We detect the "wish" to wait longer and sets a header in the response, which is then read further down in order to control how many retries we want to do.

Using a header and simply changing the number of retries reduces the number of chained methods that must be changed to propagate the information further down.

Fixes: OX-11845
This is a request not coming via URL, but at API level. In this case the caller must also signalize if a longer wait is desired.

Fixes: OX-11845
Instead of a hard-coded 15 seconds, we calculated the expected conversion time based on the actual number of retries we want to perform and the delay between them, adding the connection time as extra buffer

Fixes: OX-11845
@idlira idlira added 🧬 Enhancement Contains new features 💣 BREAKING CHANGE Contains non-backwards compatible changes to public methods or changes the behavior of existing code 🤯 Complex PR with extensive changes that should be reviewed with extra care labels Feb 7, 2025
Fixes: OX-11845
Copy link
Member

@sabieber sabieber left a comment

Choose a reason for hiding this comment

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

Nice documentation 👌🏻

Fixes: OX-11845
Solving a startup issue with newer Docker Desktop versions in macOS

Fixes: OX-11845
Copy link
Member

@jakobvogel jakobvogel left a comment

Choose a reason for hiding this comment

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

👏

@idlira idlira merged commit 7a1c4d1 into develop Feb 11, 2025
3 checks passed
@idlira idlira deleted the ili/OX-11845 branch February 11, 2025 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💣 BREAKING CHANGE Contains non-backwards compatible changes to public methods or changes the behavior of existing code 🤯 Complex PR with extensive changes that should be reviewed with extra care 🧬 Enhancement Contains new features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants