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

[Feature/extensions] Extensibility support for getNamedWriteables extension point #3925

Merged
merged 25 commits into from
Aug 8, 2022
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
9e31e52
extensibility support for getNamedWriteables extension point. Adding …
joshpalis Jul 15, 2022
bbfc81d
removing string response from parse request, this was added only for …
joshpalis Jul 15, 2022
3d3c113
fixing syntax errors
joshpalis Jul 15, 2022
1c6906f
modified logs for NamedWriteableRegistryRequest
joshpalis Jul 15, 2022
62fe9f3
addressing PR comments
joshpalis Jul 19, 2022
d64f378
addressing PR comments
joshpalis Jul 20, 2022
07df663
Merge branch 'feature/extensions' of https://github.com/opensearch-pr…
joshpalis Jul 21, 2022
1c99994
fixing gradle precommit issues, fixed logger ussage errors
joshpalis Jul 21, 2022
34c2a5e
Fixing all code comment format
joshpalis Jul 21, 2022
9800380
added unit tests, added more javadocs, fixed handleException message …
joshpalis Jul 22, 2022
72da0f5
Addressing PR comments, adding logs for eacch request sent to SDK, re…
joshpalis Jul 23, 2022
d373035
addressing PR comments, added additional unit tests for NamedWriteabl…
joshpalis Jul 25, 2022
d502edb
updating response handler test to check if ExtensionReader callback i…
joshpalis Jul 25, 2022
65fd479
Updated ExtensionReader to use a StreamInput object rather than a byt…
joshpalis Jul 26, 2022
479e094
fixing gradle precommit issues, spotless java check
joshpalis Jul 26, 2022
b193229
modified ExtensdionReader functional interface, abstracted context in…
joshpalis Jul 27, 2022
8580a84
Modified Parse Request to take in a class instead of the fully qualif…
joshpalis Jul 27, 2022
b9f4a80
Updated ExtensionsOrchestratorTests, moved extensionYmlLines and exte…
joshpalis Jul 29, 2022
467182e
linking additional issue to TODO
joshpalis Aug 1, 2022
bdc558c
using extensionsInitializedList instead of extensionsList to ensure t…
joshpalis Aug 1, 2022
d3d52a4
Merge branch 'feature/extensions' into namedwriteables
joshpalis Aug 2, 2022
861daa4
updated tests to pass, after changing extension orchestrator to use t…
joshpalis Aug 2, 2022
4bc44c2
Merge branch 'feature/extensions' into namedwriteables
joshpalis Aug 3, 2022
39ab25c
addressing PR comments, moving named writeable registry support from …
joshpalis Aug 3, 2022
cd222aa
cleaning up comments/ logs, modified extensionsInitialized to be asyn…
joshpalis Aug 8, 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
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
/*
* SPDX-License-Identifier: Apache-2.0
*
* The OpenSearch Contributors require contributions made to
* this file be licensed under the Apache-2.0 license or a
* compatible open source license.
*/

package org.opensearch.common.io.stream;

import org.opensearch.transport.TransportRequest;
import java.io.IOException;
import java.util.Arrays;
import java.util.Objects;

/**
* Extensibility support for Named Writeable Registry: request to extensions to parse context
*
* @opensearch.internal
Copy link
Member

Choose a reason for hiding this comment

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

For my own education, when is this annotation used?

*/
public class NamedWriteableRegistryParseRequest extends TransportRequest {

private final Class categoryClass;
Copy link
Member

Choose a reason for hiding this comment

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

Over on your PR on the SDK side this class is restricted to one that extends NamedWriteable so you should add that restriction here (in the constructor, throwing IllegalArgumentException if it's not a compatible class)

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've removed the generic casting on the SDK end, as per your comment on the corresponding SDK PR. Since we already ensure that all category class entries sent over by the extension utilize the NamedWriteable framework, these restrictions are not necessary.

private byte[] context;

/**
* @param categoryClass Class category for this parse request
* @param context StreamInput object to convert into a byte array and transport to the extension
* @throws IllegalArgumentException if context bytes could not be read
*/
public NamedWriteableRegistryParseRequest(Class categoryClass, StreamInput context) {
try {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need a try/catch block here because we are just assigning values in the constructor?

Copy link
Member Author

Choose a reason for hiding this comment

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

This try/catch block handles the case in which the StreamInput context bytes cannot be read. We convert the StreamInput object into a byte array to transport to the SDK, if the conversion fails then we cannot create the ParseRequest

byte[] streamInputBytes = context.readAllBytes();
this.categoryClass = categoryClass;
this.context = Arrays.copyOf(streamInputBytes, streamInputBytes.length);
} catch (IOException e) {
throw new IllegalArgumentException("Invalid context", e);
}
}

/**
* @param in StreamInput from which class fields are read from
* @throws IllegalArgumentException if the fully qualified class name is invalid and the class object cannot be generated at runtime
*/
public NamedWriteableRegistryParseRequest(StreamInput in) throws IOException {
super(in);
try {
Copy link
Member

Choose a reason for hiding this comment

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

Same as above

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 because we send the fully qualified class names between OpenSearch and the SDK. Within this constructor, we try to convert the fully qualified class name into a class instance to register. If one node does not have the class definition, we cannot register the entry with this category class, so we catch the ClassNotFound exception and throw an IllegalArguementException along with the original exception message

this.categoryClass = Class.forName(in.readString());
Copy link
Member

Choose a reason for hiding this comment

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

Similar comment to above, add some type safety here if you are going to cast to a superclass on the SDK side.

this.context = in.readByteArray();
} catch (ClassNotFoundException e) {
throw new IllegalArgumentException("Category class definition not found", e);
}
}

@Override
public void writeTo(StreamOutput out) throws IOException {
super.writeTo(out);
out.writeString(categoryClass.getName());
out.writeByteArray(context);
}

@Override
public String toString() {
return "NamedWriteableRegistryParseRequest{"
Copy link
Member

Choose a reason for hiding this comment

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

Question for my own education: do we have a standard format for toStrings? Should we?

Copy link
Member Author

Choose a reason for hiding this comment

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

The pattern we have been following is to print out the request/response type along with the contents of the request/response

Copy link
Member

Choose a reason for hiding this comment

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

It's less the type(class name) and field listing it's more the delimiters. I've seen curly braces, square braces, newlines. Most IDEs will auto-generate a toString from fields. I'm wondering which particular format we are using, if any.

+ "categoryClass="
+ categoryClass.getName()
+ ", context length="
+ context.length
Copy link
Member

Choose a reason for hiding this comment

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

Is context printable?

Copy link
Member Author

Choose a reason for hiding this comment

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

the context is a byte array, not sure if printing out the entire array would provide any useful information, so I decided to just print the length of the array. I am open to suggestions on how to represent the context.

+ " }";
}

@Override
public boolean equals(Object o) {
if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false;
NamedWriteableRegistryParseRequest that = (NamedWriteableRegistryParseRequest) o;
return Objects.equals(categoryClass, that.categoryClass) && Objects.equals(context, that.context);
}

@Override
public int hashCode() {
return Objects.hash(categoryClass, context);
}

/**
* Returns the class instance of the category class sent over by the SDK
*/
public Class getCategoryClass() {
return this.categoryClass;
}

/**
* Returns a copy of a byte array that a {@link Writeable.Reader} will be applied to. This byte array is generated from a {@link StreamInput} instance and transported to the SDK for deserialization.
*/
public byte[] getContext() {
return Arrays.copyOf(this.context, this.context.length);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
/*
* SPDX-License-Identifier: Apache-2.0
*
* The OpenSearch Contributors require contributions made to
* this file be licensed under the Apache-2.0 license or a
* compatible open source license.
*/

package org.opensearch.common.io.stream;

import org.opensearch.transport.TransportResponse;
import java.io.IOException;
import java.util.Collections;
import java.util.HashMap;
import java.util.Map;
import java.util.Objects;

/**
* Extensibility support for Named Writeable Registry: response from extensions for name writeable registry entries
*
* @opensearch.internal
*/
public class NamedWriteableRegistryResponse extends TransportResponse {

private final Map<String, Class> registry;

/**
* @param registry Map of writeable names and their associated category class
*/
public NamedWriteableRegistryResponse(Map<String, Class> registry) {
this.registry = new HashMap<>(registry);
}

/**
* @param in StreamInput from which map entries of writeable names and their associated category classes are read from
* @throws IllegalArgumentException if the fully qualified class name is invalid and the class object cannot be generated at runtime
*/
public NamedWriteableRegistryResponse(StreamInput in) throws IOException {
super(in);
// Stream output for registry map begins with a variable integer that tells us the number of entries being sent across the wire
Map<String, Class> registry = new HashMap<>();
int registryEntryCount = in.readVInt();
for (int i = 0; i < registryEntryCount; i++) {
try {
String name = in.readString();
Class categoryClass = Class.forName(in.readString());
registry.put(name, categoryClass);
} catch (ClassNotFoundException e) {
throw new IllegalArgumentException("Category class definition not found", e);
}
}

this.registry = registry;
}

@Override
public void writeTo(StreamOutput out) throws IOException {
// Stream out registry size prior to streaming out registry entries
out.writeVInt(this.registry.size());
for (Map.Entry<String, Class> entry : registry.entrySet()) {
out.writeString(entry.getKey()); // Unique named writeable name
out.writeString(entry.getValue().getName()); // Fully qualified category class name
}
}

@Override
public String toString() {
return "NamedWritableRegistryResponse{" + "registry=" + registry.toString() + "}";
}

@Override
public boolean equals(Object o) {
if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false;
NamedWriteableRegistryResponse that = (NamedWriteableRegistryResponse) o;
return Objects.equals(registry, that.registry);
}

@Override
public int hashCode() {
return Objects.hash(registry);
}

/**
* Returns a map of writeable names and their associated category class
*/
public Map<String, Class> getRegistry() {
return Collections.unmodifiableMap(this.registry);
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
/*
* SPDX-License-Identifier: Apache-2.0
*
* The OpenSearch Contributors require contributions made to
* this file be licensed under the Apache-2.0 license or a
* compatible open source license.
*/

package org.opensearch.extensions;

import org.opensearch.common.io.stream.StreamInput;
import org.opensearch.common.io.stream.StreamOutput;
import org.opensearch.transport.TransportResponse;
import java.io.IOException;
import java.util.Objects;

/**
* Generic boolean response indicating the status of some previous request sent to the SDK
*
* @opensearch.internal
*/
public class ExtensionBooleanResponse extends TransportResponse {

private final boolean status;

/**
* @param status Boolean indicating the status of the parse request sent to the SDK
*/
public ExtensionBooleanResponse(boolean status) {
this.status = status;
}

public ExtensionBooleanResponse(StreamInput in) throws IOException {
super(in);
this.status = in.readBoolean();
}

@Override
public void writeTo(StreamOutput out) throws IOException {
out.writeBoolean(status);
}

@Override
public String toString() {
return "ExtensionBooleanResponse{" + "status=" + this.status + "}";
}

@Override
public boolean equals(Object o) {
if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false;
ExtensionBooleanResponse that = (ExtensionBooleanResponse) o;
return Objects.equals(this.status, that.status);
}

@Override
public int hashCode() {
return Objects.hash(status);
}

/**
* Returns a boolean indicating the success of the request sent to the SDK
*/
public boolean getStatus() {
return this.status;
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
/*
* SPDX-License-Identifier: Apache-2.0
*
* The OpenSearch Contributors require contributions made to
* this file be licensed under the Apache-2.0 license or a
* compatible open source license.
*/

package org.opensearch.extensions;

import java.net.UnknownHostException;
import org.opensearch.cluster.node.DiscoveryNode;

/**
* Reference to a method that transports a parse request to an extension. By convention, this method takes
* a category class used to identify the reader defined within the JVM that the extension is running on.
* Additionally, this method takes in the extension's corresponding DiscoveryNode and a byte array (context) that the
* extension's reader will be applied to.
*
* By convention the extensions' reader is a constructor that takes StreamInput as an argument for most classes and a static method for things like enums.
* Classes will implement this via a constructor (or a static method in the case of enumerations), it's something that should
* look like:
* <pre><code>
* public MyClass(final StreamInput in) throws IOException {
Copy link
Member

Choose a reason for hiding this comment

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

Should this example use extends TransportRequest and the calls to super?

Copy link
Member Author

Choose a reason for hiding this comment

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

Great catch, yes this functional interface should extend TransportRequest since all methods implementing this would send requests to extensions. I'll definitely add this in.

* this.someValue = in.readVInt();
* this.someMap = in.readMapOfLists(StreamInput::readString, StreamInput::readString);
* }
* </code></pre>
*
* @opensearch.internal
*/
@FunctionalInterface
public interface ExtensionReader {

/**
* Transports category class, and StreamInput (context), to the extension identified by the Discovery Node
*
* @param extensionNode Discovery Node identifying the Extension
Copy link
Member

Choose a reason for hiding this comment

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

(Style) leave an extra space between parameter name and its description.

* @param categoryClass Super class that the reader extends
Copy link
Member

Choose a reason for hiding this comment

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

Should probably have an interface for the superclasses the readers extend, and use that for the type parameter. (And the type parameter needs its own javadoc.)

* @param context Some context to transport
* @throws UnknownHostException if the extension node host IP address could not be determined
*/
void parse(DiscoveryNode extensionNode, Class categoryClass, Object context) throws UnknownHostException;

}
Loading