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

Disallow search request with preference parameter when weighted routing enabled #5874

Merged

Conversation

anshu1106
Copy link
Contributor

@anshu1106 anshu1106 commented Jan 16, 2023

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

Description

Disallow search requests with preference parameter in case of strict weighted routing

Issues Resolved

#5873

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.

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

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

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      1 org.opensearch.snapshots.DedicatedClusterSnapshotRestoreIT.testIndexDeletionDuringSnapshotCreationInQueue


assertThrows(
PreferenceBasedSearchNotAllowedException.class,
() -> internalCluster().client(nodeMap.get("b").get(0)).prepareSearch().setSize(0).setPreference("_only_local").get()
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we add randomBetween(local, node_id, only_node) jus to cover all preference options

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed

@Bukhtawar Bukhtawar changed the title Disallow search request with preference parameter Disallow search request with preference parameter when weighted routing enabled Jan 16, 2023
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@codecov-commenter
Copy link

Codecov Report

Merging #5874 (fb5e329) into main (32dd9e2) will decrease coverage by 0.03%.
The diff coverage is 30.76%.

@@             Coverage Diff              @@
##               main    #5874      +/-   ##
============================================
- Coverage     70.88%   70.84%   -0.04%     
+ Complexity    58720    58714       -6     
============================================
  Files          4768     4769       +1     
  Lines        280575   280588      +13     
  Branches      40514    40515       +1     
============================================
- Hits         198881   198784      -97     
- Misses        65334    65534     +200     
+ Partials      16360    16270      -90     
Impacted Files Coverage Δ
...ting/PreferenceBasedSearchNotAllowedException.java 0.00% <0.00%> (ø)
...rg/opensearch/common/settings/ClusterSettings.java 91.89% <ø> (ø)
...g/opensearch/cluster/routing/OperationRouting.java 83.44% <42.85%> (-2.06%) ⬇️
.../main/java/org/opensearch/OpenSearchException.java 93.63% <100.00%> (+1.04%) ⬆️
...g/opensearch/index/analysis/CharFilterFactory.java 0.00% <0.00%> (-100.00%) ⬇️
...adonly/AddIndexBlockClusterStateUpdateRequest.java 0.00% <0.00%> (-75.00%) ⬇️
...n/admin/cluster/node/tasks/get/GetTaskRequest.java 30.30% <0.00%> (-63.64%) ⬇️
...readonly/TransportVerifyShardIndexBlockAction.java 9.75% <0.00%> (-58.54%) ⬇️
...n/admin/indices/readonly/AddIndexBlockRequest.java 17.85% <0.00%> (-53.58%) ⬇️
.../action/admin/indices/flush/ShardFlushRequest.java 50.00% <0.00%> (-50.00%) ⬇️
... and 447 more

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

@Bukhtawar Bukhtawar merged commit 60db7b5 into opensearch-project:main Jan 18, 2023
@Bukhtawar Bukhtawar added the backport 2.x Backport to 2.x branch label Jan 18, 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-5874-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 60db7b5dee6035754d9e4ee49b31f895f176282e
# Push it to GitHub
git push --set-upstream origin backport/backport-5874-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-5874-to-2.x.

nknize pushed a commit to nknize/OpenSearch that referenced this pull request Jan 19, 2023
…ng enabled (opensearch-project#5874)

* Disallow preference search with strict weighted shard routing

Signed-off-by: Anshu Agarwal <[email protected]>
anshu1106 added a commit to anshu1106/OpenSearch that referenced this pull request Jan 27, 2023
…ng enabled (opensearch-project#5874)

* Disallow preference search with strict weighted shard routing

Signed-off-by: Anshu Agarwal <[email protected]>
Bukhtawar pushed a commit that referenced this pull request Jan 27, 2023
…ence parameter when weighted routing enabled (#6042)

* Disallow search request with preference parameter when weighted routing enabled (#5874)

* Disallow preference search with strict weighted shard routing

Signed-off-by: Anshu Agarwal <[email protected]>
@kavilla
Copy link
Member

kavilla commented Feb 10, 2023

@Bukhtawar, @dblock, @nknize.

This is technically a breaking change correct? From OpenSearch Dashboards perspective, by default we pass the value from OpenSearch on the request with a session ID so that it restricts operations to execute all search requests on the same shards. This has the benefit of reusing shard caches across requests. This helps ensure the results are the same per a session based on their sharding strategy. By disallowing this and with our default setting not knowing when weighted routing is enabled then requests will fail.

But being that we are only one consumer of the _search API, it is still an important one so I'd imagine folks who do not use OpenSearch Dashboards but utilize OpenSearch and would like to enable weighted routing then they have to make sure they remove the preference param from their request even if they only take a minor version upgrade.

I understand if this is in main, but having it be backported to 2.x and 1.x seems to break SEMVER to me if the weighted routing where released on those major versions. If there is an improvement to be made about weighted routing could it respect both the weights and the preference because to ignore a requests preference seems not as user friendly.

Disregard if the weighted routing was not release in a previous version. If it did then 2.x and 1.x should still allow this param but 3.0 can accept this change to respect SEMVER.

cc: @joshuarrrr

@andrross
Copy link
Member

@kavilla Weighted routing was introduced in 2.4, though it appears it wasn't documented until 2.5. It does sound like this is a breaking change for 2.x. @Bukhtawar @anshu1106 What do you think?

@anshu1106
Copy link
Contributor Author

Makes sense. Created an issue #6297 to address this.

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.

5 participants