Skip to content

Extract ResponseLengthRecorder in REST layer#104836

Merged
elasticsearchmachine merged 2 commits intoelastic:mainfrom
DaveCTurner:2024/01/27/ResponseLengthRecorder
Jan 29, 2024
Merged

Extract ResponseLengthRecorder in REST layer#104836
elasticsearchmachine merged 2 commits intoelastic:mainfrom
DaveCTurner:2024/01/27/ResponseLengthRecorder

Conversation

@DaveCTurner
Copy link
Copy Markdown
Member

Related to #104752, we should not be confounding the lifecycle of the
EncodedLengthTrackingChunkedRestResponseBody with the lifecycle of the
overall RestResponse. This commit introduces a separate object to
record the total response length whose lifecycle is independent of that
of the response body.

Related to elastic#104752, we should not be confounding the lifecycle of the
`EncodedLengthTrackingChunkedRestResponseBody` with the lifecycle of the
overall `RestResponse`. This commit introduces a separate object to
record the total response length whose lifecycle is independent of that
of the response body.
@DaveCTurner DaveCTurner added :Distributed/Network Http and internode communication implementations >refactoring v8.13.0 labels Jan 27, 2024
@DaveCTurner DaveCTurner requested a review from ywangd January 27, 2024 13:53
@elasticsearchmachine elasticsearchmachine added the Team:Distributed Meta label for distributed team. label Jan 27, 2024
@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

Pinging @elastic/es-distributed (Team:Distributed)

Copy link
Copy Markdown
Member

@ywangd ywangd left a comment

Choose a reason for hiding this comment

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

LGTM

Just a discussion point: IIUC, the change is to align EncodedLengthTrackingChunkedRestResponseBody with the new ChunkedRestResponseBody interface to not implement Releasable. While I do think it makes sense to have the overall RestReponse implement Releasable, I found the new code a bit more challenging to follow. Maybe it is OK for both RestResponse and ChunkedRestResponseBody to implement Releasable. I think this is not unreasonable and this PR may not even be necessary in the case?

@DaveCTurner
Copy link
Copy Markdown
Member Author

This fell out of some work to make it so that a RestResponse can be made up of a sequence of ChunkedRestResponseBody instances, not just one, and in that case we want to record the length of the whole sequence. Otherwise yes I'd agree.

@DaveCTurner DaveCTurner added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Jan 29, 2024
@elasticsearchmachine elasticsearchmachine merged commit ed6e8ec into elastic:main Jan 29, 2024
@DaveCTurner DaveCTurner deleted the 2024/01/27/ResponseLengthRecorder branch January 29, 2024 09:14
@DaveCTurner
Copy link
Copy Markdown
Member Author

FWIW this was opened to prepare things for #104851.

@DaveCTurner DaveCTurner restored the 2024/01/27/ResponseLengthRecorder branch June 17, 2024 06:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) :Distributed/Network Http and internode communication implementations >refactoring Team:Distributed Meta label for distributed team. v8.13.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants