Fix handling of artifact range requests #1445
Conversation
|
Can one of the admins verify this patch? |
zyga
force-pushed
the
fix/better-range-requests
branch
3 times, most recently
from
d2940c9 to
e6407fc
Compare
Artifact range requests in HawkBit were impossible to do efficiently, as the underlying mechanism had no choice but to read the start byte and discard content both before and after. To address this situation, we need to introduce a new method to DbArtifact: getFileInputStream(long, long), which maps to the semantics of a range request. The next step is to adjust FileStreamingUtil to use it for both simple range and multi-part requests. Signed-off-by: Zygmunt Krynicki <[email protected]>
zyga
force-pushed
the
fix/better-range-requests
branch
from
e6407fc to
c684a77
Compare
There was a problem hiding this comment.
Hi @zyga,
thanks for your contribution. Sorry, for the delayed review.
I did the review and do have some comments.
| * | ||
| * @return {@link InputStream} to read from artifact. | ||
| */ | ||
| InputStream getFileInputStream(long start, long end); |
There was a problem hiding this comment.
here an default implementation could be provided - one that skips the reading of input stream start.
| @@ -63,4 +63,9 @@ public String getContentType() { | |||
| public InputStream getFileInputStream() { | |||
| return decryptionFunction.apply(encryptedDbArtifact.getFileInputStream()); | |||
| } | |||
|
|
|||
| @Override | |||
| public InputStream getFileInputStream(long start, long end) { | |||
There was a problem hiding this comment.
start and end are pointers in the plain (decrypted input stream).
here in encryptedDbArtifact.getFileInputStream(start, end) they are applied to the encrypted stream where the offsets could differ.
Also, decryption function could be stateful and depend on the previous content.
I'd propose to implement implement default range function and to use it for that DB
| @@ -340,8 +340,6 @@ private static long copyStreams(final InputStream from, final OutputStream to, | |||
| long total = 0; | |||
| int progressPercent = 1; | |||
|
|
|||
| ByteStreams.skipFully(from, start); | |||
There was a problem hiding this comment.
since this method, now reads from 0 to length (exclusively) "final long start" is little bit misleading.
I'd propose to remove start and file name parameters and to log start/end in callers
Or at least rename start / length to rangeStart / rangeLength so the start param to be not correlated to input stream start offset
| public InputStream getFileInputStream(long start, long end) { | ||
| try { | ||
| var f = new RandomAccessFile(file, "r"); | ||
| var ch = f.getChannel(); |
There was a problem hiding this comment.
I'd rather not add an additional dependency on com.google.common
wouldn't
f.seek(start);
return f;
do the same?
|
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
Artifact range requests in HawkBit were impossible to do efficiently, as the underlying mechanism had no choice but to read the start byte and discard content both before and after.
To address this situation, we need to introduce a new method to DbArtifact: getFileInputStream(long, long), which maps to the semantics of a range request.
The next step is to adjust FileStreamingUtil to use it for both simple range and multi-part requests.