Skip to content

Expose shard changes action as a rest api#118608

Merged
salvatore-campagna merged 15 commits intoelastic:mainfrom
salvatore-campagna:feature/shard-changes-action-rest
Dec 16, 2024
Merged

Expose shard changes action as a rest api#118608
salvatore-campagna merged 15 commits intoelastic:mainfrom
salvatore-campagna:feature/shard-changes-action-rest

Conversation

@salvatore-campagna
Copy link
Contributor

@salvatore-campagna salvatore-campagna commented Dec 12, 2024

This PR adds a new REST API /{index}/ccr/shard_changes to retrieve shard-level changes, including translog operations, mapping versions, and sequence numbers. It is required for a new set of Rally benchmarks, which will be used to evaluate the impact of synthetic source on recovery time. The API accepts parameters like from_seq_no, max_batch_size, poll_timeout, and max_operations_count and is exposed only in snapshot builds.

@salvatore-campagna salvatore-campagna marked this pull request as ready for review December 13, 2024 13:43
@salvatore-campagna salvatore-campagna self-assigned this Dec 13, 2024
@elasticsearchmachine elasticsearchmachine added the needs:triage Requires assignment of a team area label label Dec 13, 2024
// forget follower API
new RestForgetFollowerAction()
);
return Stream.concat(
Copy link
Contributor Author

@salvatore-campagna salvatore-campagna Dec 13, 2024

Choose a reason for hiding this comment

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

Arrays.asList returns a fixed-size list. As a result, adding later the additional handler based on the value of Build.current().isSnapshot() might fail with an UnsupportedOperationException.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe just create an array list with an if statement for the shard changes action? Looks simpler to me than all this stream stuff.

@salvatore-campagna salvatore-campagna added >non-issue :StorageEngine/Logs You know, for Logs and removed needs:triage Requires assignment of a team area label labels Dec 13, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-storage-engine (Team:StorageEngine)

builder.field("number_of_operations", response.getOperations().length);
builder.field("operations");
builder.startArray();
for (final Translog.Operation operation : response.getOperations()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we can avoid this...as serializing each operation for large requests might be way too much?

Copy link
Member

Choose a reason for hiding this comment

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

I see that you already left out the actual content of the write operations, which is good for the use case this rest api is going to be used. Let's for now keep the response as is here. If this turns out the be problematic we can always drop the operations array from the response.

Copy link
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

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

Left a few nits. LGTM 👍

builder.field("number_of_operations", response.getOperations().length);
builder.field("operations");
builder.startArray();
for (final Translog.Operation operation : response.getOperations()) {
Copy link
Member

Choose a reason for hiding this comment

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

I see that you already left out the actual content of the write operations, which is good for the use case this rest api is going to be used. Let's for now keep the response as is here. If this turns out the be problematic we can always drop the operations array from the response.

// forget follower API
new RestForgetFollowerAction()
);
return Stream.concat(
Copy link
Member

Choose a reason for hiding this comment

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

Maybe just create an array list with an if statement for the shard changes action? Looks simpler to me than all this stream stuff.

import static org.elasticsearch.rest.RestRequest.Method.GET;

/**
* A REST handler that retrieves shard changes in a specific index whose name is provided as a parameter.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe at the top of the java docs, mention that this rest action is only loaded in snapshot builds and its use is for benchmarking purposes only?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a note below in the Javadoc.

assertOK(client().performRequest(bulkRequest(indexName, randomIntBetween(10, 20))));

final Request shardChangesRequest = new Request("GET", shardChangesEndpoint(indexName));
assertOK(client().performRequest(shardChangesRequest));
Copy link
Member

Choose a reason for hiding this comment

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

maybe assert the 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.

Done

salvatore-campagna added a commit to elastic/rally-tracks that referenced this pull request Jan 22, 2025
…#722)

Benchmarking the new API introduced in PR elastic/elasticsearch#118608 to evaluate synthetic source recovery latency. The benchmark will target indices or data streams in the `elastic/logs`, `elastic/security`, and `tsdb` tracks.

Key notes:
- A retention lease must be added before indexing to prevent missing `seq_no` values during shard recovery.
- The API in PR elastic/elasticsearch#118937 supports data streams and index aliases, allowing flexibility in targeting indices, especially for data stream use cases.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Comments