-
Notifications
You must be signed in to change notification settings - Fork 36
add AD task and tune detector&AD result data model #329
add AD task and tune detector&AD result data model #329
Conversation
Codecov Report
@@ Coverage Diff @@
## master #329 +/- ##
============================================
+ Coverage 74.39% 74.90% +0.51%
- Complexity 2022 2133 +111
============================================
Files 201 206 +5
Lines 9563 9999 +436
Branches 852 892 +40
============================================
+ Hits 7114 7490 +376
- Misses 2019 2088 +69
+ Partials 430 421 -9
Flags with carried forward coverage won't be shown. Click here to find out more. |
src/main/java/com/amazon/opendistroforelasticsearch/ad/model/AnomalyDetector.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.
No major concerns from me.
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.
just double confirm. for the schema change, we will have a test plan for it, right? We always need be cautious for any schema related change
src/main/java/com/amazon/opendistroforelasticsearch/ad/model/ADTaskState.java
Show resolved
Hide resolved
src/main/java/com/amazon/opendistroforelasticsearch/ad/model/AnomalyDetectorType.java
Outdated
Show resolved
Hide resolved
src/main/java/com/amazon/opendistroforelasticsearch/ad/model/DetectionDateRange.java
Outdated
Show resolved
Hide resolved
Yes, agree. Have done backward compatibility test based on previous test plans to make sure this change not break current functions. And I have added more test cases for detector and results, improved the branch coverage to >90% and fixed some bugs in old code. |
} | ||
|
||
private static boolean isRealTimeDetector(DetectionDateRange detectionDateRange) { | ||
return detectionDateRange == null || detectionDateRange.getEndTime() == null; |
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.
minor: endTime can't be null, right?
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.
Yes, for initial release, we plan to separate historical detector and realtime detector flow, so it's true the endTime can't be null. Next phase we will build universal flow, then the endTime could be null. If the endTime is null, we will pre-train model with historical data from startTime to now, then start realtime detection with the pre-trained model.
Issue #, if available:
Description of changes:
Currently we only have realtime detector which can take streaming data and detect anomaly in realtime way. For historical detector, user can feed historical data and detect historical anomalies. When user start historical detector, we will start an AD task at backend and run it asynchronously. This PR is to add AD task and tune detector and AD result to support historical detector.
AD Task
When user start a historical detector, we will create a new AD task. We will record task state, progress and detector snapshot in task.
Detector changes
detection_date_range
andcatetory_field
to decide detector type automatically.AD result changes
Test
1.
./gradlew build
2.
./gradlew integTest -PnumNodes=3
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.