Issue #929 implement a utility class to save large downloads to a file #12292
+625
−2
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
Signed-off-by: Oleksandr Krutko <alexander.krutko@gmail.com>
Signed-off-by: Oleksandr Krutko <alexander.krutko@gmail.com>
Signed-off-by: Oleksandr Krutko <alexander.krutko@gmail.com>
Signed-off-by: Oleksandr Krutko <alexander.krutko@gmail.com>
Signed-off-by: Oleksandr Krutko <alexander.krutko@gmail.com>
Signed-off-by: Oleksandr Krutko <alexander.krutko@gmail.com>
arsenalzp
changed the title
|
The CI build failed due to javadoc issues. You can see your CI builds here -> https://jenkins.webtide.net/blue/organizations/jenkins/jetty.project/activity?branch=PR-12292 |
Signed-off-by: Oleksandr Krutko <alexander.krutko@gmail.com>
Hello, |
|
All tests passed on my laptop with JDK17. Should I also run tests with JDK22? |
sbordet
requested changes
Sep 20, 2024
There was a problem hiding this comment.
@arsenalzp good effort!
Please follow my review, and your implementation should become much simpler and with less code.
Thanks!
jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/Request.java
Show resolved
Hide resolved
jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/HttpRequest.java
Show resolved
Hide resolved
jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/PathResponseListener.java
Show resolved
Hide resolved
jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/PathResponseListener.java
Show resolved
Hide resolved
jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/PathResponseListener.java
Show resolved
Hide resolved
| * CompletableFuture>Path> completable = PathResponseListener.write(request, Path.of("/tmp/file.bin")); | ||
| * </pre> | ||
| */ | ||
| public class PathResponseListener implements CompleteListener, Response.ContentListener |
There was a problem hiding this comment.
Make this class implement Response.Listener instead, like BufferingResponseListener, so users have all events available if they want to.
This class should extend CompletableFuture<Path>, so users can get its result once the file is written (or failed).
Comment on lines
+91
to
+94
| if (response.getStatus() != HttpStatus.OK_200) | ||
| { | ||
| throw new HttpResponseException(String.format("HTTP status code of this response %d", response.getStatus()), response); | ||
| } |
There was a problem hiding this comment.
Move this check to onHeaders() so it is done only once.
See examples in BufferingResponseListener.
jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/PathResponseListener.java
Show resolved
Hide resolved
|
|
||
| public static CompletableFuture<Path> write(Request request, Path path) | ||
| { | ||
| return CompletableFuture.supplyAsync(() -> |
There was a problem hiding this comment.
This method should be implemented in terms of PathResponseListener, not by rewriting the same logic again.
Comment on lines
+136
to
+154
| public Response get(long timeout, TimeUnit unit) throws InterruptedException, TimeoutException, ExecutionException | ||
| { | ||
| boolean expired = !responseLatch.await(timeout, unit); | ||
| if (expired && this.response == null) | ||
| throw new TimeoutException(); | ||
| try (AutoLock ignored = lock.lock()) | ||
| { | ||
| // If the request failed there is no response. | ||
| if (response == null) | ||
| throw new ExecutionException(failure); | ||
| return response; | ||
| } | ||
| } | ||
|
|
||
| public Response get() throws InterruptedException, ExecutionException | ||
| { | ||
| this.completable.get(); | ||
| return this.response; | ||
| } |
There was a problem hiding this comment.
Remove these methods, as this class should be a CompletableFuture.
Our CI environment will do it, but sure if you want to try, give it a go. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Hello,
This PR is trying to implement feature described in the issue #929.
Fixes: #929