Skip to content

Make use of Chunked Encoding in the REST segments API#90136

Merged
original-brownbear merged 1 commit intoelastic:mainfrom
original-brownbear:chunk-segments-response
Sep 20, 2022
Merged

Make use of Chunked Encoding in the REST segments API#90136
original-brownbear merged 1 commit intoelastic:mainfrom
original-brownbear:chunk-segments-response

Conversation

@original-brownbear
Copy link
Copy Markdown
Contributor

This one returns the largest of all responses in the diagnostics in most cases, making it chunked by index which should be good enough.

relates #89838

This one returns the largest of all responses in the diagnostics
in most cases, making it chunked by index which should be good enough.
@original-brownbear original-brownbear added >non-issue :Distributed/Network Http and internode communication implementations v8.5.0 labels Sep 19, 2022
@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

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

import java.util.Map;

public class IndicesSegmentResponse extends BroadcastResponse {
public class IndicesSegmentResponse extends BroadcastResponse implements ChunkedToXContent {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hmm this is another case of something that implements both chunked and unchunked interfaces, risking accidental de-chunking (and see also that duplicated call to buildBroadcastShardsHeader).

Could we instead make BroadcastResponse always use chunked responses and change addCustomXContentFields to something chunking-compatible? That'd fix this case plus indices stats, searchable snapshot stats, data stream stats, field usage stats, disk usage stats, ...

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

(btw I'm ok to make this tactical change in 8.5 given its impact as long as we follow up with a better solution in 8.6)

Copy link
Copy Markdown
Contributor

@henningandersen henningandersen left a comment

Choose a reason for hiding this comment

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

LGTM.

I'd also like to see us not implement ToXContent, but ok to defer to a follow-up (hoping we can do this soon).

public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
// override the BroadcastResponse behavior that is not compatible with the toXContentChunked implementation
// TODO: make this not a ToXContent to make this unnecessary
return ChunkedToXContent.super.toXContent(builder, params);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we assert false here? May need more test fixing but seems we'd need that anyway.

@original-brownbear
Copy link
Copy Markdown
Contributor Author

Thanks both, merging this today as is but will follow-up with removing the toXContent and even more importantly making BroadcastResponse chunked in all cases (was planned anyways :)) asap.

@original-brownbear original-brownbear merged commit 5edfbcc into elastic:main Sep 20, 2022
@original-brownbear original-brownbear deleted the chunk-segments-response branch September 20, 2022 11:16
@original-brownbear original-brownbear restored the chunk-segments-response branch April 18, 2023 20:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Distributed/Network Http and internode communication implementations >non-issue Team:Distributed Meta label for distributed team. v8.5.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants