Skip to content

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

Merged
benwtrent merged 6 commits intoelastic:masterfrom
benwtrent:feature/ml-make-heavy-apis-cancellable
Jul 5, 2022
Merged

[ML] make more GET and heavier processing APIs cancellable#88142
benwtrent merged 6 commits intoelastic:masterfrom
benwtrent:feature/ml-make-heavy-apis-cancellable

Conversation

@benwtrent
Copy link
Copy Markdown
Member

@benwtrent benwtrent commented Jun 28, 2022

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
Copy Markdown
Collaborator

elasticmachine commented Jun 28, 2022

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 Outdated
Copy link
Copy Markdown
Member Author

@benwtrent benwtrent Jun 28, 2022

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
Copy Markdown
Member Author

@benwtrent benwtrent Jun 29, 2022

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 Outdated
Copy link
Copy Markdown
Member Author

@benwtrent benwtrent Jun 28, 2022

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 Outdated
Copy link
Copy Markdown
Member Author

@benwtrent benwtrent Jun 28, 2022

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
Copy Markdown
Member Author

benwtrent commented Jun 29, 2022

@elasticmachine update branch

@benwtrent
Copy link
Copy Markdown
Member Author

benwtrent commented Jul 5, 2022

@elasticmachine update branch

@benwtrent @droberts195
Apply suggestions from code review
fd8400a 
Co-authored-by: David Roberts <dave.roberts@elastic.co>
droberts195
droberts195 approved these changes Jul 5, 2022
View reviewed changes
Copy link
Copy Markdown

@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
Copy Markdown
Member Author

benwtrent commented Jul 5, 2022

@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

1 more reviewer

@droberts195 droberts195 droberts195 approved these changes

Reviewers whose approvals may not affect merge requirements

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