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.

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

update/delete historical detector;search AD tasks #1

Closed
wants to merge 5 commits into from

Conversation

ylwu-amzn
Copy link
Owner

@ylwu-amzn ylwu-amzn commented Jan 15, 2021

This PR is to gather feedback earlier, as this PR depends on some changes of PR opendistro-for-elasticsearch#359, can't send out PR based on master branch. Will send out another to merge into master branch later.

Description of changes:

  1. When user update historical detector, check if there is running task, and user can't switch between realtime and historical.
  2. When user delete historical detector, check if there is running task.
  3. Add. new search API to support searching AD tasks.

Test

  1. ./gradlew build
  2. ./gradlew integTest -PnumNodes=3

Comment on lines 239 to 250
if (existingDetector.isRealTimeDetector()) {
validateDetector(existingDetector);
} else {
validateCategoricalField(detectorId);
adTaskManager.getLatestADTask(detectorId, (adTask) -> {
if (adTask.isPresent() && !adTaskManager.isADTaskEnded(adTask.get())) {
// can't update detector if there is AD task running
listener.onFailure(new ElasticsearchStatusException("Detector is running", RestStatus.INTERNAL_SERVER_ERROR));
} else {
// TODO: change to validateDetector method when we support HC historical detector
searchAdInputIndices(detectorId);
}
}, transportService, listener);

Choose a reason for hiding this comment

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

I see the TODO for the change of validateDetector. Can it be done in this PR?

Copy link
Owner Author

@ylwu-amzn ylwu-amzn Jan 15, 2021

Choose a reason for hiding this comment

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

The validateDetector is only for HC detector. It will not validate single entity detector. We only support single entity historical detector currently.
Will change this method name to make it clear.


import com.amazon.opendistroforelasticsearch.ad.constant.CommonValue;

public class SearchADTasksAction extends ActionType<SearchResponse> {

Choose a reason for hiding this comment

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

What's the use case to call this api?

Copy link
Owner Author

Choose a reason for hiding this comment

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

For current phase, this is only used for Ops purpose. User can use this to search historical tasks as we don't show task level data on Kibana.
From UX, we may develop task management page on Kibana in future, then we can use this API to search tasks and show them on Kibana.

@codecov-io
Copy link

codecov-io commented Jan 15, 2021

Codecov Report

Merging #1 (9a9e896) into pr_stop_task (4b03a4f) will not change coverage.
The diff coverage is 61.70%.

Impacted file tree graph

@@               Coverage Diff               @@
##             pr_stop_task       #1   +/-   ##
===============================================
  Coverage           77.29%   77.29%           
  Complexity           2279     2279           
===============================================
  Files                 214      214           
  Lines               10242    10242           
  Branches              909      909           
===============================================
  Hits                 7917     7917           
  Misses               1906     1906           
  Partials              419      419           
Flag Coverage Δ Complexity Δ
cli 79.27% <ø> (ø) 0.00 <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ Complexity Δ
...ransport/SearchAnomalyDetectorTransportAction.java 48.38% <ø> (ø) 4.00 <0.00> (ø)
.../transport/SearchAnomalyResultTransportAction.java 48.38% <ø> (ø) 4.00 <0.00> (ø)
...ransport/DeleteAnomalyDetectorTransportAction.java 21.68% <30.00%> (ø) 3.00 <0.00> (ø)
...transport/IndexAnomalyDetectorTransportAction.java 72.72% <80.00%> (ø) 11.00 <0.00> (ø)
...est/handler/IndexAnomalyDetectorActionHandler.java 51.75% <82.35%> (ø) 31.00 <9.00> (ø)
...stroforelasticsearch/ad/AnomalyDetectorPlugin.java 95.08% <100.00%> (ø) 11.00 <0.00> (ø)
...stroforelasticsearch/ad/model/AnomalyDetector.java 94.24% <100.00%> (ø) 84.00 <4.00> (ø)
... and 4 more

ylwu-amzn and others added 3 commits January 15, 2021 13:21
…pendistro-for-elasticsearch#359)

* stop historical detector; support return AD task in get detector API

* change to scaling executor

* remove detector cache; handle node crash

* add more comments/logs; tune method name
@ylwu-amzn
Copy link
Owner Author

Close this PR as we have released historical detector.

@ylwu-amzn ylwu-amzn closed this May 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants