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.

Sign up for GitHub

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

Jump to bottom

[ML] make more GET and heavier processing APIs cancellable #88142

Merged
merged 6 commits into from
Jul 5, 2022
Merged

[ML] make more GET and heavier processing APIs cancellable #88142

merged 6 commits into from
Jul 5, 2022

Conversation

benwtrent
Copy link
Member

This commit makes the following APIs cancellable:

  • get datafeeds
  • get jobs
  • evaluate data frames
  • explain data frames
  • get job model snapshot
  • preview datafeed
  • preview data frame
  • get categories (this can be expensive due to grok parsing)

@elasticmachine elasticmachine added the Team:ML Meta label for the ML team label Jun 28, 2022
@elasticmachine
Copy link
Collaborator

Pinging @elastic/ml-core (Team:ML)

@benwtrent benwtrent force-pushed the feature/ml-make-heavy-apis-cancellable branch from 36e1e06 to f83534e Compare June 28, 2022 20:17
benwtrent
benwtrent commented Jun 28, 2022
View reviewed changes
...re/src/main/java/org/elasticsearch/xpack/core/ml/action/ExplainDataFrameAnalyticsAction.java
@@ -30,6 +45,118 @@ private ExplainDataFrameAnalyticsAction() {
super(NAME, ExplainDataFrameAnalyticsAction.Response::new);
}

public static class Request extends AcknowledgedRequest<Request> implements ToXContentObject {
Copy link
Member Author

Choose a reason for hiding this comment

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

I added this request object (just a copy of PutDataFrameAnalytics) as the PUT object was being reused here, but I didn't want to make that request's task cancellable (as PUT isn't cancellable).

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, I added a BWC test for explain in a mixed cluster to ensure this doesn't break anything. Ran it many many times and its all good since the explain request and the put request can be serialized to each other without issue.

benwtrent
benwtrent commented Jun 28, 2022
View reviewed changes
...rc/main/java/org/elasticsearch/xpack/ml/action/TransportPreviewDataFrameAnalyticsAction.java
@@ -101,13 +102,14 @@ protected void doExecute(Task task, Request request, ActionListener<Response> li
}

void preview(Task task, DataFrameAnalyticsConfig config, ActionListener<Response> listener) {
final TaskId parentTaskId = new TaskId(clusterService.getNodeName(), task.getId());
Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure if task.getParentTaskId() was strictly correct. task is indeed the transport action task (created via our request object) that is being executed here. It SHOULD have a parent task that is the original HTTP request (I think?) but it seems that the prevailing pattern is to rely on this task object instead of its parent.

benwtrent
benwtrent commented Jun 28, 2022
View reviewed changes
...lugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportPreviewDatafeedAction.java
@@ -131,7 +135,7 @@ private void previewDatafeed(
// requesting the preview doesn't have permission to search the relevant indices.
DatafeedConfig previewDatafeedConfig = previewDatafeedBuilder.build();
DataExtractorFactory.create(
new ParentTaskAssigningClient(client, clusterService.localNode(), task),
new ParentTaskAssigningClient(client, parentTaskId),
Copy link
Member Author

Choose a reason for hiding this comment

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

This was a nice idea, but since the task being created by the request was never cancellable (and neither was the http request), the cascade effect of cancellations was missed :/

@benwtrent benwtrent force-pushed the feature/ml-make-heavy-apis-cancellable branch from f83534e to bd28f76 Compare June 28, 2022 20:30
@benwtrent
Copy link
Member Author

@elasticmachine update branch

@benwtrent
Copy link
Member Author

@elasticmachine update branch

droberts195
droberts195 approved these changes Jul 5, 2022
View reviewed changes
Copy link
Contributor

@droberts195 droberts195 left a comment

Choose a reason for hiding this comment

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

LGTM

@benwtrent benwtrent added the test-full-bwc Trigger full BWC version matrix tests label Jul 5, 2022
@benwtrent
Copy link
Member Author

@elasticmachine update branch

@benwtrent benwtrent merged commit bff8840 into elastic:master Jul 5, 2022
@benwtrent benwtrent mentioned this pull request Jul 5, 2022
Open
30 tasks
@benwtrent benwtrent mentioned this pull request Jul 19, 2022
Merged
elasticsearchmachine pushed a commit that referenced this pull request Jul 19, 2022
@benwtrent
[ML] fix ml API cancellation when http connection is closed (#88616)
0d7e4c2 
API cancellation was erroneously using the node-name for the parent task
id. Task ids are constructed via the node ID, not name. 

This fixes that bug.

related to:   - #88030  -
#88142  -
#88009
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@droberts195 droberts195 droberts195 approved these changes

Assignees
No one assigned
Labels
:ml Machine learning >non-issue Team:ML Meta label for the ML team test-full-bwc Trigger full BWC version matrix tests v8.4.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@benwtrent @elasticmachine @droberts195