[Weighted Routing] Allow Search with preference parameter in weighed in nodes #6296
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Anshu Agarwal <[email protected]>
Gradle Check (Jenkins) Run Completed with:
|
Signed-off-by: Anshu Agarwal <[email protected]>
Gradle Check (Jenkins) Run Completed with:
|
anshu1106
requested review from
reta,
anasalkouz,
andrross,
Bukhtawar,
CEHENKLE,
dblock,
gbbafna,
setiah,
kartg,
kotwanikunal,
mch2,
nknize,
owaiskazi19,
adnapibar,
Rishikesh1159,
ryanbogan,
saratvemulapalli,
shwetathareja,
dreamer-89,
tlfeng,
VachaShah and
xuezhou25
as code owners
|
Does this effectively revert #5874? Is there a top level issue somewhere the fully describes the behavior of weighted routing that can help me understand this? |
Bukhtawar
reviewed
Feb 13, 2023
| * round-robin scheduling policy. Uses the passed seed to shuffle the shards. | ||
| * | ||
| */ | ||
| public ShardIterator activeInitializingShardsSimpleWeightedIt( |
There was a problem hiding this comment.
Should this be package private
There was a problem hiding this comment.
tests won't compile
Bukhtawar
reviewed
Feb 13, 2023
No the previous change was strict, this change relaxes the checks to allow the search preference parameter if preference isn't hard(like ONLY_NODES) when those nodes are weighed away. |
Signed-off-by: Anshu Agarwal <[email protected]>
Gradle Check (Jenkins) Run Completed with:
|
Signed-off-by: Anshu Agarwal <[email protected]>
Gradle Check (Jenkins) Run Completed with:
|
added 3 commits
February 14, 2023 17:53
Signed-off-by: Anshu Agarwal <[email protected]>
Signed-off-by: Anshu Agarwal <[email protected]>
@Bukhtawar added tests for all the preference parameter |
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Codecov Report
📣 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 #6296 +/- ##
============================================
- Coverage 70.78% 70.59% -0.19%
+ Complexity 59062 58951 -111
============================================
Files 4800 4800
Lines 282420 282496 +76
Branches 40718 40735 +17
============================================
- Hits 199908 199442 -466
- Misses 66126 66658 +532
- Partials 16386 16396 +10
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Bukhtawar
reviewed
Feb 15, 2023
Comment on lines
376
to
409
| final Set<String> nodesIds = Arrays.stream(preference.substring(Preference.PREFER_NODES.type().length() + 1).split(",")) | ||
| .collect(Collectors.toSet()); | ||
| return indexShard.preferNodeActiveInitializingShardsWeightedIt( | ||
| nodesIds, | ||
| nodes, | ||
| weightedRoutingMetadata.getWeightedRouting(), | ||
| getWeightedRoutingDefaultWeight() | ||
| ); | ||
| case LOCAL: | ||
| return indexShard.preferNodeActiveInitializingShardsWeightedIt( | ||
| Collections.singleton(localNodeId), | ||
| nodes, | ||
| weightedRoutingMetadata.getWeightedRouting(), | ||
| getWeightedRoutingDefaultWeight() | ||
| ); | ||
| case ONLY_LOCAL: | ||
| if (!indexShard.isNodeWeighedAway( | ||
| weightedRoutingMetadata.getWeightedRouting(), | ||
| nodes, | ||
| localNodeId, | ||
| getWeightedRoutingDefaultWeight() | ||
| )) return indexShard.onlyNodeActiveInitializingShardsIt(localNodeId); | ||
| else { | ||
| throw new PreferenceBasedSearchNotAllowedException( | ||
| "Preference based routing not allowed on weigh away node with strict weighted shard routing enabled" | ||
| ); | ||
| } | ||
| case ONLY_NODES: | ||
| String nodeAttributes = preference.substring(Preference.ONLY_NODES.type().length() + 1); | ||
| return indexShard.onlyNodeSelectorActiveInitializingWeightedShardsIt( | ||
| nodeAttributes.split(","), | ||
| nodes, | ||
| weightedRoutingMetadata.getWeightedRouting(), | ||
| getWeightedRoutingDefaultWeight() |
There was a problem hiding this comment.
There could be multiple cases for each of these cases. Lets add comments on the behavioural change. We might also want to document the same
added 2 commits
February 15, 2023 19:13
Signed-off-by: Anshu Agarwal <[email protected]>
Signed-off-by: Anshu Agarwal <[email protected]>
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
andrross
reviewed
Feb 15, 2023
| .routingTable(routingTable) | ||
| .build(); | ||
|
|
||
| threadPool = new TestThreadPool("testThatOnlyNodesSupport"); |
There was a problem hiding this comment.
Copy/paste error on "testThatOnlyNodesSupport"? Also can probably move this assignment up and skip assigning null to threadPool.
| @@ -112,6 +112,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), | |||
| - Cluster health call to throw decommissioned exception for local decommissioned node([#6008](https://github.com/opensearch-project/OpenSearch/pull/6008)) | |||
| - [Refactor] core.common to new opensearch-common library ([#5976](https://github.com/opensearch-project/OpenSearch/pull/5976)) | |||
| - Cluster local health call to throw exception if node is decommissioned or weighed away ([#6198](https://github.com/opensearch-project/OpenSearch/pull/6198)) | |||
| - Allow search with preference parameter when strict weighted shard routing is enabled ([#6296](https://github.com/opensearch-project/OpenSearch/pull/6296)) | |||
There was a problem hiding this comment.
We now have the following two entries to go in the next release notes:
- Add support to disallow search request with preference parameter with strict weighted shard routing
- Allow search with preference parameter when strict weighted shard routing is enabled
I find these two statements confusing! Since the second change is just modifying a subtle behavior of the first change (which is still unreleased), maybe we don't need the second entry?
There was a problem hiding this comment.
Sure, will modify the first entry itself
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Signed-off-by: Anshu Agarwal [email protected]
Description
This PR adds support to allow searches with preference parameter when strict weighted routing is enabled.
Issues Resolved
#6297
Check List
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.