Fix weighted shard routing state across search requests #6004

anshu1106 · 2023-01-25T05:30:23Z

Signed-off-by: Anshu Agarwal anshukag@amazon.com

Description

This PR fixes issue due to which state is not maintained across weighted shard routing search requests . The shuffler is moved twice in a call which is causing the issue. The PR adds code logic to prevent unwanted shuffler movement.

Issues Resolved

#6056

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: Anshu Agarwal <anshukag@amazon.com>

github-actions · 2023-01-25T05:52:53Z

Gradle Check (Jenkins) Run Completed with:

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

github-actions · 2023-01-25T07:10:12Z

Gradle Check (Jenkins) Run Completed with:

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

Bukhtawar · 2023-01-25T07:38:41Z

server/src/main/java/org/opensearch/cluster/routing/IndexShardRoutingTable.java

    final ShardShuffler shuffler;
+    final ShardShuffler shufflerForWeightedRouting;


Do we need a second shuffler or just creating another method the prevents the shuffle for onlyNodeSelectorActiveInitializingShardsIt should work?

The issue is with this line https://github.com/opensearch-project/OpenSearch/blob/main/server/src/main/java/org/opensearch/cluster/routing/IndexShardRoutingTable.java#L587. This is moving the shuffler one more time ie in a three node cluster with weights (1,1,0) the second request also hits the same node since the shuffler moves by 2 in the first request.

Since we need onlyNodeSelectorActiveInitializingShardsIt to get shards belonging to node with weight zero. Introducing a method without shuffler will require a code duplication.

Lets put up a comment explaining the same.

github-actions · 2023-01-25T09:06:48Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/9881/
CommitID: a5b2125

codecov-commenter · 2023-01-25T09:08:05Z

Codecov Report

Merging #6004 (296a62e) into main (715ff72) will increase coverage by 0.05%.
The diff coverage is 100.00%.

📣 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    #6004      +/-   ##
============================================
+ Coverage     70.75%   70.81%   +0.05%     
- Complexity    58720    58738      +18     
============================================
  Files          4771     4771              
  Lines        280818   280819       +1     
  Branches      40568    40568              
============================================
+ Hits         198704   198860     +156     
+ Misses        65824    65630     -194     
- Partials      16290    16329      +39

Impacted Files	Coverage Δ
...search/cluster/routing/IndexShardRoutingTable.java	`86.43% <100.00%> (-1.48%)`	⬇️
...java/org/opensearch/client/indices/DataStream.java	`0.00% <0.00%> (-76.09%)`	⬇️
.../opensearch/client/indices/CloseIndexResponse.java	`17.50% <0.00%> (-73.75%)`	⬇️
...luster/routing/allocation/RoutingExplanations.java	`41.37% <0.00%> (-58.63%)`	⬇️
...ava/org/opensearch/action/NoSuchNodeException.java	`0.00% <0.00%> (-50.00%)`	⬇️
...ch/transport/ReceiveTimeoutTransportException.java	`50.00% <0.00%> (-50.00%)`	⬇️
.../admin/cluster/reroute/ClusterRerouteResponse.java	`55.00% <0.00%> (-45.00%)`	⬇️
.../org/opensearch/client/indices/AnalyzeRequest.java	`31.00% <0.00%> (-42.00%)`	⬇️
...java/org/opensearch/threadpool/ThreadPoolInfo.java	`56.25% <0.00%> (-37.50%)`	⬇️
...ndex/seqno/RetentionLeaseBackgroundSyncAction.java	`37.50% <0.00%> (-37.50%)`	⬇️
... 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.

Signed-off-by: Anshu Agarwal <anshukag@amazon.com>

github-actions · 2023-01-25T09:55:21Z

Gradle Check (Jenkins) Run Completed with:

RESULT: UNSTABLE ❕
TEST FAILURES:

      1 org.opensearch.indices.replication.SegmentReplicationIT.testDeleteOperations
      1 org.opensearch.cluster.routing.allocation.decider.DiskThresholdDeciderIT.testIndexCreateBlockWithAReadOnlyBlock

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

andrross

This PR fixes issue due to which state is not maintained across weighted shard routing search requests .

Can you provide more detail about what is changing here (or link an issue that explains in detail what the problem is)? "state is not maintained" is the cause, but what is the actual issue? i.e. what is the behavior change that a user will as a result of this fix?

andrross · 2023-01-25T18:48:32Z

CHANGELOG.md

@@ -69,6 +69,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - Fix 'org.apache.hc.core5.http.ParseException: Invalid protocol version' under JDK 16+ ([#4827](https://github.com/opensearch-project/OpenSearch/pull/4827))
 - Fixed compression support for h2c protocol ([#4944](https://github.com/opensearch-project/OpenSearch/pull/4944))
 - Support OpenSSL Provider with default Netty allocator ([#5460](https://github.com/opensearch-project/OpenSearch/pull/5460))
+- Fix weighted shard routing state across search requests([#6004](https://github.com/opensearch-project/OpenSearch/pull/6004))


Do you intend to backport this to 2.x? If so, you'll need to put this in the [Unreleased 2.x] section.

yes, moved the entry to 2.x section. Thanks for pointing this out.

This PR fixes issue due to which state is not maintained across weighted shard routing search requests .

Can you provide more detail about what is changing here (or link an issue that explains in detail what the problem is)? "state is not maintained" is the cause, but what is the actual issue? i.e. what is the behavior change that a user will as a result of this fix?

Created an issue with the details #6056

andrross

CHANGELOG entry should move to 2.x section

Signed-off-by: Anshu Agarwal <anshukag@amazon.com>

github-actions · 2023-01-28T08:20:53Z

Gradle Check (Jenkins) Run Completed with:

RESULT: UNSTABLE ❕
TEST FAILURES:

      1 org.opensearch.indices.replication.SegmentReplicationRelocationIT.testIndexReopenClose
      1 org.opensearch.indices.replication.SegmentReplicationRelocationIT.testDeleteOperations
      1 org.opensearch.cluster.routing.allocation.decider.DiskThresholdDeciderIT.testIndexCreateBlockWithAReadOnlyBlock
      1 org.opensearch.cluster.routing.allocation.decider.DiskThresholdDeciderIT.testIndexCreateBlockIsRemovedWhenAnyNodesNotExceedHighWatermark

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

gbbafna · 2023-01-28T11:32:45Z

server/src/main/java/org/opensearch/cluster/routing/IndexShardRoutingTable.java

    final ShardShuffler shuffler;
+    final ShardShuffler shufflerForWeightedRouting;


Lets put up a comment explaining the same.

gbbafna · 2023-01-28T11:33:52Z

server/src/test/java/org/opensearch/cluster/structure/RoutingIteratorTests.java

+     * Test to validate that shard routing state is maintained across requests, requests are assigned to nodes
+     * according to assigned routing weights
+     */
+    public void testWeightedRoutingShardStateWithDifferentWeights() {


Wondering how did we miss this regression since we had ITs covering same . If not , lets please add

Yeah we have added a test now

Signed-off-by: Anshu Agarwal <anshukag@amazon.com>

github-actions · 2023-01-29T15:32:48Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/10126/
CommitID: 296a62e

Comments addressed

* Fix maintaining state across search requests with weighted shard routing Signed-off-by: Anshu Agarwal <anshukag@amazon.com> (cherry picked from commit cd860ec) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

* Fix maintaining state across search requests with weighted shard routing (cherry picked from commit cd860ec) Signed-off-by: Anshu Agarwal <anshukag@amazon.com> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Andrew Ross <andrross@amazon.com>

Fix state across search requests using weighted shard routing

2007e8b

Signed-off-by: Anshu Agarwal <anshukag@amazon.com>

anshu1106 marked this pull request as ready for review January 25, 2023 05:30

Merge branch 'main' into wrr-state-across-requests

b1ca890

Bukhtawar reviewed Jan 25, 2023

View reviewed changes

Bukhtawar approved these changes Jan 25, 2023

View reviewed changes

Add changelog

69d20ba

Signed-off-by: Anshu Agarwal <anshukag@amazon.com>

anshu1106 force-pushed the wrr-state-across-requests branch from a5b2125 to 69d20ba Compare January 25, 2023 09:25

andrross reviewed Jan 25, 2023

View reviewed changes

andrross previously requested changes Jan 25, 2023

View reviewed changes

Move changelog entry to 2.x

cd0e5f8

Signed-off-by: Anshu Agarwal <anshukag@amazon.com>

anshu1106 requested a review from andrross January 28, 2023 08:10

gbbafna reviewed Jan 28, 2023

View reviewed changes

Address review comments

296a62e

Signed-off-by: Anshu Agarwal <anshukag@amazon.com>

Bukhtawar approved these changes Jan 29, 2023

View reviewed changes

Bukhtawar merged commit cd860ec into opensearch-project:main Jan 29, 2023

Bukhtawar added the backport 2.x Backport to 2.x branch label Jan 29, 2023

opensearch-trigger-bot bot mentioned this pull request Jan 29, 2023

[Backport 2.x] Fix weighted shard routing state across search requests #6060

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix weighted shard routing state across search requests #6004

Fix weighted shard routing state across search requests #6004

anshu1106 commented Jan 25, 2023 •

edited

Loading

github-actions bot commented Jan 25, 2023

github-actions bot commented Jan 25, 2023

Bukhtawar Jan 25, 2023

anshu1106 Jan 25, 2023

gbbafna Jan 28, 2023

github-actions bot commented Jan 25, 2023

codecov-commenter commented Jan 25, 2023 •

edited

Loading

github-actions bot commented Jan 25, 2023

andrross left a comment

andrross Jan 25, 2023

anshu1106 Jan 28, 2023

anshu1106 Jan 28, 2023

andrross left a comment

github-actions bot commented Jan 28, 2023

gbbafna Jan 28, 2023

gbbafna Jan 28, 2023

anshu1106 Jan 29, 2023

github-actions bot commented Jan 29, 2023

		final ShardShuffler shuffler;
		final ShardShuffler shufflerForWeightedRouting;

Fix weighted shard routing state across search requests #6004

Fix weighted shard routing state across search requests #6004

Conversation

anshu1106 commented Jan 25, 2023 • edited Loading

Description

Issues Resolved

Check List

github-actions bot commented Jan 25, 2023

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Jan 25, 2023

Gradle Check (Jenkins) Run Completed with:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 25, 2023

Gradle Check (Jenkins) Run Completed with:

codecov-commenter commented Jan 25, 2023 • edited Loading

Codecov Report

github-actions bot commented Jan 25, 2023

Gradle Check (Jenkins) Run Completed with:

andrross left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrross left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 28, 2023

Gradle Check (Jenkins) Run Completed with:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 29, 2023

Gradle Check (Jenkins) Run Completed with:

anshu1106 commented Jan 25, 2023 •

edited

Loading

codecov-commenter commented Jan 25, 2023 •

edited

Loading