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.

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

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

Merged

Conversation

original-brownbear
Copy link
Member

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 Coordination/Network Http and internode communication implementations v8.5.0 labels Sep 19, 2022
@elasticsearchmachine
Copy link
Collaborator

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

@elasticsearchmachine elasticsearchmachine added the Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. label Sep 19, 2022
import java.util.List;
import java.util.Locale;
import java.util.Map;

public class IndicesSegmentResponse extends BroadcastResponse {
public class IndicesSegmentResponse extends BroadcastResponse implements ChunkedToXContent {
Copy link
Contributor

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
Contributor

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
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
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
Member 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 Coordination/Network Http and internode communication implementations >non-issue Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. v8.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants