-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Changes from 19 commits
9e31e52
bbfc81d
3d3c113
1c6906f
62fe9f3
d64f378
07df663
1c99994
34c2a5e
9800380
72da0f5
d373035
d502edb
65fd479
479e094
b193229
8580a84
b9f4a80
467182e
bdc558c
d3d52a4
861daa4
4bc44c2
39ab25c
cd222aa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 | ||
*/ | ||
public class NamedWriteableRegistryParseRequest extends TransportRequest { | ||
|
||
private final Class categoryClass; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above There was a problem hiding this comment. Choose a reason for hiding this commentThe 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()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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{" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this example use There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should probably have an |
||
* @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; | ||
|
||
} |
There was a problem hiding this comment.
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?