-
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
[Extensions] Moving ExtensionsRequest to use Protobuf #6960
Changes from all commits
6ed4085
6c0f1a2
65dfbb9
4b4c9e2
f1940e0
49a937a
cdc5718
e926b8d
f6a2170
f65a19d
c91bb9c
b6af3af
7873d2c
08b76ba
96a157a
2c40329
84019a7
b71ec29
33e500a
1089388
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,10 +30,13 @@ | |
|
||
import org.opensearch.gradle.info.BuildParams | ||
|
||
apply plugin: 'opensearch.build' | ||
apply plugin: 'com.netflix.nebula.optional-base' | ||
apply plugin: 'opensearch.publish' | ||
apply plugin: 'opensearch.internal-cluster-test' | ||
plugins { | ||
id('com.google.protobuf') version 'latest.release' | ||
id('opensearch.build') | ||
id('com.netflix.nebula.optional-base') | ||
id('opensearch.publish') | ||
id('opensearch.internal-cluster-test') | ||
} | ||
|
||
publishing { | ||
publications { | ||
|
@@ -45,6 +48,13 @@ publishing { | |
|
||
archivesBaseName = 'opensearch' | ||
|
||
sourceSets { | ||
main { | ||
java { | ||
srcDir "${buildDir}/generated/source/proto/main/java" | ||
} | ||
} | ||
} | ||
// we want to keep the JDKs in our IDEs set to JDK 8 until minimum JDK is bumped to 11 so we do not include this source set in our IDEs | ||
if (!isEclipse) { | ||
sourceSets { | ||
|
@@ -136,6 +146,9 @@ dependencies { | |
// jna | ||
api "net.java.dev.jna:jna:${versions.jna}" | ||
|
||
// protobuf | ||
api "com.google.protobuf:protobuf-java:${versions.protobuf}" | ||
|
||
testImplementation(project(":test:framework")) { | ||
// tests use the locally compiled version of server | ||
exclude group: 'org.opensearch', module: 'server' | ||
|
@@ -157,6 +170,7 @@ tasks.named("internalClusterTest").configure { | |
} | ||
|
||
tasks.named("forbiddenPatterns").configure { | ||
dependsOn("generateProto") | ||
exclude '**/*.json' | ||
exclude '**/*.jmx' | ||
exclude '**/*.dic' | ||
|
@@ -203,6 +217,12 @@ def generatePluginsList = tasks.register("generatePluginsList") { | |
} | ||
} | ||
|
||
protobuf { | ||
protoc { | ||
artifact = "com.google.protobuf:protoc:${versions.protobuf}" | ||
} | ||
} | ||
|
||
tasks.named("processResources").configure { | ||
dependsOn generateModulesList, generatePluginsList | ||
} | ||
|
@@ -303,6 +323,15 @@ tasks.named("thirdPartyAudit").configure { | |
'com.google.common.geometry.S2$Metric', | ||
'com.google.common.geometry.S2LatLng' | ||
) | ||
ignoreViolations( | ||
'com.google.protobuf.MessageSchema', | ||
'com.google.protobuf.UnsafeUtil', | ||
'com.google.protobuf.UnsafeUtil$1', | ||
'com.google.protobuf.UnsafeUtil$Android32MemoryAccessor', | ||
'com.google.protobuf.UnsafeUtil$Android64MemoryAccessor', | ||
'com.google.protobuf.UnsafeUtil$JvmMemoryAccessor', | ||
'com.google.protobuf.UnsafeUtil$MemoryAccessor' | ||
) | ||
} | ||
|
||
tasks.named("dependencyLicenses").configure { | ||
|
@@ -315,17 +344,46 @@ tasks.named("dependencyLicenses").configure { | |
} | ||
} | ||
|
||
tasks.named("missingJavadoc").configure { | ||
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. 👍 |
||
/* | ||
* Generated code doesn't follow javadocs formats. | ||
* Unfortunately the missingJavadoc task doesnt take *. | ||
* TODO: Add support to missingJavadoc task to ignore all generated source code | ||
saratvemulapalli marked this conversation as resolved.
Show resolved
Hide resolved
|
||
* https://github.com/opensearch-project/OpenSearch/issues/7264 | ||
*/ | ||
dependsOn("generateProto") | ||
javadocMissingIgnore = [ | ||
"org.opensearch.extensions.proto.ExtensionRequestProto", | ||
"org.opensearch.extensions.proto.ExtensionRequestProto.ExtensionRequestOrBuilder", | ||
"org.opensearch.extensions.proto" | ||
] | ||
} | ||
|
||
tasks.named("filepermissions").configure { | ||
mustRunAfter("generateProto") | ||
} | ||
|
||
tasks.named("licenseHeaders").configure { | ||
dependsOn("generateProto") | ||
// Ignore our vendored version of Google Guice | ||
excludes << 'org/opensearch/common/inject/**/*' | ||
// Ignore temporary copies of impending 8.7 Lucene classes | ||
excludes << 'org/apache/lucene/search/RegExp87*' | ||
excludes << 'org/apache/lucene/search/RegexpQuery87*' | ||
excludes << 'org/opensearch/client/documentation/placeholder.txt' | ||
// Ignore for protobuf generated code | ||
excludes << 'org/opensearch/extensions/proto/*' | ||
} | ||
|
||
tasks.test { | ||
if (BuildParams.runtimeJavaVersion > JavaVersion.VERSION_1_8) { | ||
jvmArgs += ["--add-opens", "java.base/java.nio.file=ALL-UNNAMED"] | ||
} | ||
} | ||
|
||
tasks.named("sourcesJar").configure { | ||
// Ignore duplicates for protobuf generated code (main and generatedSources). | ||
filesMatching("**/proto/*") { | ||
duplicatesStrategy = DuplicatesStrategy.EXCLUDE | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,11 +13,11 @@ | |
import org.opensearch.common.Nullable; | ||
import org.opensearch.common.io.stream.StreamInput; | ||
import org.opensearch.common.io.stream.StreamOutput; | ||
import org.opensearch.extensions.proto.ExtensionRequestProto; | ||
import org.opensearch.transport.TransportRequest; | ||
|
||
import java.io.IOException; | ||
import java.util.Objects; | ||
import java.util.Optional; | ||
|
||
/** | ||
* CLusterService Request for Extensibility | ||
|
@@ -26,54 +26,54 @@ | |
*/ | ||
public class ExtensionRequest extends TransportRequest { | ||
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. Can we have the generated protobuf class generate the transport request required methods while its at it? I'd love to get rid of the error prone equals/hash/tostring also 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. One step at a time :) 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. |
||
private static final Logger logger = LogManager.getLogger(ExtensionRequest.class); | ||
private final ExtensionsManager.RequestType requestType; | ||
private final Optional<String> uniqueId; | ||
private final ExtensionRequestProto.ExtensionRequest request; | ||
owaiskazi19 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
public ExtensionRequest(ExtensionsManager.RequestType requestType) { | ||
public ExtensionRequest(ExtensionRequestProto.RequestType requestType) { | ||
this(requestType, null); | ||
} | ||
|
||
public ExtensionRequest(ExtensionsManager.RequestType requestType, @Nullable String uniqueId) { | ||
this.requestType = requestType; | ||
this.uniqueId = uniqueId == null ? Optional.empty() : Optional.of(uniqueId); | ||
public ExtensionRequest(ExtensionRequestProto.RequestType requestType, @Nullable String uniqueId) { | ||
ExtensionRequestProto.ExtensionRequest.Builder builder = ExtensionRequestProto.ExtensionRequest.newBuilder(); | ||
if (uniqueId != null) { | ||
saratvemulapalli marked this conversation as resolved.
Show resolved
Hide resolved
|
||
builder.setUniqueId(uniqueId); | ||
} | ||
this.request = builder.setRequestType(requestType).build(); | ||
} | ||
|
||
public ExtensionRequest(StreamInput in) throws IOException { | ||
super(in); | ||
this.requestType = in.readEnum(ExtensionsManager.RequestType.class); | ||
String id = in.readOptionalString(); | ||
this.uniqueId = id == null ? Optional.empty() : Optional.of(id); | ||
this.request = ExtensionRequestProto.ExtensionRequest.parseFrom(in.readByteArray()); | ||
} | ||
|
||
@Override | ||
public void writeTo(StreamOutput out) throws IOException { | ||
super.writeTo(out); | ||
out.writeEnum(requestType); | ||
out.writeOptionalString(uniqueId.orElse(null)); | ||
out.writeByteArray(request.toByteArray()); | ||
} | ||
|
||
public ExtensionsManager.RequestType getRequestType() { | ||
return this.requestType; | ||
public ExtensionRequestProto.RequestType getRequestType() { | ||
return this.request.getRequestType(); | ||
} | ||
|
||
public Optional<String> getUniqueId() { | ||
return uniqueId; | ||
public String getUniqueId() { | ||
return request.getUniqueId(); | ||
} | ||
|
||
public String toString() { | ||
return "ExtensionRequest{" + "requestType=" + requestType + "uniqueId=" + uniqueId + '}'; | ||
return "ExtensionRequest{" + request.toString() + '}'; | ||
} | ||
|
||
@Override | ||
public boolean equals(Object o) { | ||
if (this == o) return true; | ||
if (o == null || getClass() != o.getClass()) return false; | ||
ExtensionRequest that = (ExtensionRequest) o; | ||
return Objects.equals(requestType, that.requestType) && Objects.equals(uniqueId, that.uniqueId); | ||
return Objects.equals(request.getRequestType(), that.request.getRequestType()) | ||
&& Objects.equals(request.getUniqueId(), that.request.getUniqueId()); | ||
} | ||
|
||
@Override | ||
public int hashCode() { | ||
return Objects.hash(requestType, uniqueId); | ||
return Objects.hash(request.getRequestType(), request.getUniqueId()); | ||
} | ||
} |
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.
👍