[Feature/extensions] Adding extensibility support for extension point getNamedWriteables. #54

joshpalis · 2022-07-15T22:10:04Z

Signed-off-by: Joshua Palis jpalis@amazon.com

Description

Adds support for registering named writeable entries from extensions and transporting parse requests to extensions to deserialize named writeables that they have registered.

Issues Resolved

opensearch-project/OpenSearch#3495

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 <jpalis@amazon.com>

dbwiddis

Looks good as a placeholder for now. Needs some javadocs tho!

dbwiddis · 2022-07-16T02:51:55Z

src/main/java/org/opensearch/sdk/ExtensionsRunner.java

@@ -58,6 +71,8 @@
 public class ExtensionsRunner {
    private ExtensionSettings extensionSettings = readExtensionSettings();
    private DiscoveryNode opensearchNode;
+    private List<NamedWriteableRegistry.Entry> namedWriteables = getNamedWriteables();
+    final private NamedWriteableRegistry namedWriteableRegistry = new NamedWriteableRegistry(namedWriteables);

    public ExtensionsRunner() throws IOException {}


(Style) All of the instance fields should be declared above the constructor. I know this is not part of your PR but you can fix it.

dbwiddis · 2022-07-16T02:56:29Z

src/main/java/org/opensearch/sdk/ExtensionsRunner.java

@@ -77,6 +92,14 @@ private ExtensionSettings readExtensionSettings() throws IOException {
        return extensionSettings;
    }

+    // current placeholder for extension point override getNamedWriteables()
+    private List<NamedWriteableRegistry.Entry> getNamedWriteables() {


Can you put the PR number here as a comment so we know where to look for what's intended to replace this? I'm not clear what's being overridden here.

saratvemulapalli · 2022-07-19T17:29:48Z

src/main/java/org/opensearch/sdk/ExtensionsRunner.java

@@ -93,6 +116,66 @@ PluginResponse handlePluginsRequest(PluginRequest pluginRequest) {
        return pluginResponse;
    }

+    NamedWriteableRegistryResponse handleNamedWriteableRegistryRequest(NamedWriteableRegistryRequest request) {


Its nice to see how we are adding more extension points support for the SDK.
This is expanding ExtensionsRunner.

Instead could you write a new class (may be NamedWriteableRegistryApi) which is constructed during the bootstrap of extension, populate namedWritables and write up functions which handle the request, parseRequest etc

Great suggestion, I'll definitely add this change in my next commit

saratvemulapalli · 2022-07-19T17:36:31Z

src/main/java/org/opensearch/sdk/ExtensionsRunner.java

+                    try {
+                        C c = streamInput.readNamedWriteable(categoryClass);
+
+                        // TODO : current parse response to OpenSearch includes only the status of the parse request. Further research


Lets open up follow up issues for everything we haven't taken care of.

saratvemulapalli · 2022-07-19T17:37:34Z

src/main/java/org/opensearch/sdk/ExtensionsRunner.java

+        boolean status = false;
+
+        // extract data from request and procress fully qualified category class name into class instance
+        try {


Do we need a lot of nested try's?

I went ahead and removed the nested try's from the response handlers. As per Dan's suggestions, I will now handle Class object creation within the request/response constructors

…ltExtensionPointRequest. Modifed NamedWriteableRegistryResponse handler to pass in category class instead of fully qualified class name, since transport response/request files will now handle manipulation of the data Signed-off-by: Joshua Palis <jpalis@amazon.com>

owaiskazi19 · 2022-07-19T21:24:13Z

src/main/java/org/opensearch/sdk/ExtensionsRunner.java

+    private List<NamedWriteableRegistry.Entry> getNamedWriteables() {
+
+        List<NamedWriteableRegistry.Entry> namedWriteables = new ArrayList<>();
+        // extensions will add named writeable registry entries here


Let's follow the javadoc style here for the comments

Suggested change

// extensions will add named writeable registry entries here

// Extensions will add named writeable registry entries here

Also, is this part incomplete as we are creating an empty ArrayList and returning? Am I missing something here?

I'll convert this to an api that the Extension can use to pass in their named writeable entries, as per Sarat's comment

owaiskazi19 · 2022-07-19T21:24:57Z

src/main/java/org/opensearch/sdk/ExtensionsRunner.java

+        boolean status = false;
+
+        // extract data from request and procress fully qualified category class name into class instance
+        Class<C> categoryClass = (Class<C>) request.getCategoryClass();


For my education what's type C?

Type C denotes the class of the writeable object. There are many various classes that implement the writeable class, however they are all expected to extend the NamedWriteable class. By casting the category class sent over from OpenSearch with this type, we can ensure that the category class entries extend NamedWriteables

owaiskazi19 · 2022-07-19T21:25:41Z

src/main/java/org/opensearch/sdk/ExtensionsRunner.java

+                // reader from provided registry
+                // reader is then applied to the StreamInput object generated from the byte array (context)
+                try {
+                    C c = streamInput.readNamedWriteable(categoryClass);


Right naming here?

owaiskazi19 · 2022-07-19T21:26:34Z

src/main/java/org/opensearch/sdk/ExtensionsRunner.java

+
+                    // TODO : current parse response to OpenSearch includes only the status of the parse request. Further research
+                    // needed to determine the workflow for extensions to utilize parsed objects after deserialization but this will be
+                    // within the scope of dynamic registration.


Can we link this TODO with an issue?

Signed-off-by: Joshua Palis <jpalis@amazon.com>

owaiskazi19 · 2022-07-21T00:18:11Z

src/main/java/org/opensearch/sdk/NamedWriteableRegistryApi.java

+
+                // apply reader to stream input generated from the request context
+                try {
+                    C c = streamInput.readNamedWriteable(categoryClass);


Right naming here as well?

owaiskazi19 · 2022-07-21T00:18:34Z

src/main/java/org/opensearch/sdk/NamedWriteableRegistryApi.java

+     */
+    public NamedWriteableRegistryResponse handleNamedWriteableRegistryRequest(OpenSearchRequest request) {
+        logger.info("Registering Named Writeable Registry Request recieved from OpenSearch.");
+        // iterate through Extensions's named writeables and add to extension entries


Suggested change

// iterate through Extensions's named writeables and add to extension entries

// Iterate through Extensions's named writeables and add to extension entries

owaiskazi19 · 2022-07-21T00:18:46Z

src/main/java/org/opensearch/sdk/NamedWriteableRegistryApi.java

+        Class<C> categoryClass = (Class<C>) request.getCategoryClass();
+        byte[] context = request.getContext();
+
+        // transform byte array context into an input stream


Suggested change

// transform byte array context into an input stream

// Transform byte array context into an input stream

owaiskazi19 · 2022-07-21T00:18:53Z

src/main/java/org/opensearch/sdk/NamedWriteableRegistryApi.java

+        logger.info("Registering Named Writeable Registry Parse request from OpenSearch");
+        boolean status = false;
+
+        // extract data from request and procress fully qualified category class name into class instance


Suggested change

// extract data from request and procress fully qualified category class name into class instance

// Extract data from request and procress fully qualified category class name into class instance

owaiskazi19 · 2022-07-21T00:19:03Z

src/main/java/org/opensearch/sdk/NamedWriteableRegistryApi.java

+        // transform byte array context into an input stream
+        try (InputStream inputStream = new ByteArrayInputStream(context, 0, context.length)) {
+
+            // convert input stream to stream input


Suggested change

// convert input stream to stream input

// Convert input stream to stream input

owaiskazi19 · 2022-07-21T00:19:12Z

src/main/java/org/opensearch/sdk/NamedWriteableRegistryApi.java

+                )
+            ) {
+
+                // apply reader to stream input generated from the request context


Suggested change

// apply reader to stream input generated from the request context

// Apply reader to stream input generated from the request context

Signed-off-by: Joshua Palis <jpalis@amazon.com>

…o Api to return the NamedWriteableRegistry for testing purposes Signed-off-by: Joshua Palis <jpalis@amazon.com>

owaiskazi19 · 2022-07-21T21:31:32Z

src/main/java/org/opensearch/sdk/ExtensionsRunner.java

+    private NamedWriteableRegistryApi namedWriteableRegistryApi = new NamedWriteableRegistryApi();
+
+    /**
+     * Constructor for ExtensionsRunner


Any reason for the changing the javadocs here?

Javadocs for the extension runner constructor was missing and it was failing the strict javadoc check

But we had javadocs already https://github.com/opensearch-project/opensearch-sdk/pull/54/files#diff-8208658a7c4e8521b2d0ad827f1cb9c9890f66eb568e8dce05215507d3d80911L64 🙂

owaiskazi19 · 2022-07-21T21:32:11Z

src/main/java/org/opensearch/sdk/ExtensionsRunner.java

+                return namedWriteableRegistryApi.handleNamedWriteableRegistryRequest(request);
+            // Add additional request handlers here
+            default:
+                return null;


Instead of null let's return an exception here?

owaiskazi19 · 2022-07-21T21:33:22Z

src/main/java/org/opensearch/sdk/ExtensionsRunner.java

+            (request, channel, task) -> channel.sendResponse(handleOpenSearchRequest(request))
+        );
+
+        transportService.registerRequestHandler(


This is another overriden method you are using here. It's missing forceExecution and canTripCircuitBreaker

Great catch, I'll make that change and update the corresponding test

owaiskazi19 · 2022-07-21T21:33:51Z

src/main/java/org/opensearch/sdk/NamedWriteableRegistryApi.java

+/**
+ * Api used to handle named writeable registry requests from OpenSearch
+ */
+public class NamedWriteableRegistryApi {


NamedWriteableRegistryAPI, better name?

Signed-off-by: Joshua Palis <jpalis@amazon.com>

owaiskazi19 · 2022-07-21T21:36:24Z

src/main/java/org/opensearch/sdk/ExtensionsRunner.java

+     * @param request  The request to handle.
+     * @return A response to OpenSearch for the corresponding API
+     */
+    TransportResponse handleOpenSearchRequest(OpenSearchRequest request) {


Need test for this function is TestExtensionRunner

just added the test

Signed-off-by: Joshua Palis <jpalis@amazon.com>

…ption if the request handler is not present Signed-off-by: Joshua Palis <jpalis@amazon.com>

Signed-off-by: Joshua Palis <jpalis@amazon.com>

…ModuleNameResponse with ExtensionBooleanResponse Signed-off-by: Joshua Palis <jpalis@amazon.com>

…to the Parse request Signed-off-by: Joshua Palis <jpalis@amazon.com>

…ted logger to use this class instead Signed-off-by: Joshua Palis <jpalis@amazon.com>

owaiskazi19 · 2022-07-29T21:06:05Z

src/main/java/org/opensearch/sdk/NamedWriteableRegistryAPI.java

+
+                // Apply reader to stream input generated from the request context
+                try {
+                    C c = streamInput.readNamedWriteable(categoryClass);


Better name here?

Sure Ill make this change

owaiskazi19

One last comment and rest LGTM! Thanks for incorporating all the changes!

Signed-off-by: Joshua Palis <jpalis@amazon.com>

dbwiddis · 2022-07-31T03:45:33Z

src/main/java/org/opensearch/sdk/NamedWriteableRegistryAPI.java

+     * @throws IOException if InputStream generated from the byte array is unsuccessfully closed
+     * @return A response acknowledging the request to parse has executed successfully
+     */
+    public <C extends NamedWriteable> ExtensionBooleanResponse handleNamedWriteableRegistryParseRequest(


Something's not right with this generic. I'm used to seeing them in the method signature (e.g., as the type of request). But in this case you've more narrowly scoped the argument, meaning there's no real reason to use C later on. You already know the NamedWritable interface applies and you could cast it directly elsewhere.

Good callout, since we already verify that the NamedWriteable interface already applies prior to sending the parse request across the wire, I'll remove the the generic scope

dbwiddis · 2022-07-31T03:50:43Z

src/main/java/org/opensearch/sdk/NamedWriteableRegistryAPI.java

+
+                // Apply reader to stream input generated from the request context
+                try {
+                    C namedWriteable = streamInput.readNamedWriteable(categoryClass);


what do we do with this return value?

Currently, the scope of this PR consists of enabling extensions to register named writeables and handle deserialization requests from OpenSearch. Here we assume that any named writeable that an extension creates will be used only by the extension, thus maintaining the separation of concerns between OpenSearch and the extension. The question of how the named writeable object is used within the extension after it is created still requires additional research and is in the wider scope of dynamic registration. I will create another issue and link this here at this point, but as for now, if deserialization is successful, we notify OpenSearch about the status of the parse request with a boolean response.

Signed-off-by: Joshua Palis <jpalis@amazon.com>

saratvemulapalli

Thanks a lot for this @joshpalis.
I did one pass, most of the changes look good to me.

Signed-off-by: Joshua Palis <jpalis@amazon.com>

… getNamedWriteables. (opensearch-project#54) * adding extensibility support for extension point getNamedWriteables. Signed-off-by: Joshua Palis <jpalis@amazon.com> * removed NamedWriteableRegistryRequest and replaced with generic DefaultExtensionPointRequest. Modifed NamedWriteableRegistryResponse handler to pass in category class instead of fully qualified class name, since transport response/request files will now handle manipulation of the data Signed-off-by: Joshua Palis <jpalis@amazon.com> * addressing PR comments Signed-off-by: Joshua Palis <jpalis@amazon.com> * addressing PR comments, fixing code comment format Signed-off-by: Joshua Palis <jpalis@amazon.com> * fixing javadocs issues, updating extensionrunner tests Signed-off-by: Joshua Palis <jpalis@amazon.com> * Added unit tests for NamedWriteableRegistryApi, added getter method to Api to return the NamedWriteableRegistry for testing purposes Signed-off-by: Joshua Palis <jpalis@amazon.com> * Added test for handling opensearch requests Signed-off-by: Joshua Palis <jpalis@amazon.com> * addressing PR comments Signed-off-by: Joshua Palis <jpalis@amazon.com> * addressing PR comments Signed-off-by: Joshua Palis <jpalis@amazon.com> * addressing PR comments Signed-off-by: Joshua Palis <jpalis@amazon.com> * Addressing PR comments : changed invalid parse request test name Signed-off-by: Joshua Palis <jpalis@amazon.com> * modified default action for handleOpenSearch request to throw an exception if the request handler is not present Signed-off-by: Joshua Palis <jpalis@amazon.com> * Addedtest for invalid category class Signed-off-by: Joshua Palis <jpalis@amazon.com> * reverting ExtensionsRunner constructor javadocs Signed-off-by: Joshua Palis <jpalis@amazon.com> * updated BooleanResponse to ExtensionBooleanResponse, replaced IndicesModuleNameResponse with ExtensionBooleanResponse Signed-off-by: Joshua Palis <jpalis@amazon.com> * Updated unit tests to pass in a StreamInput rather than a byte array to the Parse request Signed-off-by: Joshua Palis <jpalis@amazon.com> * NamedWriteableRegistry API was using the ExtensionRunner logger, updated logger to use this class instead Signed-off-by: Joshua Palis <jpalis@amazon.com> * updated name of parsed object Signed-off-by: Joshua Palis <jpalis@amazon.com> * addressing PR comments, linking addtional issues to TODO Signed-off-by: Joshua Palis <jpalis@amazon.com> * Fixing import conflict Signed-off-by: Joshua Palis <jpalis@amazon.com> * removing netty4/shared group factory imports Signed-off-by: Joshua Palis <jpalis@amazon.com>

adding extensibility support for extension point getNamedWriteables.

81a0eee

Signed-off-by: Joshua Palis <jpalis@amazon.com>

owaiskazi19 changed the title ~~adding extensibility support for extension point getNamedWriteables.~~ [Feature/extensions] Adding extensibility support for extension point getNamedWriteables. Jul 15, 2022

dbwiddis reviewed Jul 16, 2022

View reviewed changes

dbwiddis mentioned this pull request Jul 16, 2022

[Feature/extensions] Extensibility support for getNamedWriteables extension point opensearch-project/OpenSearch#3925

Merged

5 tasks

saratvemulapalli reviewed Jul 19, 2022

View reviewed changes

owaiskazi19 reviewed Jul 19, 2022

View reviewed changes

addressing PR comments

70f3a1b

Signed-off-by: Joshua Palis <jpalis@amazon.com>

owaiskazi19 reviewed Jul 21, 2022

View reviewed changes

joshpalis added 4 commits July 21, 2022 18:28

addressing PR comments, fixing code comment format

a5d2142

Signed-off-by: Joshua Palis <jpalis@amazon.com>

rebasing fork with main

58b1509

Signed-off-by: Joshua Palis <jpalis@amazon.com>

fixing javadocs issues, updating extensionrunner tests

cd032ac

Signed-off-by: Joshua Palis <jpalis@amazon.com>

Added unit tests for NamedWriteableRegistryApi, added getter method t…

1294a94

…o Api to return the NamedWriteableRegistry for testing purposes Signed-off-by: Joshua Palis <jpalis@amazon.com>

joshpalis marked this pull request as ready for review July 21, 2022 21:22

joshpalis requested a review from a team July 21, 2022 21:22

owaiskazi19 reviewed Jul 21, 2022

View reviewed changes

Added test for handling opensearch requests

623037c

Signed-off-by: Joshua Palis <jpalis@amazon.com>

owaiskazi19 reviewed Jul 21, 2022

View reviewed changes

addressing PR comments

a001bd5

Signed-off-by: Joshua Palis <jpalis@amazon.com>

joshpalis added 6 commits July 21, 2022 22:13

modified default action for handleOpenSearch request to throw an exce…

0a2ac46

…ption if the request handler is not present Signed-off-by: Joshua Palis <jpalis@amazon.com>

Addedtest for invalid category class

56d336e

Signed-off-by: Joshua Palis <jpalis@amazon.com>

reverting ExtensionsRunner constructor javadocs

cb55af1

Signed-off-by: Joshua Palis <jpalis@amazon.com>

updated BooleanResponse to ExtensionBooleanResponse, replaced Indices…

56fd72a

…ModuleNameResponse with ExtensionBooleanResponse Signed-off-by: Joshua Palis <jpalis@amazon.com>

Updated unit tests to pass in a StreamInput rather than a byte array …

a5f7125

…to the Parse request Signed-off-by: Joshua Palis <jpalis@amazon.com>

NamedWriteableRegistry API was using the ExtensionRunner logger, upda…

5d8045f

…ted logger to use this class instead Signed-off-by: Joshua Palis <jpalis@amazon.com>

owaiskazi19 reviewed Jul 29, 2022

View reviewed changes

owaiskazi19 previously approved these changes Jul 29, 2022

View reviewed changes

owaiskazi19 requested review from dbwiddis and saratvemulapalli July 29, 2022 21:09

updated name of parsed object

7ff534e

Signed-off-by: Joshua Palis <jpalis@amazon.com>

joshpalis dismissed owaiskazi19’s stale review via 7ff534e July 29, 2022 22:11

dbwiddis reviewed Jul 31, 2022

View reviewed changes

addressing PR comments, linking addtional issues to TODO

80cb3cb

Signed-off-by: Joshua Palis <jpalis@amazon.com>

saratvemulapalli previously approved these changes Aug 1, 2022

View reviewed changes

joshpalis requested a review from ryanbogan August 4, 2022 20:45

ryanbogan previously approved these changes Aug 8, 2022

View reviewed changes

rebasing with main

95f0f53

Signed-off-by: Joshua Palis <jpalis@amazon.com>

joshpalis dismissed stale reviews from ryanbogan and saratvemulapalli via 95f0f53 August 8, 2022 20:48

joshpalis added 2 commits August 8, 2022 20:59

Fixing import conflict

7418928

Signed-off-by: Joshua Palis <jpalis@amazon.com>

removing netty4/shared group factory imports

461e0ec

Signed-off-by: Joshua Palis <jpalis@amazon.com>

joshpalis requested review from ryanbogan and saratvemulapalli August 8, 2022 21:02

ryanbogan approved these changes Aug 8, 2022

View reviewed changes

saratvemulapalli approved these changes Aug 8, 2022

View reviewed changes

saratvemulapalli merged commit 06cb467 into opensearch-project:main Aug 8, 2022

joshpalis deleted the namedwriteables branch August 8, 2022 21:16

	// extensions will add named writeable registry entries here
	// Extensions will add named writeable registry entries here

	// iterate through Extensions's named writeables and add to extension entries
	// Iterate through Extensions's named writeables and add to extension entries

	// transform byte array context into an input stream
	// Transform byte array context into an input stream

	// extract data from request and procress fully qualified category class name into class instance
	// Extract data from request and procress fully qualified category class name into class instance

	// convert input stream to stream input
	// Convert input stream to stream input

	// apply reader to stream input generated from the request context
	// Apply reader to stream input generated from the request context

[Feature/extensions] Adding extensibility support for extension point getNamedWriteables. #54

[Feature/extensions] Adding extensibility support for extension point getNamedWriteables. #54

Uh oh!

Conversation

joshpalis commented Jul 15, 2022

Description

Issues Resolved

Uh oh!

dbwiddis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

owaiskazi19 Jul 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

owaiskazi19 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

owaiskazi19 Jul 19, 2022 •

edited

Loading