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.

Sign up for GitHub

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

Communication mechanism JS (Part 1) #761

Closed
wants to merge 2 commits into from
Closed

Communication mechanism JS (Part 1) #761

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
20710d4
Fetch Job Details from AD extension
vibrantvarun Dec 15, 2022
81b8dbd
Commnunication Mechanism for JS
vibrantvarun Dec 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/main/java/org/opensearch/ad/AnomalyDetectorExtension.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import org.opensearch.client.opensearch.OpenSearchClient;
import org.opensearch.common.settings.Setting;
import org.opensearch.common.xcontent.NamedXContentRegistry;
import org.opensearch.extensions.JobDetails;
import org.opensearch.sdk.BaseExtension;
import org.opensearch.sdk.ExtensionRestHandler;
import org.opensearch.sdk.ExtensionsRunner;
Expand Down Expand Up @@ -111,6 +112,12 @@ public OpenSearchClient getClient() {
return client;
}

// TODO: After AnomalyDetectorJob.java is uncommented, fetch jobIndex from AnomalyDetectorJob
@Override
public JobDetails getJobDetails() {
return new JobDetails(AnomalyDetectorPlugin.AD_JOB_TYPE, ".opendistro-anomaly-detector-jobs");
Copy link
Member

@owaiskazi19 owaiskazi19 Dec 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

We should use Job index constant here ANOMALY_DETECTOR_JOB_INDEX.
Also, getJobDetails is equivalent to which JobScheduler extension point here?
For ADplugin, we have getJobIndex() and getJobType()

Copy link
Member Author

Choose a reason for hiding this comment

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

I have not pulled ANOMALY_DETECTOR_JOB_INDEX from AnomalyDetectorJob because the class is commented. I have mentioned the same in TODO

Copy link
Member

Choose a reason for hiding this comment

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

@owaiskazi19 JobDetails is equivalent to both getJobIndex() and getJobType(). Both these strings are utilized by Job Scheduler to configure it's behavior for onIndexModule() and to also configure the indexToJobProviders which the JobSweeper uses to schedule jobs. I also agree that we should use the ANOMALY_DETECTOR_JOB_INDEX constant, @vibrantvarun it should be fine to uncomment this

Copy link
Member

@owaiskazi19 owaiskazi19 Dec 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I want to understand more on why are we coupling up getJobIndex() and getJobType() together in jobDetails?Shouldn't it follow the old design of keeping them separate or otherwise we have to change these extension points in all the plugins/extensions using JS?

Copy link
Member

@joshpalis joshpalis Dec 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

getJobIndex and getJobType are currently used by Job Scheduler to retrieve implementations of the JobSchedulerExtension interface here. Simply put, these two extension points just provide strings to Job Scheduler.

We're coupling the Job Index and Job Type strings together in one request, rather than separate requests since this is the only initial information we need from extensions in order to configure both the indicesToListen set and indexToJobProviders hashmap. How these are further used is documented here. Separating these into two requests would be unnecessary.

This wouldn't have any affect on how plugins are using JS, since the plugin architecture for JS remains untouched within PluginsService's loadExtensionsForPlugin workflow. As for extensions, the only difference at this point is how we are providing these strings to Job Scheduler. How Job Scheduler uses these strings will remain unchanged.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @joshpalis @owaiskazi19 the job index cannot be pulled from AnomalyDetectorJob.java as of now because it still has circular dependency from JS on some other methods written in the file. If I uncomment only the required code like the Job Index initialization then I am facing an error of jacoco test coverage report. Therefore, for now I am hardcoding the value of job index.

Copy link
Member

@owaiskazi19 owaiskazi19 Dec 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

We can fix the jacoco test coverage by uncommenting the tests too

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes I did uncomment the code, but there are some dependencies which are imported from JobScheduler like Schedule.java etc. These dependencies cannot be resolved until they are imported from JS native plugin.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense. Let's create a constant in this file then

Copy link
Member Author

Choose a reason for hiding this comment

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

ACK

}

public static void main(String[] args) throws IOException {
// Execute this extension by instantiating it and passing to ExtensionsRunner
ExtensionsRunner.run(new AnomalyDetectorExtension());
Expand Down