Skip to content

Chunked encoding for tasks APIs #91935

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

Merged
merged 5 commits into from
Nov 28, 2022

Conversation

DaveCTurner
Copy link
Contributor

This response can reach many MiB in size in a large and busy cluster, let's use chunking here.

Relates #89838

This response can reach many MiB in size in a large and busy cluster,
let's use chunking here.

Relates elastic#89838
@DaveCTurner DaveCTurner added >non-issue :Distributed Coordination/Task Management Issues for anything around the Tasks API - both persistent and node level. v8.7.0 labels Nov 24, 2022
@DaveCTurner DaveCTurner marked this pull request as ready for review November 24, 2022 20:40
@elasticsearchmachine elasticsearchmachine added the Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. label Nov 24, 2022
@elasticsearchmachine
Copy link
Collaborator

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

Copy link
Contributor

@original-brownbear original-brownbear left a comment

Choose a reason for hiding this comment

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

Looks good, just one spot to fix I think :)

@SafeVarargs
@SuppressWarnings("varargs")
public static <T> Stream<T> concat(Stream<T>... streams) {
if (streams.length == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just return Arrays.stream(streams).flatMap(Function.identity()); ? We might even get into stack depth troubles if we use the concat like we do here?
The docs for concat state:

Use caution when constructing streams from repeated concatenation. Accessing an element of a deeply concatenated stream can result in deep call chains, or even StackOverflowError.
Subsequent changes to the sequential/parallel execution mode of the returned stream are not guaranteed to be propagated to the input streams.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We might even get into stack depth troubles

Touché 😁

Yeah you're right .flatMap(Function.identity()) is better here, no need for a utility to do this.

Copy link
Contributor

@original-brownbear original-brownbear left a comment

Choose a reason for hiding this comment

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

LGTM :)

@DaveCTurner DaveCTurner merged commit 4095d65 into elastic:main Nov 28, 2022
@DaveCTurner DaveCTurner deleted the 2022-11-24-chunked-tasks-apis branch November 28, 2022 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed Coordination/Task Management Issues for anything around the Tasks API - both persistent and node level. >non-issue Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. v8.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants