-
Notifications
You must be signed in to change notification settings - Fork 25.2k
Add wait for completion for Enrich policy execution #47886
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
Conversation
Pinging @elastic/es-core-features (:Core/Features/Ingest) |
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 two small comments. LGTM otherwise.
.../enrich/src/main/java/org/elasticsearch/xpack/enrich/rest/RestExecuteEnrichPolicyAction.java
Show resolved
Hide resolved
GetTaskRequest getPolicyTaskRequest = new GetTaskRequest().setTaskId(executeResponse.getTaskId()).setWaitForCompletion(true); | ||
GetTaskResponse taskResponse = client().execute(GetTaskAction.INSTANCE, getPolicyTaskRequest).actionGet(); | ||
assertThat(((ExecuteEnrichPolicyStatus) taskResponse.getTask().getTask().getStatus()).getPhase(), | ||
is(ExecuteEnrichPolicyStatus.PolicyPhases.COMPLETE)); |
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 would place this inside an assertBusy(...)
, because there is no guarantee that the task is completed (especially on slow build machines).
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've added the assertBusy line. In the code the task request has wait_for_completion
set to true, is it possible that this call can come back before the task is completed even on slow hardware? I suppose at the very least assertBusy insulates the test against timeout failures
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.
But in this test wait_for_completion
is set to false, right? So it is likely that it returns and then the task isn't completed?
If wait_for_completion
is true (which is the default) then the api doesn't return until the force merge has completed and therefor the associated task has been completed. So in this case it shouldn't be possible for the task not being in a completed state.
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.
Ah nevermind my comment... I missed that wait_for_completion
is also set on GetTaskRequest
. Then the task api will not return until the task in question has been completed. I didn't know this.
So the assertBusy(...) doesn't make sense and can be removed.
This PR adds the ability to run the enrich policy execution task in the background, returning a task id instead of waiting for the completed operation.
This PR adds the ability to run the enrich policy execution task in the background, returning a task id instead of waiting for the completed operation.