[Feature/extensions] Register REST API and forward REST requests to associated extension #4282

dbwiddis · 2022-08-23T00:56:40Z

Companion PR: opensearch-project/opensearch-sdk-java#101

Description

This and the companion PR linked above completes the work begun in #4100 by registering the received API with the RestController. When OpenSearch receives a RestRequest matching one of these registered routes, it forwards it to the appropriate extension for further action.

See the design schematic and description in #64

Steps 1 and 2 were already completed
[Feature/extensions] Provide Extension API to OpenSearch #4100 and #74 implemented step 3
This and the companion PR implement steps 4 through 8
Step 9 is outside the scope of this PR.

Issues Resolved

Closes #64, Closes #69, and Closes #70.

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.

Signed-off-by: Daniel Widdis <[email protected]>

github-actions · 2022-08-23T01:17:37Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/2010/
CommitID: 22393b71026604edbc8c11d3f3c430e27f790033

github-actions · 2022-08-23T01:28:10Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/2011/
CommitID: 6199e00

Signed-off-by: Daniel Widdis <[email protected]>

github-actions · 2022-08-23T17:51:37Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/2026/
CommitID: 7f525dd

Signed-off-by: Daniel Widdis <[email protected]>

github-actions · 2022-08-23T18:30:16Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/2027/
CommitID: ca15c35

Signed-off-by: Daniel Widdis <[email protected]>

github-actions · 2022-08-23T20:57:28Z

Gradle Check (Jenkins) Run Completed with:

RESULT: UNSTABLE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/2039/
CommitID: 254005b

Signed-off-by: Daniel Widdis <[email protected]>

github-actions · 2022-08-23T22:17:29Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/2040/
CommitID: b556070

github-actions · 2022-08-24T16:12:00Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/2073/
CommitID: b325f84a1408809a2efe10a6edef6539ef880274

Signed-off-by: Daniel Widdis <[email protected]>

github-actions · 2022-08-25T01:22:37Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/2100/
CommitID: 2bd9b167b3630d64f1bdc270ec3b6515515ad7cc

dbwiddis · 2022-08-25T01:26:42Z

@saratvemulapalli @owaiskazi19 this is ready for your review. I've added tests for everything I think I can; the remainder of things that aren't tested belong in an integration test. Let me know if you think otherwise:

The RestActionsRequestHandler registers actions with the RestController. There is no public method available on that class to confirm the registration and it is stored in a private field. It can only really be confirmed by sending a REST request and validating that it is accepted (which I have done manually).
The RestSendToExtensionAction (the single RestHandler that acts for all routes for an extension) has a prepareRequest method which implements the logic of the REST handler. It requires a working Client to receive the response, and isn't easily tested in a unit test.

Signed-off-by: Daniel Widdis <[email protected]>

github-actions · 2022-08-25T03:17:11Z

Gradle Check (Jenkins) Run Completed with:

RESULT: null ❌
URL: https://build.ci.opensearch.org/job/gradle-check/2101/
CommitID: 256453441ae67e0b484e88e6c777b2c90419c52e

github-actions · 2022-08-25T03:18:23Z

Gradle Check (Jenkins) Run Completed with:

RESULT: null ❌
URL: https://build.ci.opensearch.org/job/gradle-check/2102/
CommitID: 7ade781

github-actions · 2022-08-25T03:57:25Z

Gradle Check (Jenkins) Run Completed with:

RESULT: null ❌
URL: https://build.ci.opensearch.org/job/gradle-check/2103/
CommitID: ca5a29d

server/src/main/java/org/opensearch/extensions/rest/RestSendToExtensionAction.java

owaiskazi19 · 2022-08-26T07:49:24Z

server/src/main/java/org/opensearch/node/Node.java

@@ -1165,7 +1165,6 @@ public Node start() throws NodeValidationException {
            : "clusterService has a different local node than the factory provided";
        transportService.acceptIncomingRequests();
        extensionsOrchestrator.extensionsInitialize();
-        extensionsOrchestrator.setNamedWriteableRegistry();


Why are we removing this?

The actions it accomplishes are being moved into the initialize call immediately above it to remove the need for a public setter and more strongly enforce the correct sequencing.

server/src/main/java/org/opensearch/extensions/rest/RestSendToExtensionAction.java

owaiskazi19 · 2022-08-26T08:29:55Z

I see we have the logs messages on the issue to explain the workflow. Thanks for adding that. A small request on top of that can we have a small diagram/representation of the code workflow as well to get better understanding of the Register REST APIs?
Something like:

RestActionsRequestHandler() - handler for registering rest API's coming from Extension -> RestSendToExtensionAction() - action forwards request coming from user to extension

Signed-off-by: Daniel Widdis <[email protected]>

dbwiddis · 2022-08-26T16:23:46Z

I see we have the logs messages on the issue to explain the workflow. Thanks for adding that. A small request on top of that can we have a small diagram/representation of the code workflow as well to get better understanding of the Register REST APIs?

I updated the DESIGN.md in SDK #101 with a walk-through that identifies the classes involved. I'm not sure what added value another diagram would bring to that? Could you elaborate?

github-actions · 2022-08-26T16:34:19Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/2143/
CommitID: 7d2beca

owaiskazi19 · 2022-08-26T17:12:59Z

I updated the DESIGN.md in SDK #101 with a walk-through that identifies the classes involved. I'm not sure what added value another diagram would bring to that? Could you elaborate?

Personal preference to help understand the design from the diagram on the issue. You can ignore if you want to :)
I see you have updated the DESIGN.md with the classes. That helps too.

dbwiddis · 2022-08-26T19:04:31Z

Personal preference to help understand the design from the diagram on the issue. You can ignore if you want to :) I see you have updated the DESIGN.md with the classes. That helps too.

If you can wait until SDK #102 I expect to have a full cradle-to-grave diagram that I think will answer this, so not quite ignoring but deferring to the next PR.

github-actions · 2022-08-26T23:44:46Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/2165/
CommitID: 7d2beca

github.com:opensearch-project/OpenSearch into restController Signed-off-by: Daniel Widdis <[email protected]>

Signed-off-by: Daniel Widdis <[email protected]>

github-actions · 2022-08-27T04:36:11Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/2170/
CommitID: 885aeda

codecov-commenter · 2022-08-27T04:39:14Z

Codecov Report

Merging #4282 (dfad6d4) into feature/extensions (60bb959) will decrease coverage by 0.09%.
The diff coverage is 70.67%.

@@                   Coverage Diff                    @@
##             feature/extensions    #4282      +/-   ##
========================================================
- Coverage                 70.64%   70.54%   -0.10%     
- Complexity                57195    57275      +80     
========================================================
  Files                      4625     4633       +8     
  Lines                    275473   275817     +344     
  Branches                  40291    40337      +46     
========================================================
- Hits                     194601   194577      -24     
- Misses                    64567    64940     +373     
+ Partials                  16305    16300       -5

Impacted Files	Coverage Δ
...ava/org/opensearch/client/RestHighLevelClient.java	`44.02% <ø> (-0.31%)`	⬇️
...egations/bucket/composite/GeoTileValuesSource.java	`50.00% <ø> (ø)`
...aggregations/bucket/geogrid/BoundedCellValues.java	`100.00% <ø> (ø)`
...gregations/bucket/geogrid/BucketPriorityQueue.java	`75.00% <ø> (ø)`
...arch/aggregations/bucket/geogrid/CellIdSource.java	`69.23% <ø> (ø)`
...search/aggregations/bucket/geogrid/CellValues.java	`100.00% <ø> (ø)`
...ions/bucket/geogrid/GeoGridAggregationBuilder.java	`75.67% <ø> (ø)`
...aggregations/bucket/geogrid/GeoGridAggregator.java	`87.50% <ø> (ø)`
.../bucket/geogrid/GeoHashGridAggregationBuilder.java	`100.00% <ø> (ø)`
...egations/bucket/geogrid/GeoHashGridAggregator.java	`80.00% <ø> (ø)`
... and 554 more

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

Signed-off-by: Daniel Widdis <[email protected]>

github-actions · 2022-08-28T16:40:40Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/2173/
CommitID: dfad6d4

dbwiddis · 2022-08-29T04:40:09Z

Personal preference to help understand the design from the diagram on the issue. You can ignore if you want to :) I see you have updated the DESIGN.md with the classes. That helps too.

There is now a very pretty diagram as part of #109. Enjoy. 😉

saratvemulapalli · 2022-08-29T21:51:05Z

server/src/main/java/org/opensearch/extensions/rest/RestSendToExtensionAction.java

+            transportService.sendRequest(
+                discoveryExtension,
+                ExtensionsOrchestrator.REQUEST_REST_EXECUTE_ON_EXTENSION_ACTION,
+                new RestExecuteOnExtensionRequest(method, uri),
+                restExecuteOnExtensionResponseHandler
+            );
+            try {
+                inProgressLatch.await(5, TimeUnit.SECONDS);
+            } catch (InterruptedException e) {
+                return channel -> channel.sendResponse(
+                    new BytesRestResponse(RestStatus.REQUEST_TIMEOUT, "No response from extension to request.")
+                );
+            }
+        } catch (Exception e) {
+            logger.info("Failed to send REST Actions to extension " + discoveryExtension.getName(), e);
+        }
+        String response = responseBuilder.toString();
+        if (response.isBlank() || response.startsWith("FAILED")) {
+            return channel -> channel.sendResponse(
+                new BytesRestResponse(RestStatus.INTERNAL_SERVER_ERROR, response.isBlank() ? "Request Failed" : response)
+            );
+        }
+        return channel -> channel.sendResponse(new BytesRestResponse(RestStatus.OK, response));


Thanks for this @dbwiddis.
You've suggested an improvement to not let the user wait, could you open an issue if you haven't already to discuss a way to optimize it?

I'll think about it. We probably need to implement more RestActions and see how long-running jobs are already handled, so I'm thinking it'll pop up later when it's needed and we can address it then, along with many other incremental changes we'll need.

Really I think this is part of a bigger meta-issue regarding initialization, reachability, etc. SDK #94 touches on some related issues and it can probably be discussed there.

saratvemulapalli · 2022-08-29T21:51:39Z

Thanks @dbwiddis for the changes.
I am excited to see this rolling.

owaiskazi19

LGTM! Thanks @dbwiddis for all the changes and the design diagram!

New RestActionsRequestHandler class with RestController

de74f9d

Signed-off-by: Daniel Widdis <[email protected]>

dbwiddis mentioned this pull request Aug 23, 2022

Register REST API and forward REST requests to associated extension opensearch-project/opensearch-sdk-java#101

Merged

dbwiddis requested review from owaiskazi19 and saratvemulapalli August 23, 2022 00:59

Forward REST requests from OpenSearch to Extension

6199e00

Signed-off-by: Daniel Widdis <[email protected]>

dbwiddis force-pushed the restController branch from 22393b7 to 6199e00 Compare August 23, 2022 01:01

Fix tests to initialize RestController

7f525dd

Signed-off-by: Daniel Widdis <[email protected]>

Use the mockTransportService when verifying it

ca15c35

Signed-off-by: Daniel Widdis <[email protected]>

Remove unused nodeId (replaced by uniqueId) in register request

254005b

Signed-off-by: Daniel Widdis <[email protected]>

Add tests

b556070

Signed-off-by: Daniel Widdis <[email protected]>

dbwiddis added 3 commits August 24, 2022 18:16

Reorder setters to minimize diff

ed37195

Signed-off-by: Daniel Widdis <[email protected]>

Remove unneeded fields and setters, consolidate initialization

6c56504

Signed-off-by: Daniel Widdis <[email protected]>

Remove outdated TODOs

7ade781

Signed-off-by: Daniel Widdis <[email protected]>

dbwiddis force-pushed the restController branch from 2564534 to 7ade781 Compare August 25, 2022 01:16

dbwiddis marked this pull request as ready for review August 25, 2022 01:26

dbwiddis requested review from a team and reta as code owners August 25, 2022 01:26

Fix tests

ca5a29d

Signed-off-by: Daniel Widdis <[email protected]>

owaiskazi19 reviewed Aug 26, 2022

View reviewed changes

server/src/main/java/org/opensearch/extensions/rest/RestSendToExtensionAction.java Outdated Show resolved Hide resolved

owaiskazi19 reviewed Aug 26, 2022

View reviewed changes

server/src/main/java/org/opensearch/extensions/rest/RestSendToExtensionAction.java Outdated Show resolved Hide resolved

Refactor constant string and fix log message

7d2beca

Signed-off-by: Daniel Widdis <[email protected]>

dbwiddis requested review from saratvemulapalli and owaiskazi19 August 26, 2022 17:05

dbwiddis added 2 commits August 26, 2022 20:59

Merge branch 'feature/extensions' of

f414c93

github.com:opensearch-project/OpenSearch into restController Signed-off-by: Daniel Widdis <[email protected]>

Fix tests after merge

885aeda

Signed-off-by: Daniel Widdis <[email protected]>

Process Extension response as REST response

dfad6d4

Signed-off-by: Daniel Widdis <[email protected]>

saratvemulapalli mentioned this pull request Aug 29, 2022

[Feature/extensions] Create a new parent action for all extensions. #4334

Open

saratvemulapalli reviewed Aug 29, 2022

View reviewed changes

saratvemulapalli approved these changes Aug 29, 2022

View reviewed changes

owaiskazi19 approved these changes Aug 30, 2022

View reviewed changes

saratvemulapalli merged commit 50f2d9e into opensearch-project:feature/extensions Aug 30, 2022

dbwiddis deleted the restController branch October 6, 2022 17:12

ryanbogan mentioned this pull request Nov 17, 2022

[Feature/extensions] Merge to main tracking - Extension Points, initial REST implementation and registering Transport Actions #5271

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature/extensions] Register REST API and forward REST requests to associated extension #4282

[Feature/extensions] Register REST API and forward REST requests to associated extension #4282

dbwiddis commented Aug 23, 2022 •

edited

Loading

github-actions bot commented Aug 23, 2022

github-actions bot commented Aug 23, 2022

github-actions bot commented Aug 23, 2022

github-actions bot commented Aug 23, 2022

github-actions bot commented Aug 23, 2022

github-actions bot commented Aug 23, 2022

github-actions bot commented Aug 24, 2022

github-actions bot commented Aug 25, 2022

dbwiddis commented Aug 25, 2022

github-actions bot commented Aug 25, 2022

github-actions bot commented Aug 25, 2022

github-actions bot commented Aug 25, 2022

owaiskazi19 Aug 26, 2022

dbwiddis Aug 26, 2022

owaiskazi19 commented Aug 26, 2022 •

edited

Loading

dbwiddis commented Aug 26, 2022 •

edited

Loading

github-actions bot commented Aug 26, 2022

owaiskazi19 commented Aug 26, 2022 •

edited

Loading

dbwiddis commented Aug 26, 2022 •

edited

Loading

github-actions bot commented Aug 26, 2022

github-actions bot commented Aug 27, 2022

codecov-commenter commented Aug 27, 2022 •

edited

Loading

github-actions bot commented Aug 28, 2022

dbwiddis commented Aug 29, 2022

saratvemulapalli Aug 29, 2022

dbwiddis Aug 30, 2022

dbwiddis Aug 30, 2022

saratvemulapalli commented Aug 29, 2022

owaiskazi19 left a comment •

edited

Loading

[Feature/extensions] Register REST API and forward REST requests to associated extension #4282

[Feature/extensions] Register REST API and forward REST requests to associated extension #4282

Conversation

dbwiddis commented Aug 23, 2022 • edited Loading

Description

Issues Resolved

Check List

github-actions bot commented Aug 23, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Aug 23, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Aug 23, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Aug 23, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Aug 23, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Aug 23, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Aug 24, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Aug 25, 2022

Gradle Check (Jenkins) Run Completed with:

dbwiddis commented Aug 25, 2022

github-actions bot commented Aug 25, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Aug 25, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Aug 25, 2022

Gradle Check (Jenkins) Run Completed with:

owaiskazi19 Aug 26, 2022

Choose a reason for hiding this comment

dbwiddis Aug 26, 2022

Choose a reason for hiding this comment

owaiskazi19 commented Aug 26, 2022 • edited Loading

dbwiddis commented Aug 26, 2022 • edited Loading

github-actions bot commented Aug 26, 2022

Gradle Check (Jenkins) Run Completed with:

owaiskazi19 commented Aug 26, 2022 • edited Loading

dbwiddis commented Aug 26, 2022 • edited Loading

github-actions bot commented Aug 26, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Aug 27, 2022

Gradle Check (Jenkins) Run Completed with:

codecov-commenter commented Aug 27, 2022 • edited Loading

Codecov Report

github-actions bot commented Aug 28, 2022

Gradle Check (Jenkins) Run Completed with:

dbwiddis commented Aug 29, 2022

saratvemulapalli Aug 29, 2022

Choose a reason for hiding this comment

dbwiddis Aug 30, 2022

Choose a reason for hiding this comment

dbwiddis Aug 30, 2022

Choose a reason for hiding this comment

saratvemulapalli commented Aug 29, 2022

owaiskazi19 left a comment • edited Loading

Choose a reason for hiding this comment

dbwiddis commented Aug 23, 2022 •

edited

Loading

owaiskazi19 commented Aug 26, 2022 •

edited

Loading

dbwiddis commented Aug 26, 2022 •

edited

Loading

owaiskazi19 commented Aug 26, 2022 •

edited

Loading

dbwiddis commented Aug 26, 2022 •

edited

Loading

codecov-commenter commented Aug 27, 2022 •

edited

Loading

owaiskazi19 left a comment •

edited

Loading