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

Communication mechanism JS (Part 1) #761

Closed
wants to merge 2 commits into from
Closed

Communication mechanism JS (Part 1) #761

wants to merge 2 commits into from

Conversation

vibrantvarun
Copy link
Member

@vibrantvarun vibrantvarun commented Dec 15, 2022

Description

Request Response model to fetch Job details like job index and job type from extensions.

Issues Resolved

opensearch-project/opensearch-sdk-java#274

Companion PR

OpenSearch opensearch-project/OpenSearch#5586

opensearch-sdk-java opensearch-project/opensearch-sdk-java#289
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.

@vibrantvarun vibrantvarun requested a review from a team December 15, 2022 21:54
// 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

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

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

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

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

@owaiskazi19
Copy link
Member

DCO check is failing. Commit using -s flag. Thanks

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.

4 participants