Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed

Conversation

VachaShah
Copy link
Collaborator

@VachaShah VachaShah commented Jan 17, 2024

Description

This PR is part 1 of the changes and POC done in #10684. It contains the following changes:

  1. QueryFetchSearchResult as a proto message.
  2. A new interface for serialization and deserialization using Protobuf - ProtobufWriteable.
  3. Node-to-node communication with protobuf for a basic response QueryFetchSearchResult during a search API.
  4. Feature flag for the above changes.
  5. Supporting framework for adding protobuf.

Incoming changes in this PR:

  • Add tests for protobuf messages and node-to-node communication for QueryFetchSearchResult.

Next part of changes:

  • Make the feature flag dynamic so that it can be accessed via cluster settings.
  • Add support for Aggregations and some remaining fields.
  • Abstractions which support making the transport layer pluggable.

Related Issues

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

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.

Copy link
Contributor

github-actions bot commented Jan 17, 2024

Compatibility status:

Checks if related components are compatible with change 324c58f

Incompatible components

Incompatible components: [https://github.com/opensearch-project/asynchronous-search.git]

Skipped components

Compatible components

Compatible components: [https://github.com/opensearch-project/custom-codecs.git, https://github.com/opensearch-project/performance-analyzer-rca.git, https://github.com/opensearch-project/cross-cluster-replication.git, https://github.com/opensearch-project/flow-framework.git, https://github.com/opensearch-project/reporting.git, https://github.com/opensearch-project/job-scheduler.git, https://github.com/opensearch-project/security.git, https://github.com/opensearch-project/geospatial.git, https://github.com/opensearch-project/opensearch-oci-object-storage.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/anomaly-detection.git, https://github.com/opensearch-project/security-analytics.git, https://github.com/opensearch-project/performance-analyzer.git, https://github.com/opensearch-project/ml-commons.git, https://github.com/opensearch-project/notifications.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]

Copy link
Contributor

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

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

First, it's awesome that this works end to end.

Some high level thoughts on the implementation. The way I imagined this would look is that the transport parts are abstracted enough that we can add another transport protocol after protobuf. This means putting current transport and protobuf transport at the same level, not switched by an if statement when the message is protobuf, and not adding protobuf to the current hierarchy without extracting the current transport protocol. WDYT?

@reta
Copy link
Collaborator

reta commented Jan 17, 2024

@VachaShah ever more endorsements than @dblock (:-D), but on general note: the Protobuf support (as of this pull request) is very much entangled with the transport APIs and implementation (mostly every layer has to be aware of it). I think this is a good opportunity to make breaking changes so adding new serialization protocols could be more or less simple (let say we would need to add FlatBuffers, you name it, what would be the cost of that)

@VachaShah VachaShah force-pushed the search-protobuf-part1 branch from 449a4ff to ed00f89 Compare April 9, 2024 19:59
Copy link
Contributor

github-actions bot commented Apr 9, 2024

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

@VachaShah
Copy link
Collaborator Author

The transport abstractions from PR #12967 are now merged in this PR and protobuf is incorporated in that framework.

Copy link
Contributor

✅ Gradle check result for 324c58f: SUCCESS

@dblock
Copy link
Member

dblock commented Apr 12, 2024

@VachaShah this is still a meaty change, do you want us to go over it, or is there a smaller piece extractable?

@VachaShah
Copy link
Collaborator Author

@VachaShah this is still a meaty change, do you want us to go over it, or is there a smaller piece extractable?

@dblock I am working on a PR for extracting protos for some search classes in another PR to make it easier to review. Then this PR can hopefully be for tying the pieces together.

@VachaShah VachaShah mentioned this pull request Apr 12, 2024
8 tasks
@VachaShah
Copy link
Collaborator Author

Extracted protos and its serialization in PR #13178.

@getsaurabh02 getsaurabh02 added v2.15.0 Issues and PRs related to version 2.15.0 and removed v2.14.0 labels Apr 25, 2024
@opensearch-trigger-bot
Copy link
Contributor

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

@opensearch-trigger-bot opensearch-trigger-bot bot added stalled Issues that have stalled and removed stalled Issues that have stalled labels May 26, 2024
@VachaShah
Copy link
Collaborator Author

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/search-protobuf-part1.

@VachaShah VachaShah closed this Jul 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v2.15.0 Issues and PRs related to version 2.15.0
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

8 participants