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

[Weighted Routing] Add support for discovered master and remove local weights in the response #5680

Merged

Conversation

anshu1106
Copy link
Contributor

@anshu1106 anshu1106 commented Jan 3, 2023

Signed-off-by: Anshu Agarwal [email protected]

Description

The local weights don't add much value since weights are relative and having just the local value gives no indication of how traffic would get distributed.
Discovered master is helpful to understand cases where after weights are set and also that the node is unable to connect to the leader either due to
Master not discovered -- This might happen to newly launched after decommission or even otherwise
Decommissioned -- The node was decommissioned

Issues Resolved

[List any issues this PR will resolve]

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.

Anshu Agarwal added 2 commits January 3, 2023 17:04
Signed-off-by: Anshu Agarwal <[email protected]>
Signed-off-by: Anshu Agarwal <[email protected]>
@github-actions
Copy link
Contributor

github-actions bot commented Jan 3, 2023

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

github-actions bot commented Jan 3, 2023

Gradle Check (Jenkins) Run Completed with:

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

github-actions bot commented Jan 4, 2023

Gradle Check (Jenkins) Run Completed with:

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

github-actions bot commented Jan 4, 2023

Gradle Check (Jenkins) Run Completed with:

@codecov-commenter
Copy link

codecov-commenter commented Jan 4, 2023

Codecov Report

Merging #5680 (4e15525) into main (2746517) will decrease coverage by 0.10%.
The diff coverage is 52.38%.

@@             Coverage Diff              @@
##               main    #5680      +/-   ##
============================================
- Coverage     71.11%   71.01%   -0.11%     
+ Complexity    58641    58600      -41     
============================================
  Files          4760     4760              
  Lines        279515   279516       +1     
  Branches      40348    40347       -1     
============================================
- Hits         198782   198491     -291     
- Misses        64463    64876     +413     
+ Partials      16270    16149     -121     
Impacted Files Coverage Δ
...eighted/get/TransportGetWeightedRoutingAction.java 73.68% <50.00%> (-2.32%) ⬇️
...eighted/get/ClusterGetWeightedRoutingResponse.java 62.26% <52.63%> (-2.96%) ⬇️
...adonly/AddIndexBlockClusterStateUpdateRequest.java 0.00% <0.00%> (-75.00%) ⬇️
...readonly/TransportVerifyShardIndexBlockAction.java 9.75% <0.00%> (-73.18%) ⬇️
...n/admin/indices/readonly/AddIndexBlockRequest.java 17.85% <0.00%> (-53.58%) ⬇️
.../java/org/opensearch/node/NodeClosedException.java 50.00% <0.00%> (-50.00%) ⬇️
...ava/org/opensearch/action/NoSuchNodeException.java 0.00% <0.00%> (-50.00%) ⬇️
...a/org/opensearch/tasks/TaskCancelledException.java 50.00% <0.00%> (-50.00%) ⬇️
...opensearch/persistent/PersistentTasksExecutor.java 22.22% <0.00%> (-44.45%) ⬇️
...adcast/BroadcastShardOperationFailedException.java 55.55% <0.00%> (-44.45%) ⬇️
... and 474 more

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

@anshu1106 anshu1106 changed the title [Weighted Routing] Deprecate GET local node weight [Weighted Routing] Add support for discovered master and remove local weights Jan 4, 2023
Signed-off-by: Anshu Agarwal <[email protected]>
@gbbafna
Copy link
Collaborator

gbbafna commented Jan 4, 2023

I think we should let this functionality be as is . A local collector could use this to find out weights in a light weight manner , which otherwise would be heavy to go to the cluster manager.

@Bukhtawar
Copy link
Collaborator

I think we should let this functionality be as is . A local collector could use this to find out weights in a light weight manner , which otherwise would be heavy to go to the cluster manager.

@anshu1106 can correct me but this doesn't remove _local query but removes local_weight since it doesn't add much value as weights are relative. The caller is responsible to select specific weights based on the awareness attribute

@gbbafna
Copy link
Collaborator

gbbafna commented Jan 4, 2023

I think we should let this functionality be as is . A local collector could use this to find out weights in a light weight manner , which otherwise would be heavy to go to the cluster manager.

@anshu1106 can correct me but this doesn't remove _local query but removes local_weight since it doesn't add much value as weights are relative. The caller is responsible to select specific weights based on the awareness attribute

Got it. My bad, i misread the title .

@@ -219,7 +219,6 @@ public void testGetWeightedRouting_WeightsAreSet() throws IOException {
.setRequestLocal(true)
.get();
assertEquals(weightedRouting, weightedRoutingResponse.weights());
assertEquals("1.0", weightedRoutingResponse.getLocalNodeWeight());
Copy link
Collaborator

Choose a reason for hiding this comment

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

add discovered_cluster_manager assertion as well .


public String getLocalNodeWeight() {
return localNodeWeight;
public Boolean getDiscoveredMaster() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

lets use cluster_manager everywhere . master is going to get deprecated in 3.0.

Anshu Agarwal added 2 commits January 4, 2023 20:17
Signed-off-by: Anshu Agarwal <[email protected]>
Signed-off-by: Anshu Agarwal <[email protected]>
@github-actions
Copy link
Contributor

github-actions bot commented Jan 4, 2023

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

github-actions bot commented Jan 4, 2023

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

github-actions bot commented Jan 5, 2023

Gradle Check (Jenkins) Run Completed with:

@anshu1106 anshu1106 force-pushed the feature/wrr-deprecate-get-local branch from 0dcfbbd to dff4cce Compare January 5, 2023 13:00
@github-actions
Copy link
Contributor

github-actions bot commented Jan 5, 2023

Gradle Check (Jenkins) Run Completed with:

@Bukhtawar Bukhtawar changed the title [Weighted Routing] Add support for discovered master and remove local weights [Weighted Routing] Add support for discovered master and remove local weights in the response Jan 6, 2023
Comment on lines 48 to 51
ClusterGetWeightedRoutingResponse() {
this.weightedRouting = null;
this.discoveredClusterManager = null;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is no explicit need to set them null

@Bukhtawar Bukhtawar merged commit 3eaf129 into opensearch-project:main Jan 7, 2023
@Bukhtawar Bukhtawar added the backport 2.x Backport to 2.x branch label Jan 9, 2023
@opensearch-trigger-bot
Copy link
Contributor

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-5680-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 3eaf129b969d7c5dd9b502c747034bd60d8a4591
# Push it to GitHub
git push --set-upstream origin backport/backport-5680-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-5680-to-2.x.

anshu1106 added a commit to anshu1106/OpenSearch that referenced this pull request Jan 10, 2023
… weights in the response (opensearch-project#5680)

* Add support for discovered master and remove local weights in the weighted routing API response

Signed-off-by: Anshu Agarwal <[email protected]>
anshu1106 added a commit to anshu1106/OpenSearch that referenced this pull request Jan 10, 2023
… weights in the response (opensearch-project#5680)

* Add support for discovered master and remove local weights in the weighted routing API response

Signed-off-by: Anshu Agarwal <[email protected]>
anshu1106 added a commit to anshu1106/OpenSearch that referenced this pull request Jan 10, 2023
… weights in the response (opensearch-project#5680)

* Add support for discovered master and remove local weights in the weighted routing API response

Signed-off-by: Anshu Agarwal <[email protected]>
gbbafna pushed a commit that referenced this pull request Jan 10, 2023
#5784)

* [Weighted Routing] Add support for discovered master and remove local weights in the response (#5680)

* Add support for discovered master and remove local weights in the weighted routing API response

Signed-off-by: Anshu Agarwal <[email protected]>

* [Weighted Shard Routing] API versioning (#5255)

* Support API versioning for weighted shard routing

Signed-off-by: Anshu Agarwal <[email protected]>

* [Weighted Shard Routing] Fail open requests on search shard failures (#5072)

* Fail open requests on search shard failures (

Signed-off-by: Anshu Agarwal <[email protected]>

* Address fail open comments (#5778)

[Weighted Shard Routing] Refactor and fix singleton in FailAwareWeightedRouting

Signed-off-by: Anshu Agarwal <[email protected]>

* remove unintended changes in changelog

Signed-off-by: Anshu Agarwal <[email protected]>

* remove unintended changes from changelog

Signed-off-by: Anshu Agarwal <[email protected]>

Signed-off-by: Anshu Agarwal <[email protected]>
Co-authored-by: Anshu Agarwal <[email protected]>
Bukhtawar pushed a commit that referenced this pull request Jan 10, 2023
…ed shard routing (#5781)

* [Backport 2.x] 
[Weighted Shard Routing] Add support for discovered master and remove local weights in the response #5680
[Weighted Shard Routing] API versioning #5255
[Weighted Shard Routing] Fail open requests on search shard failures #5072
[Weighted Shard Routing] Refactor and fix singleton in FailAwareWeightedRouting #5778

Signed-off-by: Anshu Agarwal <[email protected]>
kotwanikunal pushed a commit that referenced this pull request Jan 25, 2023
…ed shard routing (#5781)

* [Backport 2.x] 
[Weighted Shard Routing] Add support for discovered master and remove local weights in the response #5680
[Weighted Shard Routing] API versioning #5255
[Weighted Shard Routing] Fail open requests on search shard failures #5072
[Weighted Shard Routing] Refactor and fix singleton in FailAwareWeightedRouting #5778

Signed-off-by: Anshu Agarwal <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x Backport to 2.x branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants