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.

Sign up for GitHub

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

Jump to bottom

Cat Nodes API with Protobuf #9097

Closed

VachaShah wants to merge 38 commits into opensearch-project:main from VachaShah:poc-cat-nodes-protobuf

Collaborator

VachaShah commented Aug 3, 2023 •

edited

Loading

Description

The purpose of this draft PR is to create a new cat nodes API with protobuf as a serialization/de-serialization mechanism for node-to-node communication. We also benchmark the performance results in comparison to the original cat nodes API. This PR creates a separate path for the new API but when these changes are to be merged, a lot of code in the original API can be replaced with the newer approach. For the purpose of this POC, as of now the files are separate.

This PR represents the work done for the POC and is not be merged. The changes will be raised, reviewed and merged in incremental PRs.

Related Issues

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed per the DCO using --signoff
Commit changes are listed out in CHANGELOG.md file (See: Changelog)

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.

VachaShah added 30 commits

August 2, 2023 17:13


          Adding BaseWriteable and ProtobufWriteable

9be33d7

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


          Adding proto messages for TaskId and TaskResourceStats

0034b0a

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


          Adding Task related classes with protobuf integration

cdb6806

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


          Adding ProtobufStreamInput and ProtobufStreamOutput for additional st…

c8c2f53

…ream related functionality

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


          Adding TransportMessage and TransportRequest classes with protobuf in…

a2be2fa

…tegration

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


          Fixing build and precommit

c0bbf92

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


          Adding protobuf integrations for client, transport, request

551fe82

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


          Fixing build and integrating protobuf for classes related to RestNode…

a1ab589

…sAction

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


          Fixing node crashes

8e57034

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


          Fixes

a5ce258

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


          Fixing nodes api response for protobuf

0d76b3b

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


          ProtobufClusterState to ClusterState

e6f6190

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


          Fixing cluster manager x to *

b94a49e

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


          Eliminating a lot of protobuf classes to merge with original classes …

cb320c7

…- mainly TransportService to try to fix multi node problem

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


          This fixes the single node calls after the refactoring in the previou…

13c4568

…s commit

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


          Trying serialization and deserialization across nodes

b659260

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


          Debugging and fixing compile errors

e96f3e0

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


          Trying proto message serde across nodes with tests and example request

496cdbb

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


          Changes for multi node - working on single and multi node

8aab04b

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


          Calculating time

b583179

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


          Removing sysouts

515110a

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


          Cleaning up code

f94f165

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


          Ignoring proto generated classes

35d8906

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


          Cleaning up more code

d511930

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


          Cleaning up CodedInputStream, CodedOutputStream and TryWriteable

562e6c3

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


          Performance improvements

d5f3a0e

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


          Fixing compile errors after merging with main

2f423b9

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


          Renaming proto messages and related classes

eeeb40f

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


          Fixing precommit failures

8f5a327

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


          Cleaning up code

d409e71

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

VachaShah and others added 8 commits

August 3, 2023 07:07


          Improvements

967d26b

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


          [Refactor] Network and Transport common classes to Libraries (opensea…

0e9a86e

…rch-project#9073)

This commit refactors the following network and transport libraries to
the opensearch common and core libraries respectively:

* o.o.common.network.Cidrs -> :libs:opensearch-common
* o.o.common.network.InetAddresses -> :libs:opensearch-common
* o.o.common.network.NetworkAddress -> :libs:opensearch-common
* o.o.common.transport.NetworkExceptionHelper -> :libs:opensearch-common
* o.o.common.transport.PortsRange -> :libs:opensearch-common

* o.o.common.transport.TransportAddress -> :libs:opensearch-core
* o.o.common.transport.BoundTransportAddress -> :libs:opensearch-core
* o.o.transport.TransportMessage -> :libs:opensearch-core
* o.o.transport.TransportResponse -> :libs:opensearch-core

The purpose is to reduce the change surface area of the core APIs to
minimize impact to downstream consumers while moving toward establishing
a formal API for cloud native or serverless implementations.

Signed-off-by: Nicholas Walter Knize <nknize@apache.org>


          [Snapshot Interop] Add Logic in Lock Manager to cleanup stale data po…

d73fd6a

…st index deletion. (opensearch-project#8472)

Signed-off-by: Harish Bhakuni <hbhakuni@amazon.com>


          Avoid duplicate indexing in case of SegRep enabled indices' translog …

f3a17fc

…replay (opensearch-project#8578)


Signed-off-by: Gaurav Bafna <gbbafna@amazon.com>


          Fix flaky test testStatsOnShardUnassigned in RemoteStoreStatsIT (open…

64a7457

…search-project#9057)

---------

Signed-off-by: Ashish Singh <ssashish@amazon.com>


          Fix test testDropPrimaryDuringReplication and clean up ReplicationChe…

…ckpoint validation (opensearch-project#8889)

* Fix test testDropPrimaryDuringReplication and clean up ReplicationCheckpoint validation.

This test is now occasionally failing with replicas having 0 documents. This occurs in a couple of ways:
1. After dropping the old primary the new primary is not publishing a checkpoint to replicas unless it indexes docs from translog after flipping to primary mode.
If there is nothing to index, it will not publish a checkpoint, but the other replica could have never sync'd with the original primary and be left out of date.
- This PR fixes this by force publishing a checkpoint after the new primary flips to primary mode.
2. The replica receives a checkpoint post failover and cancels its sync with the former primary that is still active, recognizing a primary term bump.
However this cancellation is async and immediately starting a new replication event could fail as its still replicating.
- This PR fixes this by attempting to process the latest received checkpoint on failure, if the shard is not failed and still behind.

This PR also introduces a few changes to ensure the accuracy of the ReplicationCheckpoint tracked on primary & replicas.
- Ensure the checkpoint stored in SegmentReplicationTarget is the checkpoint passed from the primary and not locally computed. This ensures checks for primary term are accurate and not using a locally compued operationPrimaryTerm.
- Introduces a refresh listener for both primary & replica to update the ReplicationCheckpoint and store it in replicationTracker post refresh rather than redundantly computing when accessed.
- Removes unnecessary onCheckpointPublished method used to start replication timers manually. This will happen automatically on primaries once its local cp is updated.

Signed-off-by: Marc Handalian <handalm@amazon.com>

* Handle NoSuchFileException when attempting to delete decref'd files.

To avoid divergent logic with remote store, we always incref/decref the segmentinfos.files(true) which includes the segments_n file.
Decref to 0 will attempt to delete the file from the store and its possible this _n file does not yet exist. This change will ignore if we get a noSuchFile while attempting to delete.

Signed-off-by: Marc Handalian <handalm@amazon.com>

* Add more unit tests.

Signed-off-by: Marc Handalian <handalm@amazon.com>

* Clean up IndexShardTests.testCheckpointReffreshListenerWithNull

Signed-off-by: Marc Handalian <handalm@amazon.com>

* Remove unnecessary catch for NoSuchFileException.

Signed-off-by: Marc Handalian <handalm@amazon.com>

* Add another test for non segrep.

Signed-off-by: Marc Handalian <handalm@amazon.com>

* PR Feedback.

Signed-off-by: Marc Handalian <handalm@amazon.com>

* re-compute replication checkpoint on primary promotion.

Signed-off-by: Marc Handalian <handalm@amazon.com>

---------

Signed-off-by: Marc Handalian <handalm@amazon.com>


          Getting latest changes from main

46aaea2

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


          Merge branch 'main' into poc-cat-nodes-protobuf

c05f5aa

Contributor

github-actions bot commented Aug 3, 2023

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/21815/
CommitID: c05f5aa
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?

Contributor

opensearch-trigger-bot bot commented Aug 3, 2023

Compatibility status:



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

BUILD SUCCESSFUL in 33m 18s

Contributor

gashutos commented Aug 14, 2023 •

edited

Loading

Strictly speaking from performance dimension,
I was just benchmarking netty http layer for search path. We hardly spend 1 or 2 ms in netty layer per request.
Any benchmark available for protobuf ?

VachaShah mentioned this pull request

Introduce protobuf serialization/deserialization support for transport requests/responses #9737

Closed

6 tasks

Contributor

opensearch-trigger-bot bot commented Sep 13, 2023

This PR is stalled because it has been open for 30 days with no activity. Remove stalled label or comment or this will be closed in 7 days.

opensearch-trigger-bot bot added the stalled label

VachaShah mentioned this pull request

[RFC] Protobuf in OpenSearch #6844

Open

Member

dblock commented Oct 2, 2023

@VachaShah Given #6844 (comment), do you have any ideas about how to bring this into OpenSearch as maybe an experimental feature? What are next steps?

Collaborator Author

VachaShah commented Oct 2, 2023

@VachaShah Given #6844 (comment), do you have any ideas about how to bring this into OpenSearch as maybe an experimental feature? What are next steps?

I am working on getting the changes from the POC in #9097 to merge in the repo. We are also working on getting the numbers for APIs like search.

opensearch-trigger-bot bot removed the stalled label

peternied reviewed

View reviewed changes

libs/core/src/main/java/org/opensearch/core/transport/TransportMessage.java

+                  * Constructs a new transport message with the data from the {@link byte[]}. This is
+                  * currently a no-op
+                  */
+                  public TransportMessage(byte[] in) {}

Member

peternied Oct 12, 2023

Could we avoid adding this change by reusing the existing StreamInput?

peternied requested changes

View reviewed changes

Member

peternied left a comment

This is really exciting work - thanks for getting this out there.

For the purpose of this POC, as of now the files are separate.

What our your thoughts on the end state for this change? I'm resistant to merge so much duplicate code.

libs/core/src/main/java/org/opensearch/core/common/io/stream/ProtobufWriteable.java

+                      * @param out Output to write the {@code value} too
+                      * @param value The value to add
+                      */
+                      void write(OutputStream out, V value) throws IOException;

Member

peternied Oct 12, 2023

Its strange than the writer is via an OutputStream, but the reader isn't a symetrical version that uses InputStream. Can we align the types to be consistant, be it bytes[] or *Stream?

server/src/main/java/org/opensearch/tasks/ProtobufTask.java

+              *
+              * @opensearch.internal
+              */
+              public class ProtobufTask {

Member

peternied Oct 12, 2023

This class is nearly identical to the existing Task, why do we need a largely identical protobuf version? This seems to imply there is coupling between the existing OpenSearch data models and their serialized form. Can we decompose these relationships more?

peternied reviewed

View reviewed changes

Member

peternied left a comment •

edited

Loading

~ Duplicate comment ~

Collaborator Author

VachaShah commented Oct 12, 2023

This is really exciting work - thanks for getting this out there.

For the purpose of this POC, as of now the files are separate.

What our your thoughts on the end state for this change? I'm resistant to merge so much duplicate code.

Hi @peternied, thank you for your comments! This PR is not be merged, its a draft PR for the work done to get numbers for which I created a parallel API _cat/nodes_protobuf to compare with the original version. The changes to be merged will go in incremental PRs, which I will raise later. This PR is just out here in the draft state for the work done for the POC.

Collaborator Author

VachaShah commented Oct 12, 2023

This is really exciting work - thanks for getting this out there.

For the purpose of this POC, as of now the files are separate.

What our your thoughts on the end state for this change? I'm resistant to merge so much duplicate code.

Hi @peternied, thank you for your comments! This PR is not be merged, its a draft PR for the work done to get numbers for which I created a parallel API _cat/nodes_protobuf to compare with the original version. The changes to be merged will go in incremental PRs, which I will raise later. This PR is just out here in the draft state for the work done for the POC.

I think I was not clear in the description of the PR, so I added a comment about this as well.

Member

dblock commented Oct 13, 2023 •

edited

Loading

Any parts of this PR that can be merged? Otherwise I think we can document this in #6844 and close it? (And nice work!)

Collaborator Author

VachaShah commented Oct 17, 2023

Any parts of this PR that can be merged? Otherwise I think we can document this in #6844 and close it? (And nice work!)

Thank you @dblock! I will raise incremental PRs for this, so closing this one now. I will make sure to update #6844 to document this.

VachaShah closed this

VachaShah mentioned this pull request

[META] Protobuf for Search API #10684

Open

4 tasks

finnegancarroll mentioned this pull request

[META] <Leverage protobuf for serializing select node-to-node objects> #15308

Open

3 tasks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

peternied peternied requested changes

anasalkouz Awaiting requested review from anasalkouz anasalkouz will be requested when the pull request is marked ready for review anasalkouz is a code owner

andrross Awaiting requested review from andrross andrross will be requested when the pull request is marked ready for review andrross is a code owner

ashking94 Awaiting requested review from ashking94 ashking94 will be requested when the pull request is marked ready for review ashking94 is a code owner

Bukhtawar Awaiting requested review from Bukhtawar Bukhtawar will be requested when the pull request is marked ready for review Bukhtawar is a code owner

CEHENKLE Awaiting requested review from CEHENKLE CEHENKLE will be requested when the pull request is marked ready for review CEHENKLE is a code owner

dblock Awaiting requested review from dblock dblock will be requested when the pull request is marked ready for review dblock is a code owner

dbwiddis Awaiting requested review from dbwiddis dbwiddis will be requested when the pull request is marked ready for review dbwiddis is a code owner

gbbafna Awaiting requested review from gbbafna gbbafna will be requested when the pull request is marked ready for review gbbafna is a code owner

jed326 Awaiting requested review from jed326 jed326 will be requested when the pull request is marked ready for review jed326 is a code owner

kotwanikunal Awaiting requested review from kotwanikunal kotwanikunal will be requested when the pull request is marked ready for review kotwanikunal is a code owner

mch2 Awaiting requested review from mch2 mch2 will be requested when the pull request is marked ready for review mch2 is a code owner

msfroh Awaiting requested review from msfroh msfroh will be requested when the pull request is marked ready for review msfroh is a code owner

nknize Awaiting requested review from nknize nknize will be requested when the pull request is marked ready for review nknize is a code owner

owaiskazi19 Awaiting requested review from owaiskazi19 owaiskazi19 will be requested when the pull request is marked ready for review owaiskazi19 is a code owner

reta Awaiting requested review from reta reta will be requested when the pull request is marked ready for review reta is a code owner

Rishikesh1159 Awaiting requested review from Rishikesh1159 Rishikesh1159 will be requested when the pull request is marked ready for review Rishikesh1159 is a code owner

sachinpkale Awaiting requested review from sachinpkale sachinpkale will be requested when the pull request is marked ready for review sachinpkale is a code owner

saratvemulapalli Awaiting requested review from saratvemulapalli saratvemulapalli will be requested when the pull request is marked ready for review saratvemulapalli is a code owner

shwetathareja Awaiting requested review from shwetathareja shwetathareja will be requested when the pull request is marked ready for review shwetathareja is a code owner

sohami Awaiting requested review from sohami sohami will be requested when the pull request is marked ready for review sohami is a code owner

jainankitk Awaiting requested review from jainankitk jainankitk will be requested when the pull request is marked ready for review jainankitk is a code owner

linuxpi Awaiting requested review from linuxpi linuxpi will be requested when the pull request is marked ready for review linuxpi is a code owner

Labels

None yet