[Feature/extensions] Migrates Start Detector #837

joshpalis · 2023-03-07T17:47:31Z

Description

Migrates the AnomalyDetectorJobTransportAction, RestAnomalyDetectorJobAction, and other supporting classes from plugin to extension.

This PR implements the following :

AnomalyDetectorJobTransportAction/RestAnomalyDetectorJobAction - which is used for both stop and start detector
Integrates the Job Scheduler Communication Mechanism. This includes registering job details, replacing the lockService with rest calls to job scheduler and registers the ADJobParameterTransportAction/ADJobRunnerTransportAction to facilitate job index entry parsing and job execution.
Instantiation of the following metadata indices : .opendistro-anomaly-detection-state, .opendistro-anomaly-results, .opendistro-anomaly-detector-job, .opensearch-job-scheduler-job-details, and .opendistro-job-scheduler-lock
Enables the creation, configuration, and execution of anomaly detector jobs

At the state of this PR, when a job is executed, this will continue running in an infinite loop, as the job execution workflow ends when AnomalyDetectorJobRunner.runAnomalyDetectionJob is invoked, which is commented out and will be integrated with the SDK in this issue : opensearch-project/opensearch-sdk-java#626. This eventually calls the AnomalyResultAction which indexes the anomaly result of the detector and releases the lock and thus ending the job. Subsequent attempts of Job Scheduler to execute the next run of the same Job will generate an anomaly result exception document due to a null lock, since the first execution has not yet released the lock. Indexing this anomaly result exception then instantiates the default anomaly result index.

The full workflow of the communication between Job Scheduler and Anomaly Detection Extension can be seen here : opensearch-project/opensearch-sdk-java#427

Issues Resolved

opensearch-project/opensearch-sdk-java#383

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.

Signed-off-by: Joshua Palis <[email protected]>

…tion Signed-off-by: Joshua Palis <[email protected]>

…ction Signed-off-by: Joshua Palis <[email protected]>

Signed-off-by: Joshua Palis <[email protected]>

…g AnomalyDetectorJobTransportAction class to jacoco exclusions Signed-off-by: Joshua Palis <[email protected]>

codecov-commenter · 2023-03-07T18:36:23Z

Codecov Report

Merging #837 (bb740db) into feature/extensions (ec858b2) will decrease coverage by 4.98%.
The diff coverage is 3.06%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@                   Coverage Diff                    @@
##             feature/extensions     #837      +/-   ##
========================================================
- Coverage                 39.89%   34.92%   -4.98%     
+ Complexity                 2111     1865     -246     
========================================================
  Files                       297      298       +1     
  Lines                     17113    17510     +397     
  Branches                   1843     1857      +14     
========================================================
- Hits                       6827     6115     -712     
- Misses                     9799    10967    +1168     
+ Partials                    487      428      -59

Flag	Coverage Δ
plugin	`34.92% <3.06%> (-4.98%)`	⬇️

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

Impacted Files	Coverage Δ
...va/org/opensearch/ad/AnomalyDetectorExtension.java	`0.00% <0.00%> (ø)`
...va/org/opensearch/ad/AnomalyDetectorJobRunner.java	`0.00% <0.00%> (ø)`
.../main/java/org/opensearch/ad/NodeStateManager.java	`0.76% <ø> (-63.08%)`	⬇️
.../java/org/opensearch/ad/caching/PriorityCache.java	`0.00% <ø> (ø)`
.../java/org/opensearch/ad/ratelimit/BatchWorker.java	`100.00% <ø> (ø)`
.../opensearch/ad/ratelimit/CheckpointReadWorker.java	`0.72% <ø> (-86.96%)`	⬇️
...opensearch/ad/ratelimit/CheckpointWriteWorker.java	`93.82% <ø> (ø)`
.../org/opensearch/ad/ratelimit/ColdEntityWorker.java	`95.45% <ø> (ø)`
.../org/opensearch/ad/ratelimit/ConcurrentWorker.java	`68.00% <ø> (-8.00%)`	⬇️
...opensearch/ad/ratelimit/EntityColdStartWorker.java	`100.00% <ø> (ø)`
... and 26 more

... and 26 files with indirect coverage changes

… AD_JOB_TYPE to AnomalyDetectorExtension Signed-off-by: Joshua Palis <[email protected]>

…omalyDetectorJobAction Signed-off-by: Joshua Palis <[email protected]>

Signed-off-by: Joshua Palis <[email protected]>

…eating NodeFilterer Signed-off-by: Joshua Palis <[email protected]>

Signed-off-by: Joshua Palis <[email protected]>

…bRunner. Modifies updateLatestFlagofOldTasksAndCreateNewTask() to use a blocking rest request instead of client.execute() Signed-off-by: Joshua Palis <[email protected]>

Signed-off-by: Joshua Palis <[email protected]>

…/ create components. Migrates start detector workflow invocations of opensearch actions with corresponding opensearchAsyncClient methods Signed-off-by: Joshua Palis <[email protected]>

Signed-off-by: Joshua Palis <[email protected]>

…aAsyncClient as part of create components to close later along with the sdkRestClient Signed-off-by: Joshua Palis <[email protected]>

…duler rather than the action name Signed-off-by: Joshua Palis <[email protected]>

Signed-off-by: Joshua Palis <[email protected]>

owaiskazi19

Great work with the migration and integrating job scheduler! Good to see the workflow.

src/main/java/org/opensearch/ad/AnomalyDetectorJobRunner.java

src/main/java/org/opensearch/ad/indices/AnomalyDetectionIndices.java

src/main/java/org/opensearch/ad/task/ADTaskManager.java

src/main/java/org/opensearch/ad/transport/ADJobRunnerTransportAction.java

build.gradle

…tchAll Signed-off-by: Joshua Palis <[email protected]>

Signed-off-by: Joshua Palis <[email protected]>

…ndex Signed-off-by: Joshua Palis <[email protected]>

Signed-off-by: Joshua Palis <[email protected]>

src/main/java/org/opensearch/ad/indices/AnomalyDetectionIndices.java

Signed-off-by: Joshua Palis <[email protected]>

dbwiddis

LGTM with some observations and an agreement about a rename that you can consider now or later when we re-branch and merge.

dbwiddis · 2023-03-31T23:41:44Z

src/main/java/org/opensearch/ad/AnomalyDetectorExtension.java

 return ImmutableList
- .of(sdkRestClient, anomalyDetectionIndices, jvmService, adCircuitBreakerService, adTaskManager, adTaskCacheManager);
+ .of(


Just a comment as I haven't checked. Ideally the order here and in code above defining these matches the AD Plugin, to minimize the diff.

yes I've followed it as closely as I could. There are some components that have not been implemented yet, since I havent needed them yet. But overall, the order in which the components are created and returned is the same as ADPlugin

dbwiddis · 2023-03-31T23:43:09Z

src/main/java/org/opensearch/ad/AnomalyDetectorExtension.java

+ Math.max(1, OpenSearchExecutors.allocatedProcessors(settings) / 2),
+ TimeValue.timeValueMinutes(10),
+ AD_THREAD_POOL_PREFIX + AD_THREAD_POOL_NAME
+ )// ,


(Style) Would really prefer block comment format-preserving comments here. (Start with /*- and keep the indentation in the middle before the */.

Ill go ahead and fix this

dbwiddis · 2023-03-31T23:46:21Z

src/main/java/org/opensearch/ad/indices/ADIndex.java

+ AnomalyDetectorJob.ANOMALY_DETECTOR_JOB_INDEX,
+ false,
+ ThrowingSupplierWrapper.throwingSupplierWrapper(AnomalyDetectionIndices::getAnomalyDetectorJobMappings)
+ ),


No complaint here, just pointing to the diff above as a reason we shouldn't be using // comments on blocks of code :)

src/main/java/org/opensearch/ad/indices/AnomalyDetectionIndices.java

dbwiddis · 2023-03-31T23:52:25Z

src/main/java/org/opensearch/ad/rest/handler/IndexAnomalyDetectorJobActionHandler.java

-// }
-//
-// }
+/**


UGH, this diff!!!!

I don't even know what changed!

Signed-off-by: Joshua Palis <[email protected]>

owaiskazi19

Almost there. Few more nitpicks and questions

src/main/java/org/opensearch/ad/AnomalyDetectorExtension.java

src/main/java/org/opensearch/ad/AnomalyDetectorJobRunner.java

owaiskazi19 · 2023-04-03T17:23:16Z

src/main/java/org/opensearch/ad/AnomalyDetectorJobRunner.java

+ releaseLockRequest.addParameter(LockModel.LOCK_ID, lock.getLockId());
+
+ try {
+ Response releaseLockResponse = client.performRequest(releaseLockRequest);


Is performRequest async?

performRequest is a blocking call. There is currently no async mechanism to send a rest call via the SDKRestClient.

@joshpalis let's add async in SDK and use it here. This can be done in the next PR.

src/main/java/org/opensearch/ad/indices/AnomalyDetectionIndices.java

owaiskazi19 · 2023-04-03T17:25:49Z

src/main/java/org/opensearch/ad/ml/CheckpointDao.java

+ try {
+ CompletableFuture<DeleteByQueryResponse> deleteResponse = sdkJavaAsyncClient.deleteByQuery(deleteRequest);
+ DeleteByQueryResponse response = deleteResponse.orTimeout(10L, TimeUnit.SECONDS).get();
+ if (response.timedOut() || !response.failures().isEmpty()) {


We should test this whenever we uncomment the tests and also can you create an issue on opensearch-java for getBulkFailures?

owaiskazi19 · 2023-04-03T17:32:00Z

src/main/java/org/opensearch/ad/ml/CheckpointDao.java

+ logger.warn(BULK_FAILURE_LOG_MSG + " {}", detectorID);
+ for (BulkIndexByScrollFailure bulkFailure : response.failures()) {
+ logger.warn(bulkFailure);
+ }


This is a little confusing to me. We have BulkIndexByScrollFailure but old code has ScrollableHitSource which is use for the scrollable results. @saratvemulapalli need some inputs here to verify the changes in this file

@VachaShah just wanted to get your input on this. Please correct me if I am wrong, but as we discussed before, the DeleteByQueryResponse failures for opensearch-java's OpenSearchAsyncClient must be read from the list of BulkIndexByScrollFailure and there are no corresponding types for the HighLevelRestClient's SearchFailure and BulkFailure.

Yes for DeleteByQueryResponse in the java-client, the failures need to be read from the list of BulkByIndexScrollFailures.

src/main/java/org/opensearch/ad/task/ADTaskManager.java

src/main/java/org/opensearch/ad/transport/ADJobRunnerTransportAction.java

Signed-off-by: Joshua Palis <[email protected]>

owaiskazi19

Looking for some inputs from @VachaShah! If any code change is required @joshpalis you can handle it in the next PR!

joshpalis added 13 commits March 2, 2023 22:04

Initial start detector commit, sans test classes

8d0cee5

Signed-off-by: Joshua Palis <[email protected]>

Adding XContentParser import to IndexAnomalyDetectorJobActionHandler

9d40a94

Signed-off-by: Joshua Palis <[email protected]>

spotlessApply

a663f12

Signed-off-by: Joshua Palis <[email protected]>

Changing inject dependency to guice for AnomalyDetectorJobTransportAc…

ae5cd1f

…tion Signed-off-by: Joshua Palis <[email protected]>

Pulling settings from extensionRunner in AnomalyDetectorJobTransportA…

d5840be

…ction Signed-off-by: Joshua Palis <[email protected]>

Making AnomalyDetectorJobTransportAction doExecute protected

b6b139d

Signed-off-by: Joshua Palis <[email protected]>

Adding RestAnomalyDetectorJobAction to getExtensionRestHandlers

59cff9c

Signed-off-by: Joshua Palis <[email protected]>

Updating test constructors for ADJobRunner/JobParameter

c8e6b86

Signed-off-by: Joshua Palis <[email protected]>

merge feature/extensions

d5fa24e

Signed-off-by: Joshua Palis <[email protected]>

fixing merge conflict, spotlessApply

8e24c6f

Signed-off-by: Joshua Palis <[email protected]>

adding restClient()

1ba5852

Signed-off-by: Joshua Palis <[email protected]>

Merge feature/extensions into startdetector

333153c

Signed-off-by: Joshua Palis <[email protected]>

uncommenting AnomalyDetectorJobTransportAction test classes and addin…

076d14c

…g AnomalyDetectorJobTransportAction class to jacoco exclusions Signed-off-by: Joshua Palis <[email protected]>

joshpalis added 13 commits March 8, 2023 01:11

Added job details registration to RestAnomalyDetectorJobAction, added…

a4a7e9f

… AD_JOB_TYPE to AnomalyDetectorExtension Signed-off-by: Joshua Palis <[email protected]>

Triggering job details registration with Job Scheduler within RestAn…

509e3c4

…omalyDetectorJobAction Signed-off-by: Joshua Palis <[email protected]>

Inital Job Scheduler communication mechanism integration commit

33610e8

Signed-off-by: Joshua Palis <[email protected]>

Fixing SDKRestClient null action map, commenting out test classes, cr…

ca3914c

…eating NodeFilterer Signed-off-by: Joshua Palis <[email protected]>

Adding class names of commented out test classes to jacocoExclusions

2df0224

Signed-off-by: Joshua Palis <[email protected]>

jacocoExclusion

089d334

Signed-off-by: Joshua Palis <[email protected]>

Integrates AcquireLockRequest/ReleaseLockRequest in AnomalyDetectorJo…

1e097c9

…bRunner. Modifies updateLatestFlagofOldTasksAndCreateNewTask() to use a blocking rest request instead of client.execute() Signed-off-by: Joshua Palis <[email protected]>

spotlessApply

384d4f8

Signed-off-by: Joshua Palis <[email protected]>

Adds OpenSearchAsyncClient to AnomalyDetectorExtension

2832875

Signed-off-by: Joshua Palis <[email protected]>

Adds OpenSearchAsyncClient to AnomalyDetectorExtension / rest actions…

1ed4a3f

…/ create components. Migrates start detector workflow invocations of opensearch actions with corresponding opensearchAsyncClient methods Signed-off-by: Joshua Palis <[email protected]>

Removing logs from RestAnomalyDetectorJobAction

668d7e1

Signed-off-by: Joshua Palis <[email protected]>

Merge branch 'feature/extensions' into startdetector

7a62ef1

Signed-off-by: Joshua Palis <[email protected]>

Renamed opensearchAsyncClient to sdkJavaAsyncClient. Returning sdkJav…

e0e1bb0

…aAsyncClient as part of create components to close later along with the sdkRestClient Signed-off-by: Joshua Palis <[email protected]>

joshpalis force-pushed the startdetector branch from 7aee890 to e0e1bb0 Compare March 23, 2023 18:56

joshpalis mentioned this pull request Mar 23, 2023

[Feature/extensions] Migrates Stop Detector #845

Merged

Sending fully qualified class names of ADJobRunner/Parser to Job Sche…

7867c97

…duler rather than the action name Signed-off-by: Joshua Palis <[email protected]>

Adding more classes to jacoco test coverage

89c4db2

Signed-off-by: Joshua Palis <[email protected]>

joshpalis marked this pull request as ready for review March 30, 2023 23:11

joshpalis requested review from a team, owaiskazi19, vibrantvarun and dbwiddis March 30, 2023 23:11

joshpalis mentioned this pull request Mar 31, 2023

Migrate Search Detector result to SDK using SDKRestClient opensearch-project/opensearch-sdk-java#377

Closed

4 tasks

owaiskazi19 requested changes Mar 31, 2023

View reviewed changes

joshpalis added 4 commits March 31, 2023 21:04

Addressing PR comments, still need to work on replacing match with ma…

2a84fd1

…tchAll Signed-off-by: Joshua Palis <[email protected]>

Removing stray semicolon

cc37a69

Signed-off-by: Joshua Palis <[email protected]>

Replacing string with ADJobIndex constant for initAnomalyDetectorJobI…

1a593f4

…ndex Signed-off-by: Joshua Palis <[email protected]>

Fixing updateByQueryRequests

c40b7b8

Signed-off-by: Joshua Palis <[email protected]>

owaiskazi19 reviewed Mar 31, 2023

View reviewed changes

src/main/java/org/opensearch/ad/indices/AnomalyDetectionIndices.java Outdated Show resolved Hide resolved

owaiskazi19 reviewed Mar 31, 2023

View reviewed changes

src/main/java/org/opensearch/ad/indices/AnomalyDetectionIndices.java Outdated Show resolved Hide resolved

Fixing DeleteByQueryRequests in ADTaskManager

0ac4e4d

Signed-off-by: Joshua Palis <[email protected]>

dbwiddis approved these changes Mar 31, 2023

View reviewed changes

joshpalis added 4 commits April 1, 2023 00:00

Using adminClient to call .indices() in AnomalyDetectionIndices

9a26880

Signed-off-by: Joshua Palis <[email protected]>

Fixing comments

a45d303

Signed-off-by: Joshua Palis <[email protected]>

Fixing failing tests that havent failed before, weird

5002340

Signed-off-by: Joshua Palis <[email protected]>

spotless

0380d9a

Signed-off-by: Joshua Palis <[email protected]>

owaiskazi19 reviewed Apr 3, 2023

View reviewed changes

joshpalis added 2 commits April 3, 2023 19:05

Addressing PR comments

06a1fab

Signed-off-by: Joshua Palis <[email protected]>

Fixing OpenSearchRejectedExecutionException import

60a2348

Signed-off-by: Joshua Palis <[email protected]>

joshpalis force-pushed the startdetector branch from ff52de5 to 60a2348 Compare April 3, 2023 19:45

Addressing PR comments, changing ES to OS

bb740db

Signed-off-by: Joshua Palis <[email protected]>

owaiskazi19 approved these changes Apr 3, 2023

View reviewed changes

owaiskazi19 merged commit bf7c75a into opensearch-project:feature/extensions Apr 3, 2023

minalsha mentioned this pull request Apr 26, 2023

Migrate Start Detector job to SDK using SDKRestClient opensearch-project/opensearch-sdk-java#383

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature/extensions] Migrates Start Detector #837

[Feature/extensions] Migrates Start Detector #837

joshpalis commented Mar 7, 2023 •

edited

Loading

codecov-commenter commented Mar 7, 2023 •

edited

Loading

owaiskazi19 left a comment

dbwiddis left a comment

dbwiddis Mar 31, 2023

joshpalis Apr 1, 2023

dbwiddis Mar 31, 2023

joshpalis Apr 1, 2023

dbwiddis Mar 31, 2023

dbwiddis Mar 31, 2023

owaiskazi19 left a comment

owaiskazi19 Apr 3, 2023

joshpalis Apr 3, 2023

owaiskazi19 Apr 3, 2023

owaiskazi19 Apr 3, 2023

owaiskazi19 Apr 3, 2023

joshpalis Apr 3, 2023

VachaShah Apr 4, 2023

owaiskazi19 left a comment

[Feature/extensions] Migrates Start Detector #837

[Feature/extensions] Migrates Start Detector #837

Conversation

joshpalis commented Mar 7, 2023 • edited Loading

Description

Issues Resolved

codecov-commenter commented Mar 7, 2023 • edited Loading

Codecov Report

owaiskazi19 left a comment

Choose a reason for hiding this comment

dbwiddis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

owaiskazi19 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

owaiskazi19 left a comment

Choose a reason for hiding this comment

joshpalis commented Mar 7, 2023 •

edited

Loading

codecov-commenter commented Mar 7, 2023 •

edited

Loading