Skip to content

Introduce StreamingXContentResponse#111933

Merged
DaveCTurner merged 2 commits intoelastic:mainfrom
DaveCTurner:2024/08/15/StreamingXContentResponse
Aug 19, 2024
Merged

Introduce StreamingXContentResponse#111933
DaveCTurner merged 2 commits intoelastic:mainfrom
DaveCTurner:2024/08/15/StreamingXContentResponse

Conversation

@DaveCTurner
Copy link
Copy Markdown
Member

Similar to ChunkedZipResponse (#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 #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.
@DaveCTurner DaveCTurner added >non-issue :Distributed/Network Http and internode communication implementations v8.16.0 labels Aug 15, 2024
@elasticsearchmachine elasticsearchmachine added the Team:Distributed Meta label for distributed team. label Aug 15, 2024
@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

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

@Abhiram98
Copy link
Copy Markdown

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();
Copy link
Copy Markdown
Contributor

@nicktindall nicktindall Aug 19, 2024

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

@nicktindall nicktindall left a comment

Choose a reason for hiding this comment

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

One minor clarification, but otherwise LGTM

@DaveCTurner DaveCTurner merged commit fe7448e into elastic:main Aug 19, 2024
@DaveCTurner DaveCTurner deleted the 2024/08/15/StreamingXContentResponse branch August 19, 2024 05:32
weizijun added a commit to weizijun/elasticsearch that referenced this pull request Aug 19, 2024
* 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
cbuescher pushed a commit to cbuescher/elasticsearch that referenced this pull request Sep 4, 2024
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.
davidkyle pushed a commit to davidkyle/elasticsearch that referenced this pull request Sep 5, 2024
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.
DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this pull request Mar 18, 2026
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
DaveCTurner added a commit that referenced this pull request Mar 19, 2026
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
DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this pull request Mar 19, 2026
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
DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this pull request Mar 19, 2026
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
DaveCTurner added a commit that referenced this pull request Mar 19, 2026
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
ncordon pushed a commit to ncordon/elasticsearch that referenced this pull request Mar 20, 2026
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
elasticsearchmachine pushed a commit that referenced this pull request Mar 23, 2026
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
elasticsearchmachine pushed a commit that referenced this pull request Mar 23, 2026
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
michalborek pushed a commit to michalborek/elasticsearch that referenced this pull request Mar 23, 2026
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
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.16.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants