Introduce StreamingXContentResponse#111933
Conversation
Similar to `ChunkedZipResponse` (elastic#109820) this utility allows Elasticsearch to send an `XContent`-based response constructed out of a sequence of `ChunkedToXContent` fragments, provided in a streaming and asynchronous fashion. This will enable elastic#93735 to proceed without needing to create a temporary index to hold the intermediate results.
|
Pinging @elastic/es-distributed (Team:Distributed) |
server/src/main/java/org/elasticsearch/rest/StreamingXContentResponse.java
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/rest/StreamingXContentResponse.java
Show resolved
Hide resolved
|
Hi! I added a couple of comments about refactoring the StreamingXContentResponse class. I am happy to send you a small patch to change the code. Let me know what you think? |
| releasables.add(fragment.releasable()); | ||
| } | ||
| assert fragmentQueue.isEmpty() : fragmentQueue.size(); // no concurrent adds | ||
| assert releasables.size() == taskCount - 1 || releasables.size() == taskCount - 2 : taskCount + " vs " + releasables.size(); |
There was a problem hiding this comment.
I'm missing something here, why would releaseables.size() == taskCount - 2, I get that queueLength is an upper bound, but drainQueue only happens after the enqueues have stopped (thus making queueLength accurate), correct?
There was a problem hiding this comment.
We pop one item from the queue before we start to process it, but we decrement queueLength when its processing is complete. So mostly queueLength will be counting this one extra item that isn't really in the queue any more.
nicktindall
left a comment
There was a problem hiding this comment.
One minor clarification, but otherwise LGTM
* upstream/main: Fail `indexDocs()` on rejection (elastic#111962) Move repo analyzer to its own package (elastic#111963) Add generated evaluators for DateNanos conversion functions (elastic#111961) Clean the last traces from global retention in templates (elastic#111669) Fix known issue docs for elastic#111866 (elastic#111956) x-pack/plugin/otel: introduce x-pack-otel plugin (elastic#111091) Improve reaction to blob store corruptions (elastic#111954) Introduce `StreamingXContentResponse` (elastic#111933) Revert "Add 8.15.0 known issue for memory locking in Windows (elastic#111949)" Test get-snapshots API with missing details (elastic#111903) Add 8.15.0 known issue for memory locking in Windows (elastic#111949) # Conflicts: # server/src/main/java/org/elasticsearch/TransportVersions.java
Similar to `ChunkedZipResponse` (elastic#109820) this utility allows Elasticsearch to send an `XContent`-based response constructed out of a sequence of `ChunkedToXContent` fragments, provided in a streaming and asynchronous fashion. This will enable elastic#93735 to proceed without needing to create a temporary index to hold the intermediate results.
Similar to `ChunkedZipResponse` (elastic#109820) this utility allows Elasticsearch to send an `XContent`-based response constructed out of a sequence of `ChunkedToXContent` fragments, provided in a streaming and asynchronous fashion. This will enable elastic#93735 to proceed without needing to create a temporary index to hold the intermediate results.
Silly test mistake: the throttle was supposed to limit the number of fragments in the queue, but was closing the throttling ref after submission rather than transmission so that the queue could grow without bound sometimes. It's been like this since the test was introduced in elastic#111933, but it only started failing recently. I suspect until the performance improvement in elastic#139988 landed it couldn't actually submit enough tasks at once to cause a failure. Closes elastic#143644
Silly test mistake: the throttle was supposed to limit the number of fragments in the queue, but was closing the throttling ref after submission rather than transmission so that the queue could grow without bound sometimes. It's been like this since the test was introduced in #111933, but it only started failing recently. I suspect until the performance improvement in #139988 landed it couldn't actually submit enough tasks at once to cause a failure. Closes #143644
Silly test mistake: the throttle was supposed to limit the number of fragments in the queue, but was closing the throttling ref after submission rather than transmission so that the queue could grow without bound sometimes. It's been like this since the test was introduced in elastic#111933, but it only started failing recently. I suspect until the performance improvement in elastic#139988 landed it couldn't actually submit enough tasks at once to cause a failure. Closes elastic#143644
Silly test mistake: the throttle was supposed to limit the number of fragments in the queue, but was closing the throttling ref after submission rather than transmission so that the queue could grow without bound sometimes. It's been like this since the test was introduced in elastic#111933, but it only started failing recently. I suspect until the performance improvement in elastic#139988 landed it couldn't actually submit enough tasks at once to cause a failure. Closes elastic#143644
Silly test mistake: the throttle was supposed to limit the number of fragments in the queue, but was closing the throttling ref after submission rather than transmission so that the queue could grow without bound sometimes. It's been like this since the test was introduced in #111933, but it only started failing recently. I suspect until the performance improvement in #139988 landed it couldn't actually submit enough tasks at once to cause a failure. Closes #143644
Silly test mistake: the throttle was supposed to limit the number of fragments in the queue, but was closing the throttling ref after submission rather than transmission so that the queue could grow without bound sometimes. It's been like this since the test was introduced in elastic#111933, but it only started failing recently. I suspect until the performance improvement in elastic#139988 landed it couldn't actually submit enough tasks at once to cause a failure. Closes elastic#143644
Silly test mistake: the throttle was supposed to limit the number of fragments in the queue, but was closing the throttling ref after submission rather than transmission so that the queue could grow without bound sometimes. It's been like this since the test was introduced in #111933, but it only started failing recently. I suspect until the performance improvement in #139988 landed it couldn't actually submit enough tasks at once to cause a failure. Closes #143644
Silly test mistake: the throttle was supposed to limit the number of fragments in the queue, but was closing the throttling ref after submission rather than transmission so that the queue could grow without bound sometimes. It's been like this since the test was introduced in #111933, but it only started failing recently. I suspect until the performance improvement in #139988 landed it couldn't actually submit enough tasks at once to cause a failure. Closes #143644
Silly test mistake: the throttle was supposed to limit the number of fragments in the queue, but was closing the throttling ref after submission rather than transmission so that the queue could grow without bound sometimes. It's been like this since the test was introduced in elastic#111933, but it only started failing recently. I suspect until the performance improvement in elastic#139988 landed it couldn't actually submit enough tasks at once to cause a failure. Closes elastic#143644
Similar to
ChunkedZipResponse(#109820) this utility allowsElasticsearch to send an
XContent-based response constructed out of asequence of
ChunkedToXContentfragments, provided in a streaming andasynchronous fashion.
This will enable #93735 to proceed without needing to create a temporary
index to hold the intermediate results.