[GRPC][Geographic queries] Implement GRPC GeoBoundingBox, GeoDistance queries

[karenyrx]

Description

Stacked on top of #19447
Implement GRPC GeoBoundingBox, GeoDistance queries

Related Issues

Resolves #19450

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]

❕ Gradle check result for 7c676dc: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

[codecov]

Codecov Report

❌ Patch coverage is 90.64039% with 19 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.00%. Comparing base (75fd8a6) to head (b7e6d83).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
...ch/query/GeoBoundingBoxQueryBuilderProtoUtils.java	90.52%	3 Missing and 6 partials ⚠️
...earch/query/GeoDistanceQueryBuilderProtoUtils.java	85.93%	3 Missing and 6 partials ⚠️
.../grpc/proto/request/common/GeoPointProtoUtils.java	96.42%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #19451      +/-   ##
============================================
+ Coverage     72.96%   73.00%   +0.03%     
- Complexity    70178    70281     +103     
============================================
  Files          5695     5700       +5     
  Lines        321939   322140     +201     
  Branches      46580    46614      +34     
============================================
+ Hits         234917   235173     +256     
+ Misses        68096    68045      -51     
+ Partials      18926    18922       -4     

☔ 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.

[github-actions]

✅ Gradle check result for 6c241a8: SUCCESS

[cwperks]

The changes in this PR LGTM.

In general, I would really like to aim for a common place to do all ser/de. i.e. if an ActionRequest (or any serializable) object is modified then I want to write the serialization/deserialization once as is done in the Writeable.writeTo(StreamOutput out) or a constructor that accepts StreamInput.

It seems like we will now need to update many places when serializable objects are modified which increases the surface area for errors.

Serialization errors can be more difficult to catch because they arise in mixed clusters when nodes of different versions are trying to communicate with one another. If 2 nodes are not speaking the same language (i.e. serializing/deserializing what the other expects), then they throw exceptions.

To catch these issues as early as possible, OpenSearch does extensive backward compatibility (BWC) testing to run with different scenarios. In general it starts with 3 nodes of the old version and does a rolling upgrade by killing and restarting nodes one at a time. The nodes that are added are of the new version so it goes 1/3 upgraded, 2/3 upgraded and then fully upgraded and runs tests at each step along the way.

If there are not existing bwc tests utilizing the gPRC transport then I strongly urge that there is a strategy put together to add them.

[karenyrx]

Thanks @cwperks for the reviews.

Serialization errors can be more difficult to catch because they arise in mixed clusters when nodes of different versions are trying to communicate with one another. If 2 nodes are not speaking the same language (i.e. serializing/deserializing what the other expects), then they throw exceptions.

One thing to clarify - grpc transport is only applicable to client-server communication not node-to-node, so we shuold not be seeing failures in node to node comms. Though depending on which node the client hits, it could have different serialization results which is not ideal. I think it is easy to catch gRPC breaking changes though - as a breaking protobuf change is quite evident (e.g. fields are renamed or renumbered or removed).

I think what is the callenging is actually not to ensure grpc version x+1 does not introduce breaking changes from grpc version x; but rather, how to keep rest and grpc in sync. For this, for now we could consider something like this to handle it:

• case 1: removed or changed fields
  ideally, unit tests should exist for each rest field that map to a corresponding grpc field -> so if an existing rest field is changed the unit test fails and reminds the author to make a change to the grpc side too
  however today, a comprehensive set of unit tests like this does not exist for every single proto converter (some do). It's a known-ish issue, tracked in [Feature Request] GRPC Parity #18311
• case 2: added/new fields
  if a new field is added on rest side, it should also be added to the API specification. since the spec is used to convert to the protobufs, ideally a human reviewing the auto-generated protobufs should catch a new field was added and take it as an action item to add it to the server implementation too. This process can be somewhat automated/have alerts for it as well in the future

[github-actions]

✅ Gradle check result for 33fa9b0: SUCCESS

[github-actions]

❌ Gradle check result for 7aaf591: null

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]

❌ Gradle check result for 8f030a6: 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]

❌ Gradle check result for 826dd69: 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]

❌ Gradle check result for 5832bc1: 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]

❕ Gradle check result for b7e6d83: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.