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.

Sign up for GitHub

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

Avoid closing response stream while downloading instance logs #14730

Merged
merged 2 commits into from
Jan 17, 2025
Merged

Avoid closing response stream while downloading instance logs #14730

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8c47202
Avoid closing response stream while downloading instance logs
shounakmk219 Dec 30, 2024
1c03af8
Use StreamingOutput to handle the log file data stream delegation
shounakmk219 Jan 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 9 additions & 11 deletions ...roller/src/main/java/org/apache/pinot/controller/api/resources/PinotControllerLogger.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,18 +224,16 @@ public Response downloadLogFileFromInstance(
if (authorization != null) {
requestBuilder.addHeader(HttpHeaders.AUTHORIZATION, authorization);
}
try (CloseableHttpResponse httpResponse = _fileUploadDownloadClient.getHttpClient()
.execute(requestBuilder.build())) {
if (httpResponse.getCode() >= 400) {
throw new WebApplicationException(IOUtils.toString(httpResponse.getEntity().getContent(), "UTF-8"),
Response.Status.fromStatusCode(httpResponse.getCode()));
}
Response.ResponseBuilder builder = Response.ok();
builder.entity(httpResponse.getEntity().getContent());
builder.contentLocation(uri);
builder.header(HttpHeaders.CONTENT_LENGTH, httpResponse.getEntity().getContentLength());
return builder.build();
CloseableHttpResponse httpResponse = _fileUploadDownloadClient.getHttpClient().execute(requestBuilder.build());
Copy link
Contributor

Choose a reason for hiding this comment

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

It is not clear to me why this may end up closing the resource for other instances and specially it is not clear to me who is closing this closeable now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah even I am not 100% sure but my understanding is that while controller is streaming log file from other instance its acting like a proxy and passing the same stream to the client/caller. Right now controller is closing the stream before client consumes it.
Maybe jersey is managing closing the stream as we don't handle it in /loggers/download flow as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm also very confused. Does this change intentionally leak connection?

Copy link
Contributor

Choose a reason for hiding this comment

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

Given these downloads may be large, we need to be sure we are cleaning these resources in a correct fashion. Alternatively we can dump the content of the stream to a file and use that file to stream data to the incoming request, but then we will need to be sure we end up deleting the file

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated the code to handle the connection properly. Thanks a lot @gortiz for helping out with the right approach!

Copy link
Contributor

Choose a reason for hiding this comment

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

Kudos to @KKcorps, who gave me the idea of asking Claude. 😆

if (httpResponse.getCode() >= 400) {
throw new WebApplicationException(IOUtils.toString(httpResponse.getEntity().getContent(), "UTF-8"),
Response.Status.fromStatusCode(httpResponse.getCode()));
}
Response.ResponseBuilder builder = Response.ok();
builder.entity(httpResponse.getEntity().getContent());
builder.contentLocation(uri);
builder.header(HttpHeaders.CONTENT_LENGTH, httpResponse.getEntity().getContentLength());
return builder.build();
} catch (IOException e) {
throw new WebApplicationException(e, Response.Status.INTERNAL_SERVER_ERROR);
}
Expand Down
Loading