Add chunked xcontent handling to SearchTemplateResponse#99184
Add chunked xcontent handling to SearchTemplateResponse#99184romseygeek merged 15 commits intoelastic:mainfrom
Conversation
|
Pinging @elastic/es-search (Team:Search) |
|
Seems ok to me, although I haven't gone through to verify that all the response-status handling works. Is that covered by enough other tests or does it need closer inspection? |
|
The status handling for multisearchtemplate is covered by YAML tests. We have a test that checks we get a 400 for a bad request in the searchtemplate tests, but I think those are handled elsewhere in the stack, and I'm not sure we have proper tests anywhere for non-200 responses due to things like shard failures. I'll take another look. |
|
Having looked again... RestRenderSearchTemplateAction does not need status handling. RestMultiSearchTemplateAction did not have status handling before, although it arguably should have some. RestSearchTemplateAction has the standard status handling of RestSearchAction, but as far as I can tell this is not actually tested anywhere for either the template or the standard action. I will open an issue to improve testing around status handling for searches more generally, but I don't think that needs to block this. |
|
Hey @romseygeek, this PR has been open for a while. Are we still planning to merge it? If yes, could you please resolve the conflicts? |
|
Wow, this is an old one. I've brought it up to date (and learned something about ref counting in the process). |
|
We'll also be able to close out the parent issue (#95661) once this is merged. |
| ElasticsearchException.generateFailureXContent(b, p, Item.this.getFailure(), true); | ||
| b.field(Fields.STATUS, ExceptionsHelper.status(Item.this.getFailure()).getStatus()); | ||
| b.endObject(); | ||
| return b; |
There was a problem hiding this comment.
This failure branch serializes the full error object as a single, non-chunked ToXContent. If an exception serializes a very large stack trace or metadata, this chunk could be large. I also checked other related commit and found that, for example, MultiSearchResponse handles failures by chunking them along the failure path.
There was a problem hiding this comment.
I can use the same logic as in MultiSearchResponse, but it doesn't actually help much. The stack trace will still be a single chunk, as that is how ElasticsearchException.generateFailureXContent works.
| return Iterators.concat( | ||
| ChunkedToXContentHelper.startObject(), | ||
| Item.this.getResponse().innerToXContentChunked(params), | ||
| Iterators.single((b, p) -> b.field(Fields.STATUS, Item.this.getResponse().status().getStatus())), |
There was a problem hiding this comment.
nit: Item.this.getFailure() could be replaced by getFailure(), in all places where it is applied
@romseygeek, thank you for working on this. This is a nice, clean piece of work. I have a question: have you opened a ticket for handling searches? I 'd like to link it to the SearchInfra work. |
…late' into xcontent/chunked-search-template
I don't think I ever did open this, or at least if I did I can't find it anymore! |
Relates to #95661