Skip to content

Conversation

@karenyrx
Copy link
Contributor

@karenyrx karenyrx commented Aug 6, 2025

Description

Publish a lighter weight version of the GRPC module into a SPI, exposing only the interfaces needed by external plugins.

Instead of the entire transport-grpc module as a dependency, external plugins can import a lighter weight transport-grpc-spi as a dependency, which only bundles opensearch-protobufs and :server, rather than the entire grpc lib (including guava).

Example PR on how this SPI can be used by external plugins: opensearch-project/k-NN#2833

Test plan

Built and tested interanlly, and GRPC KNN Queries are still working:
(No data was ingested for this test so no vectors are returned)

  --request '{
    "size": 5,
    "request_body": {
      "query": {
        "knn": {
          "field": "vector_field",
          "vector": [0.1,0.2,0.3,0.4,0.5,0.6,0.7,0.8,0.9,1.0,0.1,0.2,0.3,0.4,0.5,0.6,0.7,0.8,0.9,1.0,0.1,0.2,0.3,0.4,0.5,0.6,0.7,0.8,0.9,1.0,0.1,0.2,0.3,0.4,0.5,0.6,0.7,0.8,0.9,1.0,0.1,0.2,0.3,0.4,0.5,0.6,0.7,0.8,0.9,1.0,0.1,0.2,0.3,0.4,0.5,0.6,0.7,0.8,0.9,1.0,0.1,0.2,0.3,0.4,0.5,0.6,0.7,0.8,0.9,1.0,0.1,0.2,0.3,0.4,0.5,0.6,0.7,0.8,0.9,1.0,0.1,0.2,0.3,0.4,0.5,0.6,0.7,0.8,0.9,1.0,0.1,0.2,0.3,0.4,0.5,0.6,0.7,0.8,0.9,1.0,0.1,0.2,0.3,0.4,0.5,0.6,0.7,0.8,0.9,1.0,0.1,0.2,0.3,0.4,0.5,0.6,0.7,0.8,0.9,1.0,0.1,0.2,0.3,0.4,0.5,0.6,0.7,0.8],
          "k": 1
        }
      }
    }
  }'
{
  "body": {
    "responseBody": {
      "took": "10",
      "timedOut": false,
      "shards": {},
      "hits": {
        "total": {
          "totalHits": {
            "relation": "TOTAL_HITS_RELATION_EQ"
          }
        },
        "maxScore": {
          "floatValue": 0
        }
      }
    }
  }
}

A sample log on node startup, showing both core Query converters (e.g. MATCH_ALL, MATCH_NONE, TERM, TERMS) as well as external Query converter (e.g. KNN) are being registered:


{"level":"INFO","timestamp":"2025-08-15T18:32:12,525","thread":"main","file":"QueryBuilderProtoConverterSpiRegistry.java", "line":"106","message":"Registered query converter for MATCH_ALL: org.opensearch.transport.grpc.proto.request.search.query.MatchAllQueryBuilderProtoConverter"}
{"level":"INFO","timestamp":"2025-08-15T18:32:12,526","thread":"main","file":"QueryBuilderProtoConverterSpiRegistry.java", "line":"106","message":"Registered query converter for MATCH_NONE: org.opensearch.transport.grpc.proto.request.search.query.MatchNoneQueryBuilderProtoConverter"}
{"level":"INFO","timestamp":"2025-08-15T18:32:12,527","thread":"main","file":"QueryBuilderProtoConverterSpiRegistry.java", "line":"106","message":"Registered query converter for TERM: org.opensearch.transport.grpc.proto.request.search.query.TermQueryBuilderProtoConverter"}
{"level":"INFO","timestamp":"2025-08-15T18:32:12,527","thread":"main","file":"QueryBuilderProtoConverterSpiRegistry.java", "line":"106","message":"Registered query converter for TERMS: org.opensearch.transport.grpc.proto.request.search.query.TermsQueryBuilderProtoConverter"}
{"level":"INFO","timestamp":"2025-08-15T18:32:12,528","thread":"main","file":"QueryBuilderProtoConverterRegistry.java", "line":"50","message":"Registered 4 built-in query converters"}
{"level":"INFO","timestamp":"2025-08-15T18:32:12,528","thread":"main","file":"GrpcPlugin.java", "line":"287","message":"Registering 1 external QueryBuilderProtoConverter(s) with the registry"}
{"level":"INFO","timestamp":"2025-08-15T18:32:12,529","thread":"main","file":"GrpcPlugin.java", "line":"289","message":"Registering external converter: org.opensearch.knn.grpc.proto.request.search.query.KNNQueryBuilderProtoConverter (handles: KNN)"}
{"level":"INFO","timestamp":"2025-08-15T18:32:12,529","thread":"main","file":"QueryBuilderProtoConverterSpiRegistry.java", "line":"106","message":"Registered query converter for KNN: org.opensearch.knn.grpc.proto.request.search.query.KNNQueryBuilderProtoConverter"}

Related Issues

#18513

Check List

  • Functionality includes testing.
  • API changes companion pull request created, if applicable.
  • Public documentation issue/PR created, if applicable.

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.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 6, 2025

❌ Gradle check result for e709b11: 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?

@karenyrx karenyrx changed the title Make QueryBuilderProtoConverter and QueryBuilderProtoConverterRegistry an SPI [GRPC] Publish transport-grpc-spi for QueryBuilderProtoConverter and QueryBuilderProtoConverterRegistry Aug 6, 2025
@github-actions
Copy link
Contributor

❌ Gradle check result for eae0332: 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?

@github-actions
Copy link
Contributor

❌ Gradle check result for c7bdd89: 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?

@github-actions
Copy link
Contributor

✅ Gradle check result for f7b8c28: SUCCESS

@codecov
Copy link

codecov bot commented Aug 12, 2025

Codecov Report

❌ Patch coverage is 97.72727% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 72.90%. Comparing base (c012a51) to head (2c59866).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...transport/grpc/spi/QueryBuilderProtoConverter.java 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main   #18949      +/-   ##
============================================
- Coverage     72.92%   72.90%   -0.03%     
- Complexity    69779    69804      +25     
============================================
  Files          5665     5667       +2     
  Lines        320521   320532      +11     
  Branches      46396    46397       +1     
============================================
- Hits         233739   233675      -64     
- Misses        67854    68038     +184     
+ Partials      18928    18819     -109     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@karenyrx karenyrx force-pushed the tgspi branch 3 times, most recently from 03c2aa5 to 373f22e Compare August 13, 2025 01:46
@github-actions
Copy link
Contributor

❌ Gradle check result for 373f22e: 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?

@github-actions
Copy link
Contributor

❌ Gradle check result for 43258db: 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?

@github-actions
Copy link
Contributor

✅ Gradle check result for c4b79fc: SUCCESS

@karenyrx karenyrx marked this pull request as ready for review August 15, 2025 14:33
@karenyrx karenyrx requested a review from a team as a code owner August 15, 2025 14:33
@github-actions
Copy link
Contributor

✅ Gradle check result for 969470c: SUCCESS

Signed-off-by: Karen Xu <[email protected]>
@github-actions
Copy link
Contributor

✅ Gradle check result for 6e18175: SUCCESS

@github-actions
Copy link
Contributor

❌ Gradle check result for 3c79981: 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?

@github-actions
Copy link
Contributor

github-actions bot commented Sep 3, 2025

❌ Gradle check result for 42762d1: 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?

@github-actions
Copy link
Contributor

github-actions bot commented Sep 3, 2025

❌ Gradle check result for 5082498: 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?

@github-actions
Copy link
Contributor

github-actions bot commented Sep 9, 2025

✅ Gradle check result for d371c98: SUCCESS

Copy link
Contributor

@msfroh msfroh left a comment

Choose a reason for hiding this comment

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

I like this interface a lot now. It's a really simple and elegant way to hook in new proto converters.

Thanks @karenyrx!

@github-actions
Copy link
Contributor

❌ Gradle check result for 2c59866: 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?

@github-actions
Copy link
Contributor

✅ Gradle check result for 2c59866: SUCCESS

@msfroh msfroh merged commit 1ca590a into opensearch-project:main Sep 11, 2025
58 of 60 checks passed
jainankitk pushed a commit to jainankitk/OpenSearch that referenced this pull request Sep 22, 2025
…QueryBuilderProtoConverterRegistry (opensearch-project#18949)

---------

Signed-off-by: Karen Xu <[email protected]>
Signed-off-by: Karen X <[email protected]>
Signed-off-by: Michael Froh <[email protected]>
Co-authored-by: Michael Froh <[email protected]>
jainankitk pushed a commit to jainankitk/OpenSearch that referenced this pull request Sep 22, 2025
…QueryBuilderProtoConverterRegistry (opensearch-project#18949)

---------

Signed-off-by: Karen Xu <[email protected]>
Signed-off-by: Karen X <[email protected]>
Signed-off-by: Michael Froh <[email protected]>
Co-authored-by: Michael Froh <[email protected]>
Signed-off-by: Ankit Jain <[email protected]>
jainankitk pushed a commit to jainankitk/OpenSearch that referenced this pull request Sep 22, 2025
…QueryBuilderProtoConverterRegistry (opensearch-project#18949)

---------

Signed-off-by: Karen Xu <[email protected]>
Signed-off-by: Karen X <[email protected]>
Signed-off-by: Michael Froh <[email protected]>
Co-authored-by: Michael Froh <[email protected]>
Signed-off-by: Ankit Jain <[email protected]>
asimmahmood1 pushed a commit to jainankitk/OpenSearch that referenced this pull request Sep 23, 2025
…QueryBuilderProtoConverterRegistry (opensearch-project#18949)

---------

Signed-off-by: Karen Xu <[email protected]>
Signed-off-by: Karen X <[email protected]>
Signed-off-by: Michael Froh <[email protected]>
Co-authored-by: Michael Froh <[email protected]>
pranikum pushed a commit to pranikum/OpenSearch that referenced this pull request Sep 23, 2025
…QueryBuilderProtoConverterRegistry (opensearch-project#18949)

---------

Signed-off-by: Karen Xu <[email protected]>
Signed-off-by: Karen X <[email protected]>
Signed-off-by: Michael Froh <[email protected]>
Co-authored-by: Michael Froh <[email protected]>
vinaykpud pushed a commit to vinaykpud/OpenSearch that referenced this pull request Sep 26, 2025
…QueryBuilderProtoConverterRegistry (opensearch-project#18949)

---------

Signed-off-by: Karen Xu <[email protected]>
Signed-off-by: Karen X <[email protected]>
Signed-off-by: Michael Froh <[email protected]>
Co-authored-by: Michael Froh <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants