Skip to content

Chunked encoding for tasks APIs#91935

Merged
DaveCTurner merged 5 commits intoelastic:mainfrom
DaveCTurner:2022-11-24-chunked-tasks-apis
Nov 28, 2022
Merged

Chunked encoding for tasks APIs#91935
DaveCTurner merged 5 commits intoelastic:mainfrom
DaveCTurner:2022-11-24-chunked-tasks-apis

Conversation

@DaveCTurner
Copy link
Copy Markdown
Member

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/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 Meta label for distributed team. label Nov 24, 2022
@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

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

Copy link
Copy Markdown
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
Copy Markdown
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
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 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
Copy Markdown
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/Task Management Issues for anything around the Tasks API - both persistent and node level. >non-issue Team:Distributed Meta label for distributed team. v8.7.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants