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

Adding support for ignore_unavailable in search request #13209

Closed
wants to merge 3 commits into from

Conversation

jainankitk
Copy link
Collaborator

@jainankitk jainankitk commented Apr 15, 2024

Description

This change adds support for passing ignore_unavailable_shards parameter in search request. Currently, the parameter is only used for snapshot requests. This will help resolve #12371 by using this parameter for delete_by_query and update_by_query requests

Related Issues

Resolves #12371

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Failing checks are inspected and point to the corresponding known issue(s) (See: Troubleshooting Failing Builds)
  • Commits are signed per the DCO using --signoff
  • Commit changes are listed out in CHANGELOG.md file (See: Changelog)
    - [ ] Public documentation issue/PR created

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.

Copy link
Contributor

❌ Gradle check result for b984227:

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?

Copy link
Contributor

❌ Gradle check result for 31f3979: FAILURE

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?

Copy link
Contributor

github-actions bot commented Apr 15, 2024

Compatibility status:

Checks if related components are compatible with change 31f3979

Incompatible components

Skipped components

Compatible components

Compatible components: [https://github.com/opensearch-project/custom-codecs.git, https://github.com/opensearch-project/asynchronous-search.git, https://github.com/opensearch-project/neural-search.git, https://github.com/opensearch-project/flow-framework.git, https://github.com/opensearch-project/job-scheduler.git, https://github.com/opensearch-project/cross-cluster-replication.git, https://github.com/opensearch-project/reporting.git, https://github.com/opensearch-project/security-analytics.git, https://github.com/opensearch-project/geospatial.git, https://github.com/opensearch-project/opensearch-oci-object-storage.git, https://github.com/opensearch-project/notifications.git, https://github.com/opensearch-project/observability.git, https://github.com/opensearch-project/common-utils.git, https://github.com/opensearch-project/k-nn.git, https://github.com/opensearch-project/anomaly-detection.git, https://github.com/opensearch-project/performance-analyzer-rca.git, https://github.com/opensearch-project/ml-commons.git, https://github.com/opensearch-project/index-management.git, https://github.com/opensearch-project/security.git, https://github.com/opensearch-project/sql.git, https://github.com/opensearch-project/alerting.git, https://github.com/opensearch-project/performance-analyzer.git]

@msfroh
Copy link
Collaborator

msfroh commented Apr 15, 2024

Would we specify this setting on all update_by_query and delete_by_query requests, or is the idea that users could specify it as part of their update_by_query and delete_by_query requests and the parameter would get propagated through to the search request?

I would go for the latter. IMO, the bug in #12371 is a bit of an edge case. Someone who wants to delete an index at the same time that they're doing a delete_by_query should be able to do so, but they need to explicitly ignore failures if they don't want an exception.

@jainankitk
Copy link
Collaborator Author

or is the idea that users could specify it as part of their update_by_query and delete_by_query requests and the parameter would get propagated through to the search request?

I am also thinking about this approach to not change default behavior, while at the same providing way for users to ignore such failures if they want to do so.

I would go for the latter. IMO, the bug in #12371 is a bit of an edge case. Someone who wants to delete an index at the same time that they're doing a delete_by_query should be able to do so, but they need to explicitly ignore failures if they don't want an exception.

+1

Comment on lines 103 to 105
private Boolean allowPartialSearchResults;

private Boolean ignoreUnavailableShards;
Copy link
Collaborator

Choose a reason for hiding this comment

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

What are all the possible interactions between allowPartialSearchResults and ignoreUnavailableShards?

Would another option be to differentiate between allowPartialSearchResults is null (which currently defaults to true) versus the case where it's explicitly passed as true?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What are all the possible interactions between allowPartialSearchResults and ignoreUnavailableShards?

Essentially, allowPartialSearchResults (ignore any type of shard failures) option supersedes ignoreUnavailableShards (only ignores unavailable shards). But given that the allowPartialSearchResults is not available with the _delete_by_query, _update_by_query and _reindex, it makes sense to expose more limited ignoreUnavailableShards instead of changing existing behavior of allowPartialSearchResults.

Would another option be to differentiate between allowPartialSearchResults is null (which currently defaults to true) versus the case where it's explicitly passed as true?

I am not sure about that option, since it changes the existing behavior and adds 3 cases for boolean value

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Interestingly, the ignore_unavailable parameter is already part of search API documentation - https://opensearch.org/docs/latest/api-reference/search/

Copy link
Contributor

❌ Gradle check result for a12dd35: FAILURE

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?

Copy link
Contributor

❌ Gradle check result for 3d9b1c3: FAILURE

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?

@jainankitk jainankitk changed the title Adding support for ignore_unavailable_shards in search request Adding support for ignore_unavailable in search request Apr 17, 2024
Copy link
Contributor

❌ Gradle check result for a58f13b: FAILURE

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?

@jainankitk
Copy link
Collaborator Author

@msfroh - Closing this in favor of #13298

@jainankitk jainankitk closed this Apr 19, 2024
@jainankitk jainankitk deleted the os-12371 branch April 19, 2024 06:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Indexing & Search Search Search query, autocomplete ...etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Concurrent index deletion during ongoing search request throws 5xx error
2 participants