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

[Feature/extensions] Modified local node response #4862

Merged
merged 9 commits into from
Oct 31, 2022
Merged

[Feature/extensions] Modified local node response #4862

merged 9 commits into from
Oct 31, 2022

Conversation

joshpalis
Copy link
Member

@joshpalis joshpalis commented Oct 20, 2022

Description

Added getter method to local node response to return the Discovery Node

Edit : Separate local Node Requests are not necessary to provide support for since the local node is already transported to the SDK during node initialization. Will now use this pull request to remove the local node request workflow from the project

Companion PR : opensearch-project/opensearch-sdk-java#202

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: Joshua Palis <[email protected]>
@joshpalis joshpalis self-assigned this Oct 20, 2022
@joshpalis joshpalis changed the title Modified local node request to return discovery node directly to caller [Feature/extensions] Modified local node response Oct 20, 2022
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

*/
public DiscoveryNode getLocalNode() {
return this.localNode;
}
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a small test to verify this getter value?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure ill add this in

Copy link
Member

Choose a reason for hiding this comment

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

(Style/entirely personal preference) Would really like this further up in the class than below the equals and hashcode, although others prefer getters and setters at the bottom.

Signed-off-by: Joshua Palis <[email protected]>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

Copy link
Member

@dbwiddis dbwiddis left a comment

Choose a reason for hiding this comment

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

LGTM with two minor comments!

@@ -485,6 +485,22 @@ public void testHandleActionListenerOnFailureRequest() throws Exception {
assertEquals("Test failure", extensionsOrchestrator.listener.getExceptionList().get(0).getMessage());
}

public void testLocalNodeResponse() throws Exception {
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for adding this test! But I'm thinking it should probably be in the same package as the LocalNodeResponse class (src/test/java/org/opensearch/cluster) as it's not specific to extensions.

*/
public DiscoveryNode getLocalNode() {
return this.localNode;
}
Copy link
Member

Choose a reason for hiding this comment

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

(Style/entirely personal preference) Would really like this further up in the class than below the equals and hashcode, although others prefer getters and setters at the bottom.

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@joshpalis
Copy link
Member Author

@dbwiddis Upon further exploration, I realized that the sendLocalNodeRequest is redundant since we already transport the local node to the SDK during extension initialization here. Moving forward, I shall remove the sendLocalNodeRequest workflow from the project and add a getter on the ExtensionRunner in order to provide access to the ExtensionRunner.opensearchNode, which is the local node that we are concerned with.

…al node is already transported to the opensearch SDK during extension intialization

Signed-off-by: Joshua Palis <[email protected]>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@codecov-commenter
Copy link

codecov-commenter commented Oct 21, 2022

Codecov Report

Merging #4862 (a41b8f0) into feature/extensions (9ea149e) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@                   Coverage Diff                    @@
##             feature/extensions    #4862      +/-   ##
========================================================
- Coverage                 70.90%   70.89%   -0.01%     
+ Complexity                58243    58215      -28     
========================================================
  Files                      4734     4733       -1     
  Lines                    278272   278254      -18     
  Branches                  40403    40402       -1     
========================================================
- Hits                     197301   197271      -30     
- Misses                    64692    64708      +16     
+ Partials                  16279    16275       -4     
Impacted Files Coverage Δ
.../opensearch/extensions/ExtensionsOrchestrator.java 66.66% <ø> (-0.18%) ⬇️
...n/indices/forcemerge/ForceMergeRequestBuilder.java 0.00% <0.00%> (-75.00%) ⬇️
...pensearch/client/cluster/RemoteConnectionInfo.java 0.00% <0.00%> (-73.18%) ⬇️
...a/org/opensearch/client/cluster/ProxyModeInfo.java 0.00% <0.00%> (-60.00%) ⬇️
...a/org/opensearch/client/cluster/SniffModeInfo.java 0.00% <0.00%> (-58.83%) ⬇️
.../java/org/opensearch/node/NodeClosedException.java 50.00% <0.00%> (-50.00%) ⬇️
...rch/client/transport/NoNodeAvailableException.java 28.57% <0.00%> (-42.86%) ⬇️
.../opensearch/client/cluster/RemoteInfoResponse.java 61.53% <0.00%> (-38.47%) ⬇️
...ensearch/client/indices/DetailAnalyzeResponse.java 20.54% <0.00%> (-34.25%) ⬇️
...nsearch/index/shard/IndexShardClosedException.java 66.66% <0.00%> (-33.34%) ⬇️
... and 490 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants