[Extensions] Adding versioning support for registering Rest actions by saratvemulapalli · Pull Request #7302 · opensearch-project/OpenSearch

saratvemulapalli · 2023-04-25T20:53:01Z

Description

Moving RestActions request to Protobuf to support cross version compatibility.
Also this PR adds ExtensionIdentity as an abstract data container for Identity of extension.
In the follow up PR's I'll move over all places where uniqueId to use ExtensionIdentity.

Issues Resolved

Part of #7402
Also moves the needle for opensearch-project/opensearch-sdk-java#702

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.

Signed-off-by: Sarat Vemulapalli <vemulapallisarat@gmail.com>

github-actions · 2023-04-25T21:28:57Z

Gradle Check (Jenkins) Run Completed with:

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

saratvemulapalli · 2023-04-25T22:20:13Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌

URL: https://build.ci.opensearch.org/job/gradle-check/14550/

CommitID: 8ef8b55
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?

[org.opensearch.indices.replication.SegmentReplicationIT.testReplicaHasDiffFilesThanPrimary](https://build.ci.opensearch.org/job/gradle-check/14550/testReport/junit/org.opensearch.indices.replication/SegmentReplicationIT/testReplicaHasDiffFilesThanPrimary/)
[org.opensearch.indices.replication.SegmentReplicationIT.testReplicaHasDiffFilesThanPrimary](https://build.ci.opensearch.org/job/gradle-check/14550/testReport/junit/org.opensearch.indices.replication/SegmentReplicationIT/testReplicaHasDiffFilesThanPrimary_2/)
[org.opensearch.indices.replication.SegmentReplicationIT.testReplicaHasDiffFilesThanPrimary](https://build.ci.opensearch.org/job/gradle-check/14550/testReport/junit/org.opensearch.indices.replication/SegmentReplicationIT/testReplicaHasDiffFilesThanPrimary_3/)
[org.opensearch.indices.replication.SegmentReplicationIT.testReplicaHasDiffFilesThanPrimary](https://build.ci.opensearch.org/job/gradle-check/14550/testReport/junit/org.opensearch.indices.replication/SegmentReplicationIT/testReplicaHasDiffFilesThanPrimary_4/)

Signed-off-by: Sarat Vemulapalli <vemulapallisarat@gmail.com>

github-actions · 2023-04-25T22:56:03Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/14551/
CommitID: 0c790e2

dblock · 2023-04-26T15:36:06Z

Looks good, but I expected some tests to have to change as well.

dbwiddis

LGTM but left a comment about future-proofing for a known feature request that we should consider doing.

dbwiddis · 2023-04-26T19:29:44Z

server/src/main/java/org/opensearch/extensions/rest/RegisterRestActionsRequest.java

+            .setIdentity(identity)
+            .addAllRestActions(restActions)
+            .addAllDeprecatedRestActions(deprecatedRestActions)
+            .build();


I haven't had time to follow up with a thorough design, but I think there may need to be one more bit of information passed from the extension here, and it would be nice to do it with this PR rather than a separate one.

It relates to SDK #355 and replaced/deprecated routes. Basically:

we want the deprecated routes coming from the extensions to be verbatim (with /_plugin prefix) without making any changes on OpenSearch side (but with possible exception handling if the plugin is installed and the prefix is already taken, per the linked issue)

we want active/new routes coming from the extension to use the uniqueId and /_extension prefix but also possibly register with a /_plugin prefix, per the linked issue.

So we may want to actually include some sort of "prefix" string.

This may be exactly what you are doing with ExtensionIdentity but I'm thinking perhaps that may need a second string argument.

Sorry this isn't fully thought out, but perhaps something we should all agree on (and possibly implement SDK #355 shortly after this).

(To be clear, my suggestion is to add a second string field to Identity, so it would have a uniqueID and a prefix.)

Had a chat with @dbwiddis, we'll add a new bool in a different PR to solve it.
Will try to get this PR merged as the framework.

Signed-off-by: Sarat Vemulapalli <vemulapallisarat@gmail.com>

github-actions · 2023-05-02T19:40:46Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/14748/
CommitID: 01ab43a

Signed-off-by: Sarat Vemulapalli <vemulapallisarat@gmail.com>

github-actions · 2023-05-02T20:31:24Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/14751/
CommitID: 41eebca

codecov-commenter · 2023-05-02T20:36:36Z

Codecov Report

Merging #7302 (91dd606) into main (1151308) will increase coverage by 0.13%.
The diff coverage is 70.83%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff              @@
##               main    #7302      +/-   ##
============================================
+ Coverage     70.56%   70.69%   +0.13%     
- Complexity    59547    59594      +47     
============================================
  Files          4876     4876              
  Lines        285813   285816       +3     
  Branches      41162    41162              
============================================
+ Hits         201686   202065     +379     
+ Misses        67519    67122     -397     
- Partials      16608    16629      +21

Impacted Files	Coverage Δ
...va/org/opensearch/extensions/ExtensionRequest.java	`68.00% <40.00%> (-2.84%)`	⬇️
...ch/extensions/rest/RegisterRestActionsRequest.java	`75.00% <78.94%> (+5.76%)`	⬆️

... and 462 files with indirect coverage changes

saratvemulapalli · 2023-05-02T22:15:23Z

Looks good, but I expected some tests to have to change as well.

Thanks for the review. Added few tests. Could you take a look again @dblock

Signed-off-by: Sarat Vemulapalli <vemulapallisarat@gmail.com>

github-actions · 2023-05-02T22:46:34Z

Gradle Check (Jenkins) Run Completed with:

RESULT: UNSTABLE ❕
TEST FAILURES:

      1 org.opensearch.search.SearchWeightedRoutingIT.testSearchAggregationWithNetworkDisruption_FailOpenEnabled
      1 org.opensearch.index.remote.RemoteRefreshSegmentPressureServiceTests.testValidateSegmentUploadLag

URL: https://build.ci.opensearch.org/job/gradle-check/14759/
CommitID: 91dd606
Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

opensearch-trigger-bot · 2023-05-02T22:52:48Z

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-7302-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 7c3c411becfea1933f93bfce4e411c189757a06e
# Push it to GitHub
git push --set-upstream origin backport/backport-7302-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-7302-to-2.x.

…pensearch-project#7302) Add cross version support for register Rest Actions for extensions Signed-off-by: Sarat Vemulapalli <vemulapallisarat@gmail.com>

kotwanikunal · 2023-05-03T17:14:10Z

@saratvemulapalli I just pulled this commit into my local workspace and I faced a compile error for missing classes.
I ran ./gradlew generateProto which seemed to have resolved the issue, but I think we should document it as a part of the dev guide.

saratvemulapalli · 2023-05-03T17:21:48Z

@saratvemulapalli I just pulled this commit into my local workspace and I faced a compile error for missing classes. I ran ./gradlew generateProto which seemed to have resolved the issue, but I think we should document it as a part of the dev guide.

My bad, absolutely. I'll send out a PR.
Also Im curious how did you compile? generateProto is a dependent task for compileJava

kotwanikunal · 2023-05-03T17:37:02Z

@saratvemulapalli I just pulled this commit into my local workspace and I faced a compile error for missing classes. I ran ./gradlew generateProto which seemed to have resolved the issue, but I think we should document it as a part of the dev guide.

My bad, absolutely. I'll send out a PR. Also Im curious how did you compile? generateProto is a dependent task for compileJava

Strange. I was trying to run a test directly on IntelliJ and it did not auto compile.

…7302) (#7373) Add cross version support for register Rest Actions for extensions Signed-off-by: Sarat Vemulapalli <vemulapallisarat@gmail.com>

…pensearch-project#7302) Add cross version support for register Rest Actions for extensions Signed-off-by: Sarat Vemulapalli <vemulapallisarat@gmail.com> Signed-off-by: Shivansh Arora <hishiv@amazon.com>

Adding protobuf support for RestActions

8ef8b55

Signed-off-by: Sarat Vemulapalli <vemulapallisarat@gmail.com>

saratvemulapalli requested review from Bukhtawar, CEHENKLE, Rishikesh1159, VachaShah, anasalkouz, andrross, dblock, dreamer-89, gbbafna, kartg, kotwanikunal, mch2, nknize, owaiskazi19, reta, ryanbogan, setiah, shwetathareja, tlfeng and xuezhou25 as code owners April 25, 2023 20:53

saratvemulapalli mentioned this pull request Apr 25, 2023

Adding companion changes to RegisterRestActions Protobuf changes opensearch-project/opensearch-sdk-java#704

Merged

saratvemulapalli changed the title ~~[Extensions] Adding versioning support for RestActions~~ [Extensions] Adding versioning support for registering Rest actions Apr 25, 2023

saratvemulapalli mentioned this pull request Apr 25, 2023

[BUG] Test org.opensearch.indices.replication.SegmentReplicationIT.testReplicaHasDiffFilesThanPrimary is flaky #6885

Closed

Adding changelog

0c790e2

Signed-off-by: Sarat Vemulapalli <vemulapallisarat@gmail.com>

dbwiddis reviewed Apr 26, 2023

View reviewed changes

saratvemulapalli mentioned this pull request May 3, 2023

[Meta] Move over existing extensions APIs to work across versions #7402

Closed

16 tasks

Adding unit tests

01ab43a

Signed-off-by: Sarat Vemulapalli <vemulapallisarat@gmail.com>

Merge branch 'main' into restactions-protobuf

41eebca

Signed-off-by: Sarat Vemulapalli <vemulapallisarat@gmail.com>

Merge branch 'main' into restactions-protobuf

91dd606

Signed-off-by: Sarat Vemulapalli <vemulapallisarat@gmail.com>

owaiskazi19 approved these changes May 2, 2023

View reviewed changes

saratvemulapalli added extensions backport 2.x Backport to 2.x branch labels May 2, 2023

saratvemulapalli merged commit 7c3c411 into opensearch-project:main May 2, 2023

saratvemulapalli deleted the restactions-protobuf branch May 2, 2023 22:52

saratvemulapalli mentioned this pull request May 2, 2023

[Backport 2.x] Adding versioning support for registering Rest actions #7373

Merged

6 tasks

saratvemulapalli mentioned this pull request May 3, 2023

[Extensions] Adding cross version support for registering Transport Actions #7403

Merged

6 tasks

owaiskazi19 mentioned this pull request May 22, 2023

Added @dbwiddis to OpenSearch maintainers #7665

Merged

6 tasks

Conversation

saratvemulapalli commented Apr 25, 2023

Description

Issues Resolved

Check List

Uh oh!

github-actions bot commented Apr 25, 2023

Gradle Check (Jenkins) Run Completed with:

Uh oh!

saratvemulapalli commented Apr 25, 2023

Gradle Check (Jenkins) Run Completed with:

Uh oh!

github-actions bot commented Apr 25, 2023

Gradle Check (Jenkins) Run Completed with:

Uh oh!

dblock commented Apr 26, 2023

Uh oh!

dbwiddis left a comment

Choose a reason for hiding this comment

Uh oh!

dbwiddis Apr 26, 2023

Choose a reason for hiding this comment

Uh oh!

dbwiddis May 1, 2023

Choose a reason for hiding this comment

Uh oh!

saratvemulapalli May 2, 2023

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented May 2, 2023

Gradle Check (Jenkins) Run Completed with:

Uh oh!

github-actions bot commented May 2, 2023

Gradle Check (Jenkins) Run Completed with:

Uh oh!

codecov-commenter commented May 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

saratvemulapalli commented May 2, 2023

Uh oh!

github-actions bot commented May 2, 2023

Gradle Check (Jenkins) Run Completed with:

Uh oh!

opensearch-trigger-bot bot commented May 2, 2023

Uh oh!

kotwanikunal commented May 3, 2023

Uh oh!

saratvemulapalli commented May 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kotwanikunal commented May 3, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov-commenter commented May 2, 2023 •

edited

Loading

saratvemulapalli commented May 3, 2023 •

edited

Loading