-
Notifications
You must be signed in to change notification settings - Fork 7
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
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
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
Fixes: OX-11845
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
Fixes: OX-11845
Fixes: OX-11845
Fixes: OX-11845
Fixes: OX-11845
sabieber
reviewed
Feb 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice documentation 👌🏻
src/main/java/sirius/biz/storage/layer2/BasicBlobStorageSpace.java
Outdated
Show resolved
Hide resolved
src/main/java/sirius/biz/storage/layer2/BasicBlobStorageSpace.java
Outdated
Show resolved
Hide resolved
Fixes: OX-11845
Solving a startup issue with newer Docker Desktop versions in macOS Fixes: OX-11845
jakobvogel
approved these changes
Feb 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏
sabieber
approved these changes
Feb 11, 2025
mkeckmkeck
approved these changes
Feb 11, 2025
mko-sci
approved these changes
Feb 11, 2025
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
The main purpose of this PR is to:
/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
Optional<FileHandle> download(String blobKey, String variant);
Optional<FileHandle> download(String blobKey, String variant, boolean waitLonger);
BasicBlobStorageSpace
tryResolvePhysicalKey
has been removed, in case it has been overwritten (very unlikely).Additional Notes
Checklist