-
Notifications
You must be signed in to change notification settings - Fork 25.1k
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
Add fleet polling API for global checkpoint #71093
Conversation
Pinging @elastic/es-distributed (Team:Distributed) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did an initial look through, only minor comments. Let me know once tests are added.
final ActionListener<Response> listener) { | ||
try { | ||
super.asyncShardOperation(request, shardId, listener); | ||
} catch (final IOException caught) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to just drop that from the signature of asyncShardOperation
, perhaps in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah probably follow-up.
...ugin/fleet/src/main/java/org/elasticsearch/xpack/fleet/action/GetGlobalCheckpointAction.java
Outdated
Show resolved
Hide resolved
...ugin/fleet/src/main/java/org/elasticsearch/xpack/fleet/action/GetGlobalCheckpointAction.java
Outdated
Show resolved
Hide resolved
...n/fleet/src/main/java/org/elasticsearch/xpack/fleet/rest/RestGetGlobalCheckpointsAction.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a few comments.
...gin/fleet/qa/rest/src/yamlRestTest/resources/rest-api-spec/test/fleet/global_checkpoints.yml
Outdated
Show resolved
Hide resolved
...ugin/fleet/src/main/java/org/elasticsearch/xpack/fleet/action/GetGlobalCheckpointAction.java
Outdated
Show resolved
Hide resolved
...gin/fleet/src/main/java/org/elasticsearch/xpack/fleet/action/GetGlobalCheckpointsAction.java
Show resolved
Hide resolved
...gin/fleet/src/main/java/org/elasticsearch/xpack/fleet/action/GetGlobalCheckpointsAction.java
Show resolved
Hide resolved
...gin/fleet/qa/rest/src/yamlRestTest/resources/rest-api-spec/test/fleet/global_checkpoints.yml
Outdated
Show resolved
Hide resolved
rest-api-spec/src/main/resources/rest-api-spec/api/fleet.global_checkpoints.json
Outdated
Show resolved
Hide resolved
rest-api-spec/src/main/resources/rest-api-spec/api/fleet.global_checkpoints.json
Outdated
Show resolved
Hide resolved
Tested with Fleet Server changes to utilize the new API. All works as expected. Thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more comments, looking good otherwise.
@@ -0,0 +1,13 @@ | |||
[role="xpack"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The preview diff from the docs build did not include this. I think you need to add it to index.asciidoc
too to get it on the main page and thus included into the docs output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still do not see it in the preview. I think a link is still missing here:
include::{es-repo-dir}/ccr/apis/ccr-apis.asciidoc[] |
similar to the one for CCR.
...gin/fleet/qa/rest/src/yamlRestTest/resources/rest-api-spec/test/fleet/global_checkpoints.yml
Show resolved
Hide resolved
x-pack/plugin/fleet/src/internalClusterTest/java/fleet/action/GetGlobalCheckpointsActionIT.java
Outdated
Show resolved
Hide resolved
...gin/fleet/src/main/java/org/elasticsearch/xpack/fleet/action/GetGlobalCheckpointsAction.java
Outdated
Show resolved
Hide resolved
...gin/fleet/src/main/java/org/elasticsearch/xpack/fleet/action/GetGlobalCheckpointsAction.java
Show resolved
Hide resolved
...gin/fleet/src/main/java/org/elasticsearch/xpack/fleet/action/GetGlobalCheckpointsAction.java
Outdated
Show resolved
Hide resolved
...leet/src/main/java/org/elasticsearch/xpack/fleet/action/GetGlobalCheckpointsShardAction.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/fleet/src/internalClusterTest/java/fleet/action/GetGlobalCheckpointsActionIT.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docs still do not appear in the preview. Notice that you can navigate to the preview from the docs build on the PR.
Once it appears there (and looks OK) and the tests pass, this LGTM.
@Override | ||
public List<ActionHandler<? extends ActionRequest, ? extends ActionResponse>> getActions() { | ||
return Arrays.asList( | ||
new ActionHandler<>(GetGlobalCheckpointsAction.INSTANCE, GetGlobalCheckpointsAction.TransportGetGlobalCheckpointsAction.class), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: just for consistency we should rename this to just TransportAction
too? Completely optional.
Fleet server needs an API to access up to date global checkpoints for
indices. Additionally, it requires a mode of operation when fleet can
provide its current knowledge about the global checkpoints and poll for
advancements. This commit introduces this API in the fleet plugin.