https://github.com/opensearch-project/job-scheduler/pull/351

[joshpalis]

Description

Coming from modifications to the ExtensionProxyAction workflow here.

The JobDetailsService used to facilitate job execution for extensions creates proxy ScheduledJobRunner and ScheduledJobParser objects that utilize the node client to execute remote extension actions.

Here is an example of the new workflow :

1. An extension registers its Job Details and Job Scheduler creates a ScheduledJobProvider, which contains a ScheduledJobParser that delegates parsing of the extension job index entry to the extension by executing the extension parse action
2. Job Scheduler is notified of an index operation on the extension job index, pulls that entry and it's proxy ScheduledJobParser invokes ExtensionProxyAction to send this to the extension to parse
3. The ExtensionProxyJobAction attaches the fully qualified class name of the ExtensionActionRequest as bytes in the responseByte field, along with the bytes provided by the inner JobParserRequest that contains the job index entry source
4. This request is sent to the SDK's REQUEST_EXTENSION_HANDLE_TRANSPORT_ACTION handler
5. This request is then handled by the ExtensionActionRequestHandler here, which delegates the request handling to the handleRemoteExtensionActionRequest method.
6. Within this method, the requestBytes are processed and the request class name is identified here which is used to identify and create the constructor for this particular ActionRequest
7. The action is then executed using the SDKClient and this request is forwarded to the corresponding TransportAction
8. The response is then sent back to Job Scheduler

This PR ensures that request bytes for the ExtensionActionRequest sent from Job Scheduler are in the correct format to be processed by the SDK.

Check List

• New functionality includes testing.
  • All tests pass
• New functionality has been documented.
  • New functionality has javadoc added
• Commits are signed per the DCO using --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.

[codecov-commenter]

Codecov Report

Merging #349 (9b77de8) into main (00bd2d4) will decrease coverage by 0.42%.
The diff coverage is 0.00%.

📣 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              @@
##               main     #349      +/-   ##
============================================
- Coverage     29.12%   28.70%   -0.42%     
  Complexity       97       97              
============================================
  Files            23       23              
  Lines          1171     1188      +17     
  Branches        109      110       +1     
============================================
  Hits            341      341              
- Misses          809      826      +17     
  Partials         21       21              

Impacted Files	Coverage Δ
...r/transport/request/ExtensionJobActionRequest.java	0.00% <0.00%> (ø)
...heduler/transport/request/JobParameterRequest.java	0.00% <0.00%> (ø)
...bscheduler/transport/request/JobRunnerRequest.java	0.00% <0.00%> (ø)
...transport/response/ExtensionJobActionResponse.java	0.00% <0.00%> (ø)
...ensearch/jobscheduler/utils/JobDetailsService.java	2.57% <ø> (+0.05%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[dbwiddis]

LGTM for working code.

But I see multiple copies of the same utility code for parsing byte arrays associated with these remote calls, and they should probably find a home in ExtensionTransportAction or maybe a new ExtensionTransportUtil class.

OK to merge this as is but if anyone else thinks like I do let's make an issue to do this.

[dbwiddis]

I wonder if we should make this a constant in the ExtensionTransportAction.

[dbwiddis]

I wonder if we should make this a util method in ExtensionTransportAction

[dbwiddis]

Note that nullPos was left over from when this was a null byte, and maybe should just be a pos.

[dbwiddis]

I used to always do this, but this is the method being invoked:

    @Override
    public void flush() {
        // nothing to do
    }

[dbwiddis]

I wonder if this should be a util method on ExtensionActionRequest or ExtensionTransportAction

[dbwiddis]

Deja vu. We definitely should find a central location for these util classes that both Extensions and Plugins can use.

[joshpalis]

@dbwiddis I agree, lets move these methods over to ExtensionActionRequest/ExtensionTransportAction. For now lets merge this change in until opensearch-project/opensearch-sdk-java#607 is resolved. Once those utilities are available then I'll make the change to Job Scheduler to use those instead.

[vibrantvarun]

LGTM!