Adding protos for search classes by VachaShah · Pull Request #13178 · opensearch-project/OpenSearch

VachaShah · 2024-04-12T20:25:09Z

Description

Coming from #11910, extracting some classes for introducing proto structures in this PR for easier reviews. This PR mainly contains:

Proto messages for QuerySearchResult, FetchSearchResult and QueryFetchSearchResult.
Separated serialization logic for model classes (as discussed in QueryFetchSearchResult as a proto message and node-to-node communication with protobuf #11910 (comment)).
Feature flag for protobuf with search enabled.

Note:

Aggregations is not currently part of this effort. So it has not been converted to proto.
Transport specific protobuf logic will be part of the next PR.

Related Issues

Resolves #[Issue number to be closed when this PR is merged]
Part of #10684

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Failing checks are inspected and point to the corresponding known issue(s) (See: Troubleshooting Failing Builds)
Commits are signed per the DCO using --signoff
Commit changes are listed out in CHANGELOG.md file (See: Changelog)
Public documentation issue/PR created

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 · 2024-04-12T20:46:58Z

Compatibility status:

Checks if related components are compatible with change b6f1d25

Incompatible components

Skipped components

Compatible components

Compatible components: [https://github.com/opensearch-project/custom-codecs.git, https://github.com/opensearch-project/asynchronous-search.git, https://github.com/opensearch-project/performance-analyzer-rca.git, https://github.com/opensearch-project/flow-framework.git, https://github.com/opensearch-project/cross-cluster-replication.git, https://github.com/opensearch-project/job-scheduler.git, https://github.com/opensearch-project/reporting.git, https://github.com/opensearch-project/security.git, https://github.com/opensearch-project/opensearch-oci-object-storage.git, https://github.com/opensearch-project/geospatial.git, https://github.com/opensearch-project/common-utils.git, https://github.com/opensearch-project/k-nn.git, https://github.com/opensearch-project/neural-search.git, https://github.com/opensearch-project/security-analytics.git, https://github.com/opensearch-project/anomaly-detection.git, https://github.com/opensearch-project/performance-analyzer.git, https://github.com/opensearch-project/notifications.git, https://github.com/opensearch-project/ml-commons.git, https://github.com/opensearch-project/index-management.git, https://github.com/opensearch-project/observability.git, https://github.com/opensearch-project/alerting.git, https://github.com/opensearch-project/sql.git]

github-actions · 2024-04-12T21:08:49Z

❌ Gradle check result for 855f7ce: 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 · 2024-04-12T21:13:48Z

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

dblock

This works to quickly add protobuf, but I think we must refactor the classes to separate transport-specific parts in this one.

dblock · 2024-04-13T08:26:23Z

server/src/main/java/org/opensearch/common/util/FeatureFlags.java

+    /**
+     * Gates the functionality of integrating protobuf within search API and node-to-node communication.
+     */
+    public static final String PROTOBUF = "opensearch.experimental.feature.search_with_protobuf.enabled";


Should we call this something more generic since we may be adding more than search and gate all of protobuf transport behind it? What do you think about "opensearch.experimental.feature.transport.protobuf.enabled"?

Currently, only search related transport will be gated behind this since when we add the transport specific node to node protobuf structure (in the next PR), it will consider only search related classes for now. We can keep adding more classes once we expand the use case. That message would be https://github.com/opensearch-project/OpenSearch/pull/11910/files#diff-3b6cef54e769860b6ff96d7052208f6fe2ef8a86ea04aec232044984ba52e5db.

dblock · 2024-04-13T08:26:48Z

server/src/main/java/org/opensearch/common/util/FeatureFlags.java


    public static final Setting<Boolean> PLUGGABLE_CACHE_SETTING = Setting.boolSetting(PLUGGABLE_CACHE, false, Property.NodeScope);

+    public static final Setting<Boolean> PROTOBUF_SETTING = Setting.boolSetting(PROTOBUF, false, Property.NodeScope, Property.Dynamic);


PROTOBUF_TRANSPORT_SETTING

The feature/effort is not just transport specific though, protobuf is at the transport level and also at the API request response level. WDYT?

I think this variable and the name of the setting should match our intent. Won't we be rolling out the search API with protobuf support, then another API, etc.? What's the setting that will live the longest?

Either way this variable and the setting string should match I think.

The most generic one:

PROTOBUF = opensearch.experimental.feature.protobuf.enabled PROTOBUF_SETTING

Search specific.

PROTOBUF_SEARCH = opensearch.experimental.feature.protobuf.search.enabled PROTOBUF_SEEARCH_SETTING

I'm good with either, but I prefer (1) because I don't think we'll ever want to have 2 APIs with protobuf support enabled/disabled separately. I could totally be wrong, too.

dblock · 2024-04-13T08:30:44Z

server/src/main/java/org/opensearch/search/fetch/FetchSearchResult.java

        hits = new SearchHits(in);
    }

+    public FetchSearchResult(InputStream in) throws IOException {


This is a code smell. We have org.opensearch.core.common.io.stream.StreamInput and java.io.InputStream, and two constructors that do completely different things depending on whether we pass StreamInput or InputStream. We're lucky they are called different class names :)

This needs to be disambiguated, likely by implementing a base class and separate implementations for one vs. the other stream.

Both the constructors come from its base class implementing Writeable and BytesWriteable (this is not part of this PR). Let me see if I can make this a little more disambiguated.

We need to do the same things as with DocumentField - the class should not deal with all possible (de)serealizations

dblock · 2024-04-13T08:31:23Z

server/src/main/java/org/opensearch/search/fetch/FetchSearchResult.java

    public FetchSearchResult(ShardSearchContextId id, SearchShardTarget shardTarget) {
        this.contextId = id;
        setSearchShardTarget(shardTarget);
+        this.fetchSearchResultProto = FetchSearchResultProto.FetchSearchResult.newBuilder()


Transport-specific implementation needs to live in its own namespaces/classes.

This is not a transport specific implementation though. This is the response/request specific serialization and deserialization. Transport specific protobuf implementation will be separate which is sent over the wire of which this message is a part. Transport specific protobuf will be a part of the next PR.

The way it's written the request/response can serialize its way two ways, however the serialization primitives are all over the class. I think there are a few options (and possibly more).

Multiple serialize methods which take different stream types, and no locals anywhere else in the class.

Different serialize methods in the style of toString(), so serializeToNative(..) or serializeToProtobuf(..).

A base class FetchResult and child classes NativeFetchResult and ProtobufFetchResult that both have a serialize method that comes from NativeSerializable and ProtobufSerializable.

A helper serializer class NativeFetchResultSerializer and ProtobufFetchResultSerializer.

Maybe @reta has better ideas?

Thanks @dblock definitively +1 (https://github.com/opensearch-project/OpenSearch/pull/13178/files#r1566328274)

Doing something similar to DocumentField might not work since the response classes implement Writeable for native protocol (and will do BytesWriteable as well). But I think option 3 specified by @dblock might be a way to go where the specific protocol class can implement the related constructor for Writeable or BytesWriteable.

Thanks for the hint, we probably need to align the that between transport and protocol, I will look into it shortly, thanks!

@VachaShah OK, I think we won't be able to move on with this change before we solve the protocol handling by outbound communication (we sort of addressed the inbound only, but not outbound, TransportResponseHandler & are heavily relying on "native" serialization). In any case, I think the serde should be moved away from the response classes, and handled by the TransportResponseHandler, depending on the protocol of the inbound message).

I'm with this. I think the "right" way is to first PR splitting out the native serialization and deserialization out of FetchSearchResult. It's a hefty piece of work, but who dares wins.

Makes sense! I will look into this. For abstracting the outbound side of transport and adding support for multiple protocols on that side of transport, I have created #13293. Next, I am looking into decoupling Writeable for native serialization from TransportResponseHandler.

VachaShah · 2024-04-15T18:11:17Z

This works to quickly add protobuf, but I think we must refactor the classes to separate transport-specific parts in this one.

I think I missed adding some explanation here. Sorry for the confusion! This is not adding any transport specific implementations. In this PR, we are adding proto structures and serialization logic for API level response classes and model classes for search. The transport specific implementation for node to node message in protobuf structure will come in the next PR where these response classes will be used as part of it.

dblock · 2024-04-15T18:40:07Z

This works to quickly add protobuf, but I think we must refactor the classes to separate transport-specific parts in this one.

I think I missed adding some explanation here. Sorry for the confusion! This is not adding any transport specific implementations. In this PR, we are adding proto structures and serialization logic for API level response classes and model classes for search. The transport specific implementation for node to node message in protobuf structure will come in the next PR where these response classes will be used as part of it.

Understood. I think the fact that FetchResult has this line is what throws me off.

this.fetchSearchResultProto = FetchSearchResultProto.FetchSearchResult.newBuilder()

If you create a fetch result with a specific constructor that knows nothing about protobuf, and that will never be serialized to/from protobuf, you still have something related to protobuf. Can we do better?

...src/main/java/org/opensearch/common/document/serializer/DocumentFieldProtobufSerializer.java

server/src/main/java/org/opensearch/common/document/serializer/DocumentFieldSerializer.java

server/src/main/java/org/opensearch/search/SearchSortValues.java

github-actions · 2024-04-17T19:51:56Z

✅ Gradle check result for 288f99c: SUCCESS

codecov · 2024-04-17T20:48:44Z

Codecov Report

Attention: Patch coverage is 18.83117% with 375 lines in your changes missing coverage. Please review.

Project coverage is 71.48%. Comparing base (b15cb0c) to head (9c07269).
Report is 559 commits behind head on main.

Files	Patch %	Lines
...alizer/protobuf/SearchHitProtobufDeserializer.java	0.00%	123 Missing ⚠️
...lizer/protobuf/SearchHitsProtobufDeserializer.java	0.00%	90 Missing ⚠️
...er/protobuf/DocumentFieldProtobufDeserializer.java	0.00%	86 Missing ⚠️
...org/opensearch/search/fetch/FetchSearchResult.java	18.18%	16 Missing and 2 partials ⚠️
...org/opensearch/search/query/QuerySearchResult.java	86.20%	7 Missing and 5 partials ⚠️
...r/protobuf/NestedIdentityProtobufDeserializer.java	0.00%	12 Missing ⚠️
...r/protobuf/HighlightFieldProtobufDeserializer.java	0.00%	11 Missing ⚠️
...protobuf/SearchSortValuesProtobufDeserializer.java	0.00%	9 Missing ⚠️
...n/java/org/opensearch/search/SearchSortValues.java	0.00%	8 Missing ⚠️
...pensearch/search/fetch/QueryFetchSearchResult.java	0.00%	5 Missing ⚠️
... and 1 more

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #13178      +/-   ##
============================================
+ Coverage     71.42%   71.48%   +0.06%     
- Complexity    59978    61155    +1177     
============================================
  Files          4985     5064      +79     
  Lines        282275   287916    +5641     
  Branches      40946    41731     +785     
============================================
+ Hits         201603   205815    +4212     
- Misses        63999    65102    +1103     
- Partials      16673    16999     +326

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

dblock

I think @reta and I are saying the same thing. Refactor serialization/deserialization out of FetchSearchResult in a separate PR, then the protobuf one will be easy to add.

dblock · 2024-04-17T21:39:27Z

server/src/main/java/org/opensearch/common/util/FeatureFlags.java


    public static final Setting<Boolean> PLUGGABLE_CACHE_SETTING = Setting.boolSetting(PLUGGABLE_CACHE, false, Property.NodeScope);

+    public static final Setting<Boolean> PROTOBUF_SETTING = Setting.boolSetting(PROTOBUF, false, Property.NodeScope, Property.Dynamic);


I think this variable and the name of the setting should match our intent. Won't we be rolling out the search API with protobuf support, then another API, etc.? What's the setting that will live the longest?

Either way this variable and the setting string should match I think.

The most generic one:

PROTOBUF = opensearch.experimental.feature.protobuf.enabled PROTOBUF_SETTING

Search specific.

PROTOBUF_SEARCH = opensearch.experimental.feature.protobuf.search.enabled PROTOBUF_SEEARCH_SETTING

I'm good with either, but I prefer (1) because I don't think we'll ever want to have 2 APIs with protobuf support enabled/disabled separately. I could totally be wrong, too.

dblock · 2024-04-17T21:42:32Z

server/src/main/java/org/opensearch/search/fetch/FetchSearchResult.java

    public FetchSearchResult(ShardSearchContextId id, SearchShardTarget shardTarget) {
        this.contextId = id;
        setSearchShardTarget(shardTarget);
+        this.fetchSearchResultProto = FetchSearchResultProto.FetchSearchResult.newBuilder()


I'm with this. I think the "right" way is to first PR splitting out the native serialization and deserialization out of FetchSearchResult. It's a hefty piece of work, but who dares wins.

VachaShah · 2024-04-17T22:59:12Z

Thank you @reta and @dblock for the review! I will work on refactoring and abstraction for this. However, one of the things that come to mind is separating the native serialization from response classes will result in a breaking change (as far as I have looked into).

reta · 2024-04-17T23:11:13Z

. However, one of the things that come to mind is separating the native serialization from response classes will result in a breaking change (as far as I have looked into).

I think it depends how we would implement it, you might be very right and we may need to keep response classes implementing the default (native) serialization, but protobuf should be 100% out , I am sure

Signed-off-by: Vacha Shah <vachshah@amazon.com>

VachaShah · 2024-05-13T23:21:33Z

Update: Merged the latest changes from #13293. Working on architecture for keeping serde separate from classes for protobuf.

github-actions · 2024-05-14T00:10:18Z

✅ Gradle check result for 9c07269: SUCCESS

VachaShah · 2024-05-16T00:42:58Z

As one of the comments of this PR, I was looking into separating out serialization primitives from classes like FetchSearchResult. I have a draft of the idea in #13697. @dblock @reta How do you feel about the idea?

dblock

I commented on the next refactor in #13697 (comment).

This PR can become mergeable when FetchSearchResult doesn't have both native stream and protobuf mixed together.

opensearch-trigger-bot · 2024-06-15T15:21:29Z

This PR is stalled because it has been open for 30 days with no activity.

opensearch-trigger-bot · 2024-07-21T15:21:59Z

This PR is stalled because it has been open for 30 days with no activity.

VachaShah · 2024-07-23T04:30:02Z

Closing this PR since won't be working on this actively. If this change needs to be picked up, please feel free to use my branch https://github.com/VachaShah/OpenSearch/tree/add-protos-for-search.

VachaShah mentioned this pull request Apr 12, 2024

QueryFetchSearchResult as a proto message and node-to-node communication with protobuf #11910

Closed

12 tasks

dblock suggested changes Apr 13, 2024

View reviewed changes

reta reviewed Apr 15, 2024

View reviewed changes

...src/main/java/org/opensearch/common/document/serializer/DocumentFieldProtobufSerializer.java Outdated Show resolved Hide resolved

reta reviewed Apr 15, 2024

View reviewed changes

server/src/main/java/org/opensearch/common/document/serializer/DocumentFieldSerializer.java Outdated Show resolved Hide resolved

reta reviewed Apr 15, 2024

View reviewed changes

server/src/main/java/org/opensearch/search/SearchSortValues.java Show resolved Hide resolved

wbeckler added the v2.14.0 label Apr 15, 2024

VachaShah force-pushed the add-protos-for-search branch from b6f1d25 to 288f99c Compare April 17, 2024 19:04

dblock suggested changes Apr 17, 2024

View reviewed changes

VachaShah mentioned this pull request Apr 18, 2024

Abstracting outbound side of transport #13293

Merged

8 tasks

getsaurabh02 added v2.15.0 Issues and PRs related to version 2.15.0 and removed v2.14.0 labels Apr 29, 2024

VachaShah mentioned this pull request May 13, 2024

[META] Protobuf for Search API #10684

Open

4 tasks

VachaShah added 3 commits May 13, 2024 23:11

Adding protos for search classes

4e0aa21

Signed-off-by: Vacha Shah <vachshah@amazon.com>

Add changelog

ea34915

Signed-off-by: Vacha Shah <vachshah@amazon.com>

Addressing comments

9c07269

Signed-off-by: Vacha Shah <vachshah@amazon.com>

VachaShah force-pushed the add-protos-for-search branch from 288f99c to 9c07269 Compare May 13, 2024 23:20

VachaShah mentioned this pull request May 16, 2024

[WIP] Separate serialization logic from class #13697

Closed

9 tasks

dblock suggested changes May 16, 2024

View reviewed changes

opensearch-trigger-bot bot added stalled Issues that have stalled and removed stalled Issues that have stalled labels Jun 15, 2024

opensearch-trigger-bot bot added the stalled Issues that have stalled label Jul 21, 2024

VachaShah closed this Jul 23, 2024


		public static final Setting<Boolean> PLUGGABLE_CACHE_SETTING = Setting.boolSetting(PLUGGABLE_CACHE, false, Property.NodeScope);

		public static final Setting<Boolean> PROTOBUF_SETTING = Setting.boolSetting(PROTOBUF, false, Property.NodeScope, Property.Dynamic);

Conversation

VachaShah commented Apr 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Check List

Uh oh!

github-actions bot commented Apr 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Compatibility status:

Incompatible components

Skipped components

Compatible components

Uh oh!

github-actions bot commented Apr 12, 2024

Uh oh!

github-actions bot commented Apr 12, 2024

Uh oh!

dblock left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reta Apr 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VachaShah commented Apr 15, 2024

Uh oh!

dblock commented Apr 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Apr 17, 2024

Uh oh!

codecov bot commented Apr 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dblock left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VachaShah commented Apr 12, 2024 •

edited

Loading

github-actions bot commented Apr 12, 2024 •

edited

Loading

reta Apr 15, 2024 •

edited

Loading

dblock commented Apr 15, 2024 •

edited

Loading

codecov bot commented Apr 17, 2024 •

edited

Loading