-
Notifications
You must be signed in to change notification settings - Fork 80
Adding create and start to AD node client #1611
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
base: main
Are you sure you want to change the base?
Conversation
e88bc67 to
a6d8027
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1611 +/- ##
============================================
- Coverage 81.42% 81.33% -0.10%
- Complexity 6161 6170 +9
============================================
Files 542 542
Lines 24994 25101 +107
Branches 2543 2551 +8
============================================
+ Hits 20351 20415 +64
- Misses 3378 3409 +31
- Partials 1265 1277 +12
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| void suggestAnomalyDetector(SuggestConfigParamRequest suggestRequest, ActionListener<SuggestConfigParamResponse> listener); | ||
|
|
||
| /** | ||
| * Create anomaly detector - refer to https://opensearch.org/docs/latest/observing-your-data/ad/api/#create-detector |
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.
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.
changed
| } | ||
|
|
||
| /** | ||
| * Create anomaly detector - refer to https://opensearch.org/docs/latest/observing-your-data/ad/api/#create-detector |
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.
same issue
| void createAnomalyDetector(IndexAnomalyDetectorRequest createRequest, ActionListener<IndexAnomalyDetectorResponse> listener); | ||
|
|
||
| /** | ||
| * Start anomaly detector - refer to https://opensearch.org/docs/latest/observing-your-data/ad/api/#start-detector |
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.
build.gradle
Outdated
|
|
||
| testImplementation "org.opensearch.test:framework:${opensearch_version}" | ||
|
|
||
| zipArchive("org.opensearch.plugin:opensearch-ml-plugin:${opensearch_build}") |
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.
why do you need ml-plugin?
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.
removing this, left from testing
| this.maxCategoricalFields = maxCategoricalFields; | ||
| } | ||
|
|
||
| public IndexAnomalyDetectorRequest(String detectorID, AnomalyDetector detector, RestRequest.Method method) { |
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.
Where do you call this constructor?
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.
from skills repo on creating an anomaly detector from create ad tool, will have PR for that by tomorrow
|
|
||
| // Initialize cluster settings for node client requests | ||
| this.maxSingleEntityAnomalyDetectors = AnomalyDetectorSettings.AD_MAX_SINGLE_ENTITY_ANOMALY_DETECTORS.get(settings); | ||
| this.maxMultiEntityAnomalyDetectors = AnomalyDetectorSettings.AD_MAX_HC_ANOMALY_DETECTORS.get(settings); | ||
| this.maxAnomalyFeatures = AnomalyDetectorSettings.MAX_ANOMALY_FEATURES.get(settings); | ||
| this.maxCategoricalFields = ADNumericSetting.maxCategoricalFields(); | ||
|
|
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.
Why do we need these changes?
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.
so we can access settings on a transport request call that doesn't go through rest layer at first
| this.maxSingleEntityAnomalyDetectors = AnomalyDetectorSettings.AD_MAX_SINGLE_ENTITY_ANOMALY_DETECTORS.get(settings); | ||
| this.maxMultiEntityAnomalyDetectors = AnomalyDetectorSettings.AD_MAX_HC_ANOMALY_DETECTORS.get(settings); | ||
| this.maxAnomalyFeatures = AnomalyDetectorSettings.MAX_ANOMALY_FEATURES.get(settings); | ||
| this.maxCategoricalFields = ADNumericSetting.maxCategoricalFields(); | ||
|
|
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.
time series package should try to avoid cross-module dependency. These setting are not useful to forecast subclass.
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.
will change so we have ad with ad settings and forecaster with forecaster. We have some of those checks on Config level but i see we have different settings for some of them to supply that value. will fix
kaituo
left a comment
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.
partial review
| detector = new AnomalyDetector(in); | ||
| method = in.readEnum(RestRequest.Method.class); | ||
| requestTimeout = in.readTimeValue(); | ||
| maxSingleEntityAnomalyDetectors = in.readInt(); |
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.
This may cause bwc issue as if the caller sends request between new and old nodes.
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.
will make this optional
| return new IndexAnomalyDetectorResponse(namedWriteableAwareInput); | ||
| } | ||
| } catch (IOException e) { | ||
| throw new UncheckedIOException("failed to parse ActionResponse into IndexAnomalyDetectorResponse", e); |
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.
be consistent: IndexAnomalyDetectorRequest.fromActionRequest(...) throws IllegalArgumentException wrapping the IOException, while we throw UncheckedIOException here.
| return new IndexAnomalyDetectorResponse(namedWriteableAwareInput); | ||
| } | ||
| } catch (IOException e) { | ||
| throw new UncheckedIOException("failed to parse ActionResponse into IndexAnomalyDetectorResponse", e); |
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.
Also consider including the source class in the error message, e.g.:
"failed to parse " + actionRequest.getClass().getName() + " into IndexAnomalyDetectorRequest"
Signed-off-by: Amit Galitzky <[email protected]>
Signed-off-by: Amit Galitzky <[email protected]>
abd0c6f to
6538cf5
Compare
Description
Adding create and start to AD node client.
Involves creating new constructor for a few request types were we pass AD cluster settings from rest layer and utilizing cluster setting fetching in transport layer instead for plugin to plugin communication.
Check List
--signoff.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.