Skip to content
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

Chunked encoding for CCR APIs #92526

Merged

Conversation

DaveCTurner
Copy link
Contributor

The CCR info and stats APIs can send fairly sizeable responses that scale as O(#shards). This commit makes them chunked.

Relates #89838

The CCR info and stats APIs can send fairly sizeable responses that
scale as `O(#shards)`. This commit makes them chunked.

Relates elastic#89838
@DaveCTurner DaveCTurner added >non-issue :Distributed Indexing/CCR Issues around the Cross Cluster State Replication features v8.7.0 labels Dec 22, 2022
@elasticsearchmachine
Copy link
Collaborator

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

@elasticsearchmachine elasticsearchmachine added the Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. label Dec 22, 2022
Copy link
Member

@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 a random NIT and a question on the threading.

return channel -> client.execute(
CcrStatsAction.INSTANCE,
request,
new ThreadedActionListener<>(
Copy link
Member

Choose a reason for hiding this comment

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

Why do I still need a threaded listener with chunked encoding?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The toXContent implementation collects all the shard responses into a map keyed by index up-front - I think that should be done elsewhere.

// sort by index name, then shard ID
final Map<String, Map<Integer, StatsResponse>> taskResponsesByIndex = new TreeMap<>();
for (final StatsResponse response : statsResponse) {
taskResponsesByIndex.computeIfAbsent(response.status().followerIndex(), k -> new TreeMap<>())
.put(response.status().getShardId(), response);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On reflection this could reasonably be using the CCR pool, adjusted in a194c11.

return channel -> client.execute(
FollowStatsAction.INSTANCE,
request,
new ThreadedActionListener<>(
Copy link
Member

Choose a reason for hiding this comment

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

Same here why does this need threading if it's chunked encoding?

(builder, params) -> builder.startObject().field("auto_follow_stats", autoFollowStats, params).field("follow_stats")
),
followStats.toXContentChunked(outerParams),
Iterators.single((builder, params) -> builder.endObject())
Copy link
Member

Choose a reason for hiding this comment

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

NIT: I added org.elasticsearch.common.xcontent.ChunkedToXContentHelper#endObject for this.

Copy link
Member

@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 thanks for the explanation.

@DaveCTurner DaveCTurner added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Dec 22, 2022
@elasticsearchmachine elasticsearchmachine merged commit 9906dbf into elastic:main Dec 22, 2022
@DaveCTurner DaveCTurner deleted the 2022-12-22-ccr-chunking branch December 22, 2022 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) :Distributed Indexing/CCR Issues around the Cross Cluster State Replication features >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