Add test-framework package to gRPC transport module #20383
Conversation
Migrate high level test utils to a test-framwork package. Purpose of this refactor is to provide test utils to dependant plugins which hide the details of the protobuf schema. Breaking schema changes are liable to cause errors in downstream plugins unexpectedly. This change quarantines details of the proto schema to the core gRPC module. Signed-off-by: Finn Carroll <carrofin@amazon.com>
📝 WalkthroughWalkthroughAdds a new gRPC transport test-framework module with build configuration to publish test artifacts, many license/checksum assets, a test base class with gRPC helpers, unit and integration tests, README and changelog updates, and a small test-client proxy-detector tweak. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant IT as IntegrationTest
participant C as NettyGrpcClient
participant CH as ManagedChannel
participant Node as OpenSearch Node (gRPC server)
rect rgba(135,206,250,0.12)
Note over IT,C: Test setup & client creation
IT->>C: createGrpcClient()
C->>CH: open ManagedChannel
end
rect rgba(144,238,144,0.12)
Note over IT,Node: Indexing via gRPC bulk
IT->>C: doBulk(index, docs)
C->>CH: send BulkRequest (gRPC)
CH->>Node: deliver request
Node->>Node: index documents
Node-->>CH: BulkResponse
CH-->>C: BulkResponse
C-->>IT: GrpcTestBulkResponse
end
rect rgba(255,228,181,0.12)
Note over IT,Node: Search via gRPC
IT->>C: doMatchAll(index, size)
C->>CH: send SearchRequest (gRPC)
CH->>Node: deliver request
Node->>Node: execute search
Node-->>CH: SearchResponse
CH-->>C: SearchResponse
C-->>IT: GrpcTestSearchResponse
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
❌ Gradle check result for eb8067c: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Signed-off-by: Finn Carroll <carrofin@amazon.com>
Signed-off-by: Finn Carroll <carrofin@amazon.com>
Signed-off-by: Finn Carroll <carrofin@amazon.com>
Signed-off-by: Finn Carroll <carrofin@amazon.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In
@modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/ssl/NettyGrpcClient.java:
- Line 26: Replace the internal API usage: remove the import of
io.grpc.internal.GrpcUtil and change any use of GrpcUtil.NOOP_PROXY_DETECTOR (in
NettyGrpcClient, e.g. the argument passed to proxyDetector(...)) to the public
API by passing null to proxyDetector(...) to use gRPC's default detector, or
supply an implementation of io.grpc.ProxyDetector if custom behavior is
required.
In @modules/transport-grpc/test-framework/build.gradle:
- Line 20: Remove the duplicate dependency declaration of
io.grpc:grpc-protobuf:${versions.grpc} in the Gradle file: locate both
occurrences of the line api "io.grpc:grpc-protobuf:${versions.grpc}" and delete
the redundant one so the dependency is declared only once.
In @modules/transport-grpc/test-framework/licenses/grpc-stub-1.75.0.jar.sha1:
- Line 1: Add class-level Javadoc and constructor Javadoc for the two public
static inner classes in GrpcOpenSearchIntegTestCase: GrpcTestBulkResponse and
GrpcTestSearchResponse; for each class add a brief description of the purpose of
the class, and for each constructor add a Javadoc block that describes the
constructor and documents every parameter using @param tags (matching the
constructor parameter names) and any thrown exceptions with @throws if
applicable so the Javadoc generation requirement for "parameter" level
documentation is satisfied.
In @modules/transport-grpc/test-framework/README.md:
- Around line 15-27: The example in MyPluginTest.testMyPluginFeature calls
NettyChannelBuilder.forAddress("localhost", "9200") with the port as a String
which won't compile; update the call to pass an int (e.g., 9200) instead of the
quoted string so NettyChannelBuilder.forAddress("localhost", 9200) is used,
keeping the rest of the channel setup (proxyDetector, usePlaintext, build)
unchanged.
In
@modules/transport-grpc/test-framework/src/main/java/org/opensearch/transport/grpc/test/GrpcOpenSearchIntegTestCase.java:
- Around line 111-117: Update the Javadoc for the overridden method
nodeSettings(int nodeOrdinal) in GrpcOpenSearchIntegTestCase to include a @param
tag describing the nodeOrdinal parameter (e.g., "node index for which to
configure settings") and, if appropriate, add a brief @return tag describing the
returned Settings; ensure the Javadoc stays above the method declaration and
keeps the existing description about configuring node settings for gRPC
transport.
- Around line 95-109: Add missing Javadocs for the GrpcTestSearchResponse class
and its public members: document the class purpose and behavior, the constructor
GrpcTestSearchResponse(SearchResponse) parameters, and each public method
getTotalCount() and getDocumentSource(int) (describe return values and parameter
semantics, including units/interpretation of the long total count and that
getDocumentSource accepts a hit index and returns the document source string).
Ensure the Javadoc uses standard tags (@param, @return) and appears above the
class, constructor, and both methods.
- Around line 150-166: The Javadocs for createTestDocument and
createTestDocuments are missing @param tags (and should include @return); update
the Javadoc block above createTestDocument(String field, String value) to add
@param field description, @param value description and @return description, and
update the Javadoc above createTestDocuments(String field, String prefix, int
count) to add @param field, @param prefix, @param count descriptions and an
@return description so both methods have complete parameter and return
documentation.
- Around line 48-93: Add Javadoc comments for the public inner class
GrpcTestBulkResponse, its constructor GrpcTestBulkResponse(BulkResponse), and
its public methods getCount() and getErrors() so they satisfy OpenSearch Javadoc
requirements; include brief description of the class purpose, parameter
description for the constructor (BulkResponse protoBulkResponse), and @return
descriptions for getCount() and getErrors(), and ensure comments use standard
Javadoc format (/** ... */) placed immediately above each symbol.
🧹 Nitpick comments (3)
modules/transport-grpc/test-framework/README.md (1)
14-27: Add import statements and clarifydoBulk()method.The example is missing import statements and does not explain what
doBulk()is, where it comes from, or what parameters it accepts. Readers will be unclear whetherdoBulk()is inherited fromOpenSearchIntegTestCase, provided by the test-framework module, or must be implemented by the plugin.Consider adding:
- Import statements for the example (e.g.,
import io.grpc.netty.shaded.io.grpc.netty.NettyChannelBuilder).- A brief explanation or Javadoc link for the
doBulk()helper method.modules/transport-grpc/test-framework/src/internalClusterTest/java/org/opensearch/transport/grpc/test/GrpcTestFrameworkIT.java (1)
47-57: Consider validating bulk response before searching.The test indexes documents and immediately searches, but doesn't verify the bulk operation succeeded. If
doBulkfails silently (e.g., partial failures), the search assertion could produce a confusing error message.♻️ Suggested improvement
public void testDoMatchAll() throws Exception { String indexName = "test-search-index"; try (NettyGrpcClient client = createGrpcClient()) { ManagedChannel channel = client.getChannel(); List<String> docs = Arrays.asList("{\"field\": \"doc 0 body\"}", "{\"field\": \"doc 1 body\"}"); - doBulk(channel, indexName, docs); + GrpcTestBulkResponse bulkResponse = doBulk(channel, indexName, docs); + assertEquals("Bulk should have no errors", 0, bulkResponse.getErrors()); GrpcTestSearchResponse response = doMatchAll(channel, indexName, 10); assertTrue("Should return results", response.getTotalCount() >= 2); assertNotNull("Document source should not be null", response.getDocumentSource(0)); } }modules/transport-grpc/test-framework/src/main/java/org/opensearch/transport/grpc/test/GrpcOpenSearchIntegTestCase.java (1)
141-148: Potential failure if no gRPC transports are available.
randomFrom(addresses)will throwIllegalArgumentExceptionif the list is empty. Consider adding a precondition check for a clearer error message.♻️ Add defensive check
protected TransportAddress randomNetty4GrpcServerTransportAddr() { List<TransportAddress> addresses = new ArrayList<>(); for (Netty4GrpcServerTransport transport : internalCluster().getInstances(Netty4GrpcServerTransport.class)) { TransportAddress tAddr = new TransportAddress(transport.getBoundAddress().publishAddress().address()); addresses.add(tAddr); } + assertFalse("No gRPC transport addresses available in cluster", addresses.isEmpty()); return randomFrom(addresses); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
CHANGELOG.mdmodules/transport-grpc/README.mdmodules/transport-grpc/build.gradlemodules/transport-grpc/src/test/java/org/opensearch/transport/grpc/ssl/NettyGrpcClient.javamodules/transport-grpc/test-framework/README.mdmodules/transport-grpc/test-framework/build.gradlemodules/transport-grpc/test-framework/licenses/failureaccess-1.0.2.jar.sha1modules/transport-grpc/test-framework/licenses/failureaccess-LICENSE.txtmodules/transport-grpc/test-framework/licenses/failureaccess-NOTICE.txtmodules/transport-grpc/test-framework/licenses/grpc-LICENSE.txtmodules/transport-grpc/test-framework/licenses/grpc-NOTICE.txtmodules/transport-grpc/test-framework/licenses/grpc-api-1.75.0.jar.sha1modules/transport-grpc/test-framework/licenses/grpc-core-1.75.0.jar.sha1modules/transport-grpc/test-framework/licenses/grpc-netty-shaded-1.75.0.jar.sha1modules/transport-grpc/test-framework/licenses/grpc-protobuf-1.75.0.jar.sha1modules/transport-grpc/test-framework/licenses/grpc-protobuf-lite-1.75.0.jar.sha1modules/transport-grpc/test-framework/licenses/grpc-services-1.75.0.jar.sha1modules/transport-grpc/test-framework/licenses/grpc-stub-1.75.0.jar.sha1modules/transport-grpc/test-framework/licenses/grpc-util-1.75.0.jar.sha1modules/transport-grpc/test-framework/licenses/guava-33.2.1-jre.jar.sha1modules/transport-grpc/test-framework/licenses/guava-LICENSE.txtmodules/transport-grpc/test-framework/licenses/guava-NOTICE.txtmodules/transport-grpc/test-framework/licenses/junit-4.13.2.jar.sha1modules/transport-grpc/test-framework/licenses/junit-LICENSE.txtmodules/transport-grpc/test-framework/licenses/junit-NOTICE.txtmodules/transport-grpc/test-framework/licenses/perfmark-api-0.27.0.jar.sha1modules/transport-grpc/test-framework/licenses/perfmark-api-LICENSE.txtmodules/transport-grpc/test-framework/licenses/perfmark-api-NOTICE.txtmodules/transport-grpc/test-framework/licenses/protobufs-1.0.0.jar.sha1modules/transport-grpc/test-framework/licenses/protobufs-LICENSE.txtmodules/transport-grpc/test-framework/licenses/protobufs-NOTICE.txtmodules/transport-grpc/test-framework/src/internalClusterTest/java/org/opensearch/transport/grpc/test/GrpcTestFrameworkIT.javamodules/transport-grpc/test-framework/src/main/java/org/opensearch/transport/grpc/test/GrpcOpenSearchIntegTestCase.javamodules/transport-grpc/test-framework/src/test/java/org/opensearch/transport/grpc/test/GrpcTestFrameworkTests.java
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2026-01-02T19:23:29.698Z
Learnt from: karenyrx
Repo: opensearch-project/OpenSearch PR: 20335
File: modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/SearchSourceBuilderProtoUtils.java:155-172
Timestamp: 2026-01-02T19:23:29.698Z
Learning: The gRPC search API in OpenSearch is marked as "experimental" in official documentation, so changes to proto schemas that remove previously unsupported fields (those throwing UnsupportedOperationException) are not considered breaking changes.
Applied to files:
CHANGELOG.mdmodules/transport-grpc/README.mdmodules/transport-grpc/test-framework/README.md
📚 Learning: 2026-01-02T19:23:29.698Z
Learnt from: karenyrx
Repo: opensearch-project/OpenSearch PR: 20335
File: modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/SearchSourceBuilderProtoUtils.java:155-172
Timestamp: 2026-01-02T19:23:29.698Z
Learning: In the transport-grpc module, suggest and aggregations protos were removed from SearchRequestBody in protobufs 1.0.0 because they haven't been vetted for accuracy in the API specification. The URL parameter suggest support (suggest_field, suggest_mode, suggest_size, suggest_text) is a minimized subset and not intended as a replacement for full Suggester functionality.
Applied to files:
modules/transport-grpc/README.mdmodules/transport-grpc/test-framework/README.mdmodules/transport-grpc/test-framework/src/main/java/org/opensearch/transport/grpc/test/GrpcOpenSearchIntegTestCase.java
📚 Learning: 2025-12-12T18:40:08.452Z
Learnt from: reta
Repo: opensearch-project/OpenSearch PR: 20017
File: plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java:256-256
Timestamp: 2025-12-12T18:40:08.452Z
Learning: In the OpenSearch ReactorNetty4 secure HTTP transport tests (plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java), URI limit validation has been moved from the protocol layer to the transport layer, making it protocol-agnostic. The random protocol selection in ReactorHttpClient.https(settings) is intentional to ensure all tests validate correct behavior across HTTP/1.1, HTTP/2, and HTTP/3.
Applied to files:
modules/transport-grpc/README.mdmodules/transport-grpc/src/test/java/org/opensearch/transport/grpc/ssl/NettyGrpcClient.javamodules/transport-grpc/test-framework/build.gradlemodules/transport-grpc/test-framework/src/main/java/org/opensearch/transport/grpc/test/GrpcOpenSearchIntegTestCase.java
📚 Learning: 2025-12-19T21:26:37.090Z
Learnt from: reta
Repo: opensearch-project/OpenSearch PR: 20017
File: modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4QuicServerTransport.java:0-0
Timestamp: 2025-12-19T21:26:37.090Z
Learning: In Netty4QuicServerTransport (modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4QuicServerTransport.java), the secureHttpTransportSettingsProvider parameter is guaranteed to be non-null because the plugin verifies its presence during instantiation, so no explicit null-check is needed in the constructor.
Applied to files:
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/ssl/NettyGrpcClient.java
📚 Learning: 2025-12-12T13:31:51.234Z
Learnt from: andriyredko
Repo: opensearch-project/OpenSearch PR: 20017
File: plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java:642-646
Timestamp: 2025-12-12T13:31:51.234Z
Learning: In the OpenSearch ReactorNetty4 secure HTTP transport tests (plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java), the createBuilderWithPort() helper intentionally uses randomBoolean() for the HTTP/3 enabled setting to ensure all tests validate correct behavior with both HTTP/3 enabled and disabled configurations.
Applied to files:
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/ssl/NettyGrpcClient.javamodules/transport-grpc/test-framework/src/main/java/org/opensearch/transport/grpc/test/GrpcOpenSearchIntegTestCase.java
📚 Learning: 2026-01-02T19:23:16.689Z
Learnt from: karenyrx
Repo: opensearch-project/OpenSearch PR: 20335
File: modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/SearchSourceBuilderProtoUtils.java:155-172
Timestamp: 2026-01-02T19:23:16.689Z
Learning: In OpenSearch's gRPC transport module, treat removals of previously-unsupported fields (those throwing UnsupportedOperationException) from experimental proto schemas as non-breaking changes. When reviewing changes to proto-related Java code in this module, document that such removals do not count as breaking API changes, and ensure tests reflect compatibility expectations accordingly.
Applied to files:
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/ssl/NettyGrpcClient.javamodules/transport-grpc/test-framework/src/test/java/org/opensearch/transport/grpc/test/GrpcTestFrameworkTests.javamodules/transport-grpc/test-framework/src/main/java/org/opensearch/transport/grpc/test/GrpcOpenSearchIntegTestCase.javamodules/transport-grpc/test-framework/src/internalClusterTest/java/org/opensearch/transport/grpc/test/GrpcTestFrameworkIT.java
🧬 Code graph analysis (2)
modules/transport-grpc/test-framework/src/main/java/org/opensearch/transport/grpc/test/GrpcOpenSearchIntegTestCase.java (3)
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/GrpcPlugin.java (1)
GrpcPlugin(78-431)modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/Netty4GrpcServerTransport.java (1)
Netty4GrpcServerTransport(64-489)modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/ssl/NettyGrpcClient.java (1)
NettyGrpcClient(42-176)
modules/transport-grpc/test-framework/src/internalClusterTest/java/org/opensearch/transport/grpc/test/GrpcTestFrameworkIT.java (1)
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/ssl/NettyGrpcClient.java (1)
NettyGrpcClient(42-176)
🪛 GitHub Actions: Gradle Precommit
modules/transport-grpc/test-framework/licenses/grpc-api-1.75.0.jar.sha1
[error] 1-1: Javadoc generation failed for :modules:transport-grpc:test-framework. See javadoc outputs for details.
modules/transport-grpc/test-framework/licenses/grpc-LICENSE.txt
[error] 1-1: Javadoc generation failed for :modules:transport-grpc:test-framework. See javadoc outputs for details.
modules/transport-grpc/test-framework/licenses/guava-LICENSE.txt
[error] 1-1: Javadoc generation failed for :modules:transport-grpc:test-framework. See javadoc outputs for details.
modules/transport-grpc/test-framework/licenses/protobufs-LICENSE.txt
[error] 1-1: Javadoc generation failed for :modules:transport-grpc:test-framework. See javadoc outputs for details.
modules/transport-grpc/test-framework/licenses/protobufs-1.0.0.jar.sha1
[error] 1-1: Javadoc generation failed for :modules:transport-grpc:test-framework. See javadoc outputs for details.
modules/transport-grpc/test-framework/licenses/grpc-protobuf-lite-1.75.0.jar.sha1
[error] 1-1: Javadoc generation failed for :modules:transport-grpc:test-framework. See javadoc outputs for details.
modules/transport-grpc/test-framework/licenses/grpc-stub-1.75.0.jar.sha1
[error] 1-1: Javadoc generation failed for :modules:transport-grpc:test-framework. See javadoc outputs for details.
modules/transport-grpc/test-framework/licenses/perfmark-api-LICENSE.txt
[error] 1-1: Javadoc generation failed for :modules:transport-grpc:test-framework. See javadoc outputs for details.
modules/transport-grpc/test-framework/licenses/perfmark-api-NOTICE.txt
[error] 1-1: Javadoc generation failed for :modules:transport-grpc:test-framework. See javadoc outputs for details.
modules/transport-grpc/test-framework/licenses/failureaccess-LICENSE.txt
[error] 1-1: Javadoc generation failed for :modules:transport-grpc:test-framework. See javadoc outputs for details.
modules/transport-grpc/test-framework/licenses/grpc-netty-shaded-1.75.0.jar.sha1
[error] 1-1: Javadoc generation failed for :modules:transport-grpc:test-framework. See javadoc outputs for details.
modules/transport-grpc/test-framework/licenses/grpc-protobuf-1.75.0.jar.sha1
[error] 1-1: Javadoc generation failed for :modules:transport-grpc:test-framework. See javadoc outputs for details.
modules/transport-grpc/test-framework/README.md
[error] 1-1: Javadoc generation failed for :modules:transport-grpc:test-framework. See javadoc outputs for details.
modules/transport-grpc/test-framework/src/test/java/org/opensearch/transport/grpc/test/GrpcTestFrameworkTests.java
[error] 1-1: Javadoc generation failed for :modules:transport-grpc:test-framework. See javadoc outputs for details.
modules/transport-grpc/test-framework/licenses/protobufs-NOTICE.txt
[error] 1-1: Javadoc generation failed for :modules:transport-grpc:test-framework. See javadoc outputs for details.
modules/transport-grpc/test-framework/licenses/junit-4.13.2.jar.sha1
[error] 1-1: Javadoc generation failed for :modules:transport-grpc:test-framework. See javadoc outputs for details.
modules/transport-grpc/test-framework/licenses/failureaccess-1.0.2.jar.sha1
[error] 1-1: Javadoc generation failed for :modules:transport-grpc:test-framework. See javadoc outputs for details.
modules/transport-grpc/test-framework/licenses/grpc-NOTICE.txt
[error] 1-1: Javadoc generation failed for :modules:transport-grpc:test-framework. See javadoc outputs for details.
modules/transport-grpc/test-framework/licenses/junit-LICENSE.txt
[error] 1-1: Javadoc generation failed for :modules:transport-grpc:test-framework. See javadoc outputs for details.
modules/transport-grpc/test-framework/licenses/perfmark-api-0.27.0.jar.sha1
[error] 1-1: Javadoc generation failed for :modules:transport-grpc:test-framework. See javadoc outputs for details.
modules/transport-grpc/test-framework/licenses/grpc-services-1.75.0.jar.sha1
[error] 1-1: Javadoc generation failed for :modules:transport-grpc:test-framework. See javadoc outputs for details.
modules/transport-grpc/test-framework/licenses/grpc-util-1.75.0.jar.sha1
[error] 1-1: Javadoc generation failed for :modules:transport-grpc:test-framework. See javadoc outputs for details.
modules/transport-grpc/test-framework/build.gradle
[error] 1-1: Javadoc generation failed for :modules:transport-grpc:test-framework. See javadoc outputs for details.
modules/transport-grpc/test-framework/licenses/guava-33.2.1-jre.jar.sha1
[error] 1-1: Javadoc generation failed for :modules:transport-grpc:test-framework. See javadoc outputs for details.
modules/transport-grpc/test-framework/src/main/java/org/opensearch/transport/grpc/test/GrpcOpenSearchIntegTestCase.java
[error] 48-48: GrpcOpenSearchIntegTestCase.(): javadocs are missing
[error] 115-115: GrpcOpenSearchIntegTestCase.nodeSettings (method): missing javadoc @param for parameter 'nodeOrdinal'
[error] 153-153: GrpcOpenSearchIntegTestCase.createTestDocument (method): missing javadoc @param for parameter 'field'
[error] 160-160: GrpcOpenSearchIntegTestCase.createTestDocuments (method): missing javadoc @param for parameter 'field'
[error] 95-95: GrpcTestSearchResponse (class): javadocs are missing
[error] 102-102: GrpcTestSearchResponse.getTotalCount (method): javadocs are missing
[error] 106-106: GrpcTestSearchResponse.getDocumentSource (method): javadocs are missing
[error] 50-50: GrpcTestBulkResponse (class): javadocs are missing
[error] 57-57: GrpcTestBulkResponse.getCount (method): javadocs are missing
[error] 61-61: GrpcTestBulkResponse.getErrors (method): javadocs are missing
[error] 1-1: Javadoc generation failed for :modules:transport-grpc:test-framework. See javadoc outputs for details.
modules/transport-grpc/test-framework/licenses/grpc-core-1.75.0.jar.sha1
[error] 1-1: Javadoc generation failed for :modules:transport-grpc:test-framework. See javadoc outputs for details.
modules/transport-grpc/test-framework/src/internalClusterTest/java/org/opensearch/transport/grpc/test/GrpcTestFrameworkIT.java
[error] 1-1: Javadoc generation failed for :modules:transport-grpc:test-framework. See javadoc outputs for details.
🪛 LanguageTool
modules/transport-grpc/test-framework/licenses/grpc-LICENSE.txt
[style] ~162-~162: ‘any and all’ might be wordy. Consider a shorter alternative.
Context: ...ge, computer failure or malfunction, or any and all other commercial damages or losse...
(EN_WORDINESS_PREMIUM_ANY_AND_ALL)
modules/transport-grpc/test-framework/licenses/guava-LICENSE.txt
[style] ~162-~162: ‘any and all’ might be wordy. Consider a shorter alternative.
Context: ...ge, computer failure or malfunction, or any and all other commercial damages or losse...
(EN_WORDINESS_PREMIUM_ANY_AND_ALL)
modules/transport-grpc/test-framework/licenses/protobufs-LICENSE.txt
[style] ~162-~162: ‘any and all’ might be wordy. Consider a shorter alternative.
Context: ...ge, computer failure or malfunction, or any and all other commercial damages or losse...
(EN_WORDINESS_PREMIUM_ANY_AND_ALL)
[grammar] ~324-~324: Use a hyphen to join words.
Context: ...LAR PURPOSE AND NONINFRINGEMENT OF THIRD PARTY RIGHTS. * IN NO EVENT SHALL THE C...
(QB_NEW_EN_HYPHEN)
[style] ~435-~435: To make your text as clear as possible to all readers, do not use this foreign term. A possible alternative is “among other things”.
Context: ...ik/en/ and is licenced on the terms of (inter alia) LGPL and Creative Commons ShareAlike. ...
(INTER_ALIA)
modules/transport-grpc/test-framework/licenses/perfmark-api-LICENSE.txt
[style] ~161-~161: ‘any and all’ might be wordy. Consider a shorter alternative.
Context: ...ge, computer failure or malfunction, or any and all other commercial damages or losse...
(EN_WORDINESS_PREMIUM_ANY_AND_ALL)
modules/transport-grpc/test-framework/licenses/perfmark-api-NOTICE.txt
[grammar] ~17-~17: Use a hyphen to join words.
Context: ... modified portion of 'Catapult', an open source Trace Event viewer for Chome, Lin...
(QB_NEW_EN_HYPHEN)
[grammar] ~18-~18: Ensure spelling is correct
Context: ..., an open source Trace Event viewer for Chome, Linux, and Android applications, which...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~34-~34: Use a hyphen to join words.
Context: ...ins a modified portion of 'ASM', an open source Java Bytecode library, which can ...
(QB_NEW_EN_HYPHEN)
[grammar] ~38-~38: Use a hyphen to join words.
Context: ...fmark/agent/third_party/asm/LICENSE (BSD style License) * HOMEPAGE: * https...
(QB_NEW_EN_HYPHEN)
modules/transport-grpc/test-framework/licenses/failureaccess-LICENSE.txt
[style] ~162-~162: ‘any and all’ might be wordy. Consider a shorter alternative.
Context: ...ge, computer failure or malfunction, or any and all other commercial damages or losse...
(EN_WORDINESS_PREMIUM_ANY_AND_ALL)
modules/transport-grpc/test-framework/licenses/protobufs-NOTICE.txt
[style] ~162-~162: ‘any and all’ might be wordy. Consider a shorter alternative.
Context: ...ge, computer failure or malfunction, or any and all other commercial damages or losse...
(EN_WORDINESS_PREMIUM_ANY_AND_ALL)
[grammar] ~324-~324: Use a hyphen to join words.
Context: ...LAR PURPOSE AND NONINFRINGEMENT OF THIRD PARTY RIGHTS. * IN NO EVENT SHALL THE C...
(QB_NEW_EN_HYPHEN)
[style] ~435-~435: To make your text as clear as possible to all readers, do not use this foreign term. A possible alternative is “among other things”.
Context: ...ik/en/ and is licenced on the terms of (inter alia) LGPL and Creative Commons ShareAlike. ...
(INTER_ALIA)
modules/transport-grpc/test-framework/licenses/grpc-NOTICE.txt
[grammar] ~17-~17: Use a hyphen to join words.
Context: ... a modified portion of 'OkHttp', an open source HTTP & SPDY client for Android an...
(QB_NEW_EN_HYPHEN)
[grammar] ~28-~28: Use a hyphen to join words.
Context: ...s a modified portion of 'Envoy', an open source cloud-native high-performance edg...
(QB_NEW_EN_HYPHEN)
[grammar] ~42-~42: Use a hyphen to join words.
Context: ... of 'protoc-gen-validate (PGV)', an open source protoc plugin to generate polyglo...
(QB_NEW_EN_HYPHEN)
[grammar] ~55-~55: Use a hyphen to join words.
Context: ...ns a modified portion of 'udpa', an open source universal data plane API, which c...
(QB_NEW_EN_HYPHEN)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: gradle-check
- GitHub Check: assemble (25, ubuntu-24.04-arm)
- GitHub Check: assemble (25, windows-latest)
- GitHub Check: assemble (25, ubuntu-latest)
- GitHub Check: assemble (21, windows-latest)
- GitHub Check: assemble (21, ubuntu-latest)
- GitHub Check: assemble (21, ubuntu-24.04-arm)
- GitHub Check: Analyze (java)
🔇 Additional comments (26)
modules/transport-grpc/test-framework/licenses/failureaccess-1.0.2.jar.sha1 (1)
1-1: [rewritten comment]
[classification tag]modules/transport-grpc/test-framework/licenses/grpc-netty-shaded-1.75.0.jar.sha1 (1)
1-1: License checksum file is correctly formatted.The grpc-netty-shaded-1.75.0.jar.sha1 file contains a valid SHA-1 hash (40 hexadecimal characters) matching the format of other dependency checksums in the licenses directory. All Java source files in the test-framework module (GrpcOpenSearchIntegTestCase, GrpcTestFrameworkTests, and GrpcTestFrameworkIT) have complete and well-formed Javadoc comments on classes and public methods.
Likely an incorrect or invalid review comment.
modules/transport-grpc/test-framework/licenses/grpc-api-1.75.0.jar.sha1 (1)
1-1: The checksum file is correctly formatted and has no issues. However, this review comment is incorrect—it conflates a license checksum file with Java source code Javadoc problems. The SHA1 hash in this file cannot cause Javadoc generation failures. If Javadoc issues exist in the test-framework module, they are in the Java source files themselves (where public methods and inner classes lack Javadoc comments), not in this license checksum file.Likely an incorrect or invalid review comment.
modules/transport-grpc/test-framework/licenses/perfmark-api-0.27.0.jar.sha1 (1)
1-1: ✓ SHA-1 checksum format is correct.The checksum file contains a valid 40-character hexadecimal SHA-1 digest as expected for dependency verification.
modules/transport-grpc/test-framework/licenses/perfmark-api-LICENSE.txt (1)
1-201: ✓ Standard Apache License 2.0 text included correctly.This file contains the complete, unmodified Apache License 2.0 text required for open-source compliance. The license text should not be altered.
modules/transport-grpc/test-framework/licenses/perfmark-api-NOTICE.txt (2)
1-40: ✓ Third-party attribution notice included correctly.This NOTICE file contains the required attribution for upstream PerfMark components and their dependencies (Catapult, Polymer, ASM). The text should be preserved as-is from upstream to maintain proper licensing attribution.
1-1: License file changes do not affect Javadoc generation. The file under review is a metadata/license notice file which cannot cause Javadoc compilation failures. The Java source files in the module already contain proper Javadoc comments at the class and method levels. No verification of Javadoc issues is applicable to this change.Likely an incorrect or invalid review comment.
modules/transport-grpc/test-framework/licenses/grpc-services-1.75.0.jar.sha1 (1)
1-1: SHA-1 checksum file is correctly formatted.The file
grpc-services-1.75.0.jar.sha1contains a valid 40-character hexadecimal SHA-1 checksum and requires no changes. The test-framework module's Java source files already contain appropriate class-level Javadoc documentation, and there is no evidence of Javadoc generation failures in the codebase.Likely an incorrect or invalid review comment.
CHANGELOG.md (1)
10-10: LGTM: Changelog entry properly formatted.The entry correctly documents the new test-framework feature under the "Added" section with proper PR reference and clear description. Formatting and placement are consistent with project standards.
modules/transport-grpc/test-framework/licenses/grpc-core-1.75.0.jar.sha1 (1)
1-1: File is valid. The SHA-1 checksum is properly formatted. All public classes and methods in the test-framework module have appropriate Javadoc comments, and there is no evidence of Javadoc generation failures.Likely an incorrect or invalid review comment.
modules/transport-grpc/test-framework/licenses/protobufs-1.0.0.jar.sha1 (1)
1-1: No action needed. The claim of a Javadoc generation failure cannot be substantiated. The file under review is a checksum artifact and has no relationship to Javadoc generation. The three Java source files in the test-framework module have appropriate class-level documentation, and no build failure related to Javadoc is present in the codebase.Likely an incorrect or invalid review comment.
modules/transport-grpc/test-framework/README.md (1)
1-1: Add missing Javadoc to public inner classes and their methods.The inner classes
GrpcTestBulkResponseandGrpcTestSearchResponseinGrpcOpenSearchIntegTestCase.javalack class-level and method-level Javadoc documentation. Their public methods (getCount(),getErrors(),getTotalCount(),getDocumentSource()) need Javadoc. This blocks Javadoc generation in the precommit pipeline.⛔ Skipped due to learnings
Learnt from: karenyrx Repo: opensearch-project/OpenSearch PR: 20335 File: modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/SearchSourceBuilderProtoUtils.java:155-172 Timestamp: 2026-01-02T19:23:29.698Z Learning: The gRPC search API in OpenSearch is marked as "experimental" in official documentation, so changes to proto schemas that remove previously unsupported fields (those throwing UnsupportedOperationException) are not considered breaking changes.modules/transport-grpc/test-framework/licenses/guava-LICENSE.txt (1)
1-202: License file content is standard Apache 2.0 boilerplate.The Apache License 2.0 text is correct and follows standard attribution requirements. The static analysis flag about "any and all" (line 162) is a minor style note in the boilerplate license text itself—this phrasing is part of the official Apache License 2.0 and should not be modified.
modules/transport-grpc/test-framework/licenses/failureaccess-LICENSE.txt (1)
1-202: License file content is standard Apache 2.0 boilerplate.The Apache License 2.0 text is correct and properly attributed. The static analysis flag about "any and all" (line 162) is a minor note in the boilerplate license text—this phrasing is part of the official Apache License 2.0 and should not be modified.
modules/transport-grpc/test-framework/licenses/junit-LICENSE.txt (1)
1-22: The review comment incorrectly attaches a Javadoc generation build failure to a static license file. License files are inert metadata and cannot cause Javadoc generation failures; Javadoc processes only Java source files (.java), configuration, and dependencies. If a Javadoc generation issue exists in the test-framework module, it is unrelated to changes in this license file and should not be flagged in this review.Likely an incorrect or invalid review comment.
modules/transport-grpc/README.md (1)
74-98: Documentation updates accurately reflect the new test framework structure.The reorganization from "Unit Tests" / "Integration Tests" to "Module Tests" / "Test Framework Tests" / "SPI Tests" properly documents the new testing architecture introduced in this PR, with clear Gradle commands for each test category.
modules/transport-grpc/test-framework/build.gradle (1)
45-103: Third-party audit exclusions are extensive but appear necessary.The configuration ignores a large number of optional dependencies from shaded gRPC and Guava libraries. While extensive, these exclusions align with typical patterns for handling shaded Netty dependencies and optional compression/security libraries.
modules/transport-grpc/test-framework/licenses/grpc-NOTICE.txt (1)
1-62: No action required. The license file addition does not affect Javadoc generation. The test-framework module has proper Javadoc documentation on public classes and methods; test methods do not require Javadoc comments per standard Java conventions. There is no evidence of Javadoc generation failures in this module.modules/transport-grpc/test-framework/licenses/protobufs-NOTICE.txt (1)
1-475: LGTM - Standard Apache license notice file.This notice file aggregates the Apache License 2.0 text along with required third-party attributions (Lucene components, Unicode utils, Brics automaton, Snowball stemmers, ICU, Morfologik, etc.). The format follows standard Apache project conventions for bundled dependencies.
The static analysis style hints about wording in the license text should be ignored as the license text must remain verbatim.
modules/transport-grpc/build.gradle (1)
24-35: LGTM - Standard test artifact configuration.This is the correct Gradle pattern for exposing test utilities to dependent modules. The
testArtifactsconfiguration extendingtestImplementationensures transitive dependencies are available, and thetestJartask with the'test'classifier properly packages test classes for consumption by the test-framework submodule.modules/transport-grpc/test-framework/src/test/java/org/opensearch/transport/grpc/test/GrpcTestFrameworkTests.java (1)
18-31: LGTM - Good coverage of utility methods.The tests correctly validate the
createTestDocumentandcreateTestDocumentsstatic helpers. The assertions are thorough, checking both the document count and individual document content for the batch creation method.modules/transport-grpc/test-framework/src/internalClusterTest/java/org/opensearch/transport/grpc/test/GrpcTestFrameworkIT.java (1)
23-45: LGTM - Well-structured integration tests.The bulk indexing tests properly use try-with-resources for client lifecycle management and make clear assertions on document counts and error counts.
modules/transport-grpc/test-framework/src/main/java/org/opensearch/transport/grpc/test/GrpcOpenSearchIntegTestCase.java (1)
168-231: LGTM - Well-implemented gRPC test helpers.The
doBulkanddoMatchAllhelper methods are well-structured:
- Use builder pattern correctly for protobuf messages
- Set
Refresh.REFRESH_TRUEto ensure documents are immediately searchable- Both overloads of
doBulkfollow consistent patterns- Proper use of blocking stubs for synchronous test operations
modules/transport-grpc/test-framework/licenses/guava-33.2.1-jre.jar.sha1 (1)
1-1: SHA-1 hash is correct and verified against Maven Central.The checksum
818e780da2c66c63bbb6480fef1f3855eeafa3e4matches the official Guava 33.2.1-jre release. Guava 33.2.1 is not affected by known security vulnerabilities (CVE-2023-2976 and CVE-2018-10237 were fixed in earlier versions).Consider upgrading to Guava 33.5.0 (available as of January 2026) when feasible.
modules/transport-grpc/test-framework/licenses/grpc-protobuf-lite-1.75.0.jar.sha1 (2)
1-1: Verify necessity of grpc-protobuf-lite dependency.The test framework includes both
grpc-protobufandgrpc-protobuf-lite. The lite variant is typically used for resource-constrained environments like Android. For a standard JVM test framework,grpc-protobufis usually sufficient.Please confirm whether
grpc-protobuf-liteis required for your use case, or if it can be removed to simplify dependencies.
1-1: Verify the claimed Javadoc pipeline failure and validate SHA-1 hash independently.The SHA-1 hash file is properly formatted and consistent with other gRPC dependencies in the repository (40 hexadecimal characters). However, the claims about a pipeline Javadoc failure cannot be confirmed from the codebase—no evidence of such failure is visible in the transport-grpc module configuration or gradle/missing-javadoc.gradle exclusions. Additionally, the grpc-protobuf-lite dependency is intentionally declared in the test-framework build.gradle and is not erroneous.
Please confirm the pipeline failure status and independently verify the SHA-1 hash against Maven Central outside the sandbox environment to ensure accuracy.
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/ssl/NettyGrpcClient.java
Outdated
Show resolved
Hide resolved
modules/transport-grpc/test-framework/licenses/grpc-stub-1.75.0.jar.sha1
Show resolved
Hide resolved
...-framework/src/main/java/org/opensearch/transport/grpc/test/GrpcOpenSearchIntegTestCase.java
Show resolved
Hide resolved
...-framework/src/main/java/org/opensearch/transport/grpc/test/GrpcOpenSearchIntegTestCase.java
Show resolved
Hide resolved
...-framework/src/main/java/org/opensearch/transport/grpc/test/GrpcOpenSearchIntegTestCase.java
Show resolved
Hide resolved
...-framework/src/main/java/org/opensearch/transport/grpc/test/GrpcOpenSearchIntegTestCase.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
@modules/transport-grpc/test-framework/src/main/java/org/opensearch/transport/grpc/test/GrpcOpenSearchIntegTestCase.java:
- Around line 134-141: The Javadoc for
GrpcOpenSearchIntegTestCase.getDocumentSource is inaccurate: the implementation
calls protoSearchResponse.getHits().getHits(i).getXSource() which will throw
IndexOutOfBoundsException for an invalid index, not return null; update the
method Javadoc to remove the "or null if index is out of bounds" phrasing and
explicitly document that the method will throw IndexOutOfBoundsException when i
is out of range (or alternately, if you prefer the null behavior, add a bounds
check against protoSearchResponse.getHits().getHitsCount()/size and return null
when out of range—choose one consistent approach and update either the Javadoc
or the implementation accordingly).
🧹 Nitpick comments (2)
modules/transport-grpc/test-framework/src/main/java/org/opensearch/transport/grpc/test/GrpcOpenSearchIntegTestCase.java (2)
184-193: Consider documenting input constraints.The method uses simple string formatting without JSON escaping. If
fieldorvaluecontain special JSON characters (quotes, backslashes, etc.), the resulting JSON would be malformed. While this is acceptable for a test utility with simple inputs, consider adding a note to the Javadoc about expected input format.📝 Suggested Javadoc clarification
/** * Creates a test document with the specified field and value. * Useful for dependent plugins to generate consistent test data. + * Note: field and value should contain only simple alphanumeric characters; + * special JSON characters are not escaped. * @param field the field name * @param value the field value * @return a JSON string representing the document */
237-259: Consider documenting ID assignment behavior.This
doBulkoverload does not set explicit document IDs (Line 248), while the other overload at lines 211-235 does set IDs viasetXId(). This means documents indexed with this method will receive auto-generated IDs from OpenSearch. Consider adding a note to the Javadoc to clarify this behavior difference.📝 Suggested Javadoc clarification
/** * Helper method to index a number of provided test documents. + * Documents will receive auto-generated IDs from OpenSearch. * @param channel The gRPC channel to use for the indexing operation. * @param index The index to which documents should be indexed. * @param docs List of test documents to index. * @return A GrpcTestBulkResponse containing the results of the indexing operation. */
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
modules/transport-grpc/test-framework/src/main/java/org/opensearch/transport/grpc/test/GrpcOpenSearchIntegTestCase.javamodules/transport-grpc/test-framework/src/main/java/org/opensearch/transport/grpc/test/package-info.java
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2026-01-02T19:23:16.689Z
Learnt from: karenyrx
Repo: opensearch-project/OpenSearch PR: 20335
File: modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/SearchSourceBuilderProtoUtils.java:155-172
Timestamp: 2026-01-02T19:23:16.689Z
Learning: In OpenSearch's gRPC transport module, treat removals of previously-unsupported fields (those throwing UnsupportedOperationException) from experimental proto schemas as non-breaking changes. When reviewing changes to proto-related Java code in this module, document that such removals do not count as breaking API changes, and ensure tests reflect compatibility expectations accordingly.
Applied to files:
modules/transport-grpc/test-framework/src/main/java/org/opensearch/transport/grpc/test/package-info.javamodules/transport-grpc/test-framework/src/main/java/org/opensearch/transport/grpc/test/GrpcOpenSearchIntegTestCase.java
📚 Learning: 2026-01-02T19:23:29.698Z
Learnt from: karenyrx
Repo: opensearch-project/OpenSearch PR: 20335
File: modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/SearchSourceBuilderProtoUtils.java:155-172
Timestamp: 2026-01-02T19:23:29.698Z
Learning: In the transport-grpc module, suggest and aggregations protos were removed from SearchRequestBody in protobufs 1.0.0 because they haven't been vetted for accuracy in the API specification. The URL parameter suggest support (suggest_field, suggest_mode, suggest_size, suggest_text) is a minimized subset and not intended as a replacement for full Suggester functionality.
Applied to files:
modules/transport-grpc/test-framework/src/main/java/org/opensearch/transport/grpc/test/GrpcOpenSearchIntegTestCase.java
📚 Learning: 2025-12-12T18:40:08.452Z
Learnt from: reta
Repo: opensearch-project/OpenSearch PR: 20017
File: plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java:256-256
Timestamp: 2025-12-12T18:40:08.452Z
Learning: In the OpenSearch ReactorNetty4 secure HTTP transport tests (plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java), URI limit validation has been moved from the protocol layer to the transport layer, making it protocol-agnostic. The random protocol selection in ReactorHttpClient.https(settings) is intentional to ensure all tests validate correct behavior across HTTP/1.1, HTTP/2, and HTTP/3.
Applied to files:
modules/transport-grpc/test-framework/src/main/java/org/opensearch/transport/grpc/test/GrpcOpenSearchIntegTestCase.java
📚 Learning: 2025-12-12T13:31:51.234Z
Learnt from: andriyredko
Repo: opensearch-project/OpenSearch PR: 20017
File: plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java:642-646
Timestamp: 2025-12-12T13:31:51.234Z
Learning: In the OpenSearch ReactorNetty4 secure HTTP transport tests (plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java), the createBuilderWithPort() helper intentionally uses randomBoolean() for the HTTP/3 enabled setting to ensure all tests validate correct behavior with both HTTP/3 enabled and disabled configurations.
Applied to files:
modules/transport-grpc/test-framework/src/main/java/org/opensearch/transport/grpc/test/GrpcOpenSearchIntegTestCase.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (22)
- GitHub Check: gradle-check
- GitHub Check: precommit (25, ubuntu-24.04-arm)
- GitHub Check: precommit (21, macos-15)
- GitHub Check: precommit (21, ubuntu-latest)
- GitHub Check: precommit (25, windows-latest)
- GitHub Check: precommit (25, macos-15-intel)
- GitHub Check: precommit (25, macos-15)
- GitHub Check: precommit (21, windows-2025, true)
- GitHub Check: precommit (25, ubuntu-latest)
- GitHub Check: precommit (21, macos-15-intel)
- GitHub Check: precommit (21, ubuntu-24.04-arm)
- GitHub Check: precommit (21, windows-latest)
- GitHub Check: assemble (25, ubuntu-latest)
- GitHub Check: assemble (25, ubuntu-24.04-arm)
- GitHub Check: assemble (21, windows-latest)
- GitHub Check: assemble (25, macos-15)
- GitHub Check: assemble (21, ubuntu-latest)
- GitHub Check: assemble (25, windows-latest)
- GitHub Check: assemble (21, ubuntu-24.04-arm)
- GitHub Check: detect-breaking-change
- GitHub Check: Analyze (java)
- GitHub Check: Mend Security Check
🔇 Additional comments (6)
modules/transport-grpc/test-framework/src/main/java/org/opensearch/transport/grpc/test/package-info.java (1)
1-15: LGTM!The package-info documentation clearly describes the test framework's purpose and aligns well with the PR objective to abstract protobuf schema details from dependent plugins.
modules/transport-grpc/test-framework/src/main/java/org/opensearch/transport/grpc/test/GrpcOpenSearchIntegTestCase.java (5)
45-56: LGTM!The class documentation and constructor are properly documented and implemented.
58-113: LGTM!The bulk response wrapper correctly abstracts protobuf details and provides a clean API for counting indexed documents and errors. The error-counting logic properly handles all operation types.
144-160: LGTM!The configuration overrides properly enable gRPC transport and register the necessary plugin for test execution.
162-182: LGTM!The client creation and address selection methods are well-documented and appropriate for test infrastructure. The
randomFrom()call assumes at least one gRPC transport is available, which is guaranteed by the test configuration.
261-274: LGTM!The match-all search helper properly abstracts the protobuf construction details and provides a clean testing API.
...-framework/src/main/java/org/opensearch/transport/grpc/test/GrpcOpenSearchIntegTestCase.java
Show resolved
Hide resolved
… libs. Signed-off-by: Finn Carroll <carrofin@amazon.com>
Signed-off-by: Finn Carroll <carrofin@amazon.com>
Signed-off-by: Finn Carroll <carrofin@amazon.com>
Signed-off-by: Finn Carroll <carrofin@amazon.com>
Signed-off-by: Finn Carroll <carrofin@amazon.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @modules/transport-grpc/test-framework/README.md:
- Around line 19-22: The README example references the removed internal symbol
NOOP_PROXY_DETECTOR; update the example to use the public API by removing the
proxyDetector(...) call or passing null instead of NOOP_PROXY_DETECTOR so it
matches how NettyGrpcClient now constructs channels (it uses null for the proxy
detector).
🧹 Nitpick comments (1)
modules/transport-grpc/test-framework/src/main/java/org/opensearch/transport/grpc/test/GrpcOpenSearchIntegTestCase.java (1)
142-146: Use standard @throws tag for exception documentation.The Javadoc phrasing "or throw IndexOutOfBoundsException" is grammatically awkward. Use the standard
@throwstag for cleaner documentation.📝 Proposed improvement
/** * Gets the source of a document at the specified index. * @param i the index of the document - * @return the document source as a string, or throw IndexOutOfBoundsException if index is out of bounds + * @return the document source as a string + * @throws IndexOutOfBoundsException if the index is out of bounds */ public String getDocumentSource(int i) {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/ssl/NettyGrpcClient.javamodules/transport-grpc/test-framework/README.mdmodules/transport-grpc/test-framework/build.gradlemodules/transport-grpc/test-framework/src/main/java/org/opensearch/transport/grpc/test/GrpcOpenSearchIntegTestCase.java
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-12-12T18:40:08.452Z
Learnt from: reta
Repo: opensearch-project/OpenSearch PR: 20017
File: plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java:256-256
Timestamp: 2025-12-12T18:40:08.452Z
Learning: In the OpenSearch ReactorNetty4 secure HTTP transport tests (plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java), URI limit validation has been moved from the protocol layer to the transport layer, making it protocol-agnostic. The random protocol selection in ReactorHttpClient.https(settings) is intentional to ensure all tests validate correct behavior across HTTP/1.1, HTTP/2, and HTTP/3.
Applied to files:
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/ssl/NettyGrpcClient.javamodules/transport-grpc/test-framework/src/main/java/org/opensearch/transport/grpc/test/GrpcOpenSearchIntegTestCase.java
📚 Learning: 2025-12-19T21:26:37.090Z
Learnt from: reta
Repo: opensearch-project/OpenSearch PR: 20017
File: modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4QuicServerTransport.java:0-0
Timestamp: 2025-12-19T21:26:37.090Z
Learning: In Netty4QuicServerTransport (modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4QuicServerTransport.java), the secureHttpTransportSettingsProvider parameter is guaranteed to be non-null because the plugin verifies its presence during instantiation, so no explicit null-check is needed in the constructor.
Applied to files:
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/ssl/NettyGrpcClient.java
📚 Learning: 2025-12-12T13:31:51.234Z
Learnt from: andriyredko
Repo: opensearch-project/OpenSearch PR: 20017
File: plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java:642-646
Timestamp: 2025-12-12T13:31:51.234Z
Learning: In the OpenSearch ReactorNetty4 secure HTTP transport tests (plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java), the createBuilderWithPort() helper intentionally uses randomBoolean() for the HTTP/3 enabled setting to ensure all tests validate correct behavior with both HTTP/3 enabled and disabled configurations.
Applied to files:
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/ssl/NettyGrpcClient.javamodules/transport-grpc/test-framework/README.mdmodules/transport-grpc/test-framework/src/main/java/org/opensearch/transport/grpc/test/GrpcOpenSearchIntegTestCase.java
📚 Learning: 2026-01-02T19:23:16.689Z
Learnt from: karenyrx
Repo: opensearch-project/OpenSearch PR: 20335
File: modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/SearchSourceBuilderProtoUtils.java:155-172
Timestamp: 2026-01-02T19:23:16.689Z
Learning: In OpenSearch's gRPC transport module, treat removals of previously-unsupported fields (those throwing UnsupportedOperationException) from experimental proto schemas as non-breaking changes. When reviewing changes to proto-related Java code in this module, document that such removals do not count as breaking API changes, and ensure tests reflect compatibility expectations accordingly.
Applied to files:
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/ssl/NettyGrpcClient.javamodules/transport-grpc/test-framework/src/main/java/org/opensearch/transport/grpc/test/GrpcOpenSearchIntegTestCase.java
📚 Learning: 2026-01-02T19:23:29.698Z
Learnt from: karenyrx
Repo: opensearch-project/OpenSearch PR: 20335
File: modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/SearchSourceBuilderProtoUtils.java:155-172
Timestamp: 2026-01-02T19:23:29.698Z
Learning: In the transport-grpc module, suggest and aggregations protos were removed from SearchRequestBody in protobufs 1.0.0 because they haven't been vetted for accuracy in the API specification. The URL parameter suggest support (suggest_field, suggest_mode, suggest_size, suggest_text) is a minimized subset and not intended as a replacement for full Suggester functionality.
Applied to files:
modules/transport-grpc/test-framework/README.mdmodules/transport-grpc/test-framework/src/main/java/org/opensearch/transport/grpc/test/GrpcOpenSearchIntegTestCase.java
📚 Learning: 2026-01-02T19:23:29.698Z
Learnt from: karenyrx
Repo: opensearch-project/OpenSearch PR: 20335
File: modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/SearchSourceBuilderProtoUtils.java:155-172
Timestamp: 2026-01-02T19:23:29.698Z
Learning: The gRPC search API in OpenSearch is marked as "experimental" in official documentation, so changes to proto schemas that remove previously unsupported fields (those throwing UnsupportedOperationException) are not considered breaking changes.
Applied to files:
modules/transport-grpc/test-framework/README.md
🧬 Code graph analysis (1)
modules/transport-grpc/test-framework/src/main/java/org/opensearch/transport/grpc/test/GrpcOpenSearchIntegTestCase.java (2)
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/Netty4GrpcServerTransport.java (1)
Netty4GrpcServerTransport(64-489)modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/ssl/NettyGrpcClient.java (1)
NettyGrpcClient(41-174)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: gradle-check
- GitHub Check: detect-breaking-change
- GitHub Check: precommit (21, macos-15)
- GitHub Check: Analyze (java)
- GitHub Check: precommit (21, macos-15-intel)
- GitHub Check: precommit (21, windows-2025, true)
- GitHub Check: precommit (25, ubuntu-24.04-arm)
- GitHub Check: assemble (21, ubuntu-24.04-arm)
- GitHub Check: assemble (25, ubuntu-24.04-arm)
- GitHub Check: precommit (25, macos-15)
- GitHub Check: precommit (25, macos-15-intel)
- GitHub Check: precommit (25, windows-latest)
- GitHub Check: precommit (21, windows-latest)
- GitHub Check: assemble (21, windows-latest)
- GitHub Check: precommit (25, ubuntu-latest)
- GitHub Check: assemble (25, windows-latest)
- GitHub Check: assemble (25, ubuntu-latest)
- GitHub Check: assemble (21, ubuntu-latest)
- GitHub Check: precommit (21, ubuntu-latest)
- GitHub Check: precommit (21, ubuntu-24.04-arm)
🔇 Additional comments (1)
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/ssl/NettyGrpcClient.java (1)
135-135: LGTM! Internal API usage correctly replaced.The change from
GrpcUtil.NOOP_PROXY_DETECTORto passingnulltoproxyDetector()correctly adopts the public gRPC API and addresses the previous review concern about relying on unstable internal APIs.
|
❌ Gradle check result for 7cacdd4: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
❌ Gradle check result for 7cacdd4: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
Hi @karenyrx , @cwperks. |
|
❌ Gradle check result for 7cacdd4: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
1 participant
Description
Migrate high level test utils to a test-framework package. The purpose of this refactor is to provide test utils to dependent plugins which hide the details of the protobuf schema. Breaking schema changes are liable to cause errors in downstream plugins unexpectedly. This change quarantines details of the proto schema to the core gRPC module.
Related Issues
5524 - In particular the security plugin faces this problem, as it contains no additional conversion logic for the proto schema but still needs to validate security functionality with basic index/search functionality.
Check List
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.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.