-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Use chunked encoding for indices stats response #91760
Conversation
These responses can become huge, lets chunk them by index. relates elastic#89838
Pinging @elastic/es-distributed (Team:Distributed) |
There was a problem hiding this 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.
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. |
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
I just tried this. Currently this would cover |
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. |
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 :) |
@DaveCTurner I pushed a775d47 now and used it for the indices stats + existing segment stats. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM2
server/src/main/java/org/elasticsearch/action/support/broadcast/ChunkedBroadCastResponse.java
Outdated
Show resolved
Hide resolved
Thanks both! |
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