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

Use chunked encoding for indices stats response #91760

Merged

Conversation

original-brownbear
Copy link
Member

These responses can become huge, lets chunk them by index.
Unfortunately, slightly awkward to implement this when the chunks depend on the given params but what can you do ...

relates #89838

These responses can become huge, lets chunk them by index.

relates elastic#89838
@original-brownbear original-brownbear added >non-issue :Distributed/Network Http and internode communication implementations v8.7.0 labels Nov 21, 2022
@elasticsearchmachine elasticsearchmachine added the Team:Distributed Meta label for distributed team label Nov 21, 2022
@elasticsearchmachine
Copy link
Collaborator

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

Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

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

I would be inclined to do all the BroadcastResponse implementations at once, changing addCustomXContentFields so it returns a stream of chunks instead. There's only 8 of them. At least we could change addCustomXContentFields to yield a single chunk in each case and then add finer chunking in follow-ups.

@original-brownbear
Copy link
Member Author

I would be inclined to do all the BroadcastResponse implementations at once

Are we sure none of the others generate a large response? The problem of moving to chunked by just wrapping a single chunk like that when the response is large, is that it will mean that we will serialize a large response on the transport thread where it was on whatever other thread it was on before. I'd rather be careful with that tbh.

@DaveCTurner
Copy link
Contributor

DaveCTurner commented Nov 24, 2022

Ah good point. Several of them are indeed quite large, and a little too complex to do them all at once I think. Could we introduce a ChunkedBroadcastResponse extends BaseBroadcastResponse implements ChunkedToXContent here since that's where I think we will eventually want to end up and it'll make the other moves a bit more mechanical?

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.

@original-brownbear
Copy link
Member Author

here since that's where I think we will eventually want to end up and it'll make the other moves a bit more mechanical?

I just tried this. Currently this would cover IndicesStatsResponse and IndicesSegmentResponse. Both of these work somewhat differently in how they build the response, except for the header field. At least at this point forcing some abstraction onto this doesn't seem to get us much. Maybe try this again when moving the next one and it gets a little clearer what a nice version of this would be?

@DaveCTurner
Copy link
Contributor

I just meant something like this: main...DaveCTurner:elasticsearch:2022-11-24-ChunkedBroadcastResponse-example

i.e. keep the header stuff in the base class and just add the custom stuff in the actual implementations.

@original-brownbear
Copy link
Member Author

Jup that's what I tried too, I guess I didn't really like how it adds extra chunks everywhere but meh maybe that's ok lets do it :)

@original-brownbear
Copy link
Member Author

@DaveCTurner I pushed a775d47 now and used it for the indices stats + existing segment stats.

Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

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

LGTM

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.

LGTM2

@original-brownbear
Copy link
Member Author

Thanks both!

@original-brownbear original-brownbear merged commit 7dbc1ea into elastic:main Nov 24, 2022
@original-brownbear original-brownbear deleted the chunk-indices-stats branch November 24, 2022 17:43
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.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants