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

Part 1: Support for cancel_after_timeinterval parameter in search and msearch request #986

Merged
merged 7 commits into from
Aug 12, 2021

Conversation

sohami
Copy link
Collaborator

@sohami sohami commented Jul 19, 2021

This PR introduces the new request level parameter to configure the timeout interval after which
a search request will be cancelled. For msearch request the parameter is supported both at parent
request and at sub child search requests. If it is provided at parent level and child search request
doesn't have it then the parent level value is set at such child request. The parent level msearch
is not used to cancel the parent request as it may be tricky to come up with correct value in cases
when child search request can have different runtimes

TEST: Added test for ser/de with new parameter

Signed-off-by: Sorabh Hamirwasia [email protected]

Description

Part-1: This commit introduces the new request level parameter (cancel_after_time_interval) to configure the timeout interval after which
a search request will be cancelled. For msearch request the parameter is supported both at parent
request and at sub child search requests. If it is provided at parent level and child search request
doesn't have it then the parent level value is set at such child request. The parent level msearch
is not used to cancel the parent request as it may be tricky to come up with correct value in cases
when child search request can have different runtimes

Part 2: Support for consuming cancel_after_time_interval parameter in search and msearch request. This commit adds the handling of the new request level parameter and schedule cancellation task. It also adds a cluster setting search.cancel_after_time_interval: To set a global cancellation timeout for search request which will be used in absence of request level timeout.

Below is the behavior in various cases:

request param cluster param Effective Feature
null 10s 10s
null -1 -1 disabled
-1 10s -1 disabled
-1 -1 -1 disabled
20s 10s 20s
20s -1 20s

Issues Resolved

#817

Check List

  • [Y] New functionality includes testing.
    • [Y] All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • [Y] Commits are signed per the DCO using --signoff

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.

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Wrapper Validation success 0f3785fbd3856a72834e53493b50f463603f0d4d

@opensearch-ci-bot
Copy link
Collaborator

✅   DCO Check Passed 0f3785fbd3856a72834e53493b50f463603f0d4d

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Precommit success 0f3785fbd3856a72834e53493b50f463603f0d4d

Copy link
Contributor

@AmiStrn AmiStrn left a comment

Choose a reason for hiding this comment

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

Nice!

I am commenting as I am not a maintainer. please flag one of them in a comment to take a look as well:)

My only real issue is the name of the param - I think that cancel_after_time_interval would be better. the main indicator for this is that you wrote it in camel case like this: cancelAfterTimeInterval... separating between the time and the interval makes sense.
Other things I commented on are minor, to say the least.

Thanks for calling my attention to the PR I would be happy to review the next phase as well when you get to it!

@dblock
Copy link
Member

dblock commented Jul 27, 2021

@AmiStrn thanks for reviewing this! Looking forward to the next update @sohami

@opensearch-ci-bot
Copy link
Collaborator

✅   DCO Check Passed 433f91f

@sohami
Copy link
Collaborator Author

sohami commented Jul 28, 2021

@AmiStrn - Thanks for the review. I have rebased and added commit for part 2 as well. Have also replied to your comment related to naming. Please take a look.
@dblock - Updated with part 2 changes

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Wrapper Validation success 433f91f

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Precommit success 433f91f

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

The change looks good to me, but we should spend some time talking about the name of this setting. I see two problems with the current name.

  1. It does not tell me whether it's seconds, milliseconds or something else, while we do have absoluteStartMillis that is in milliseconds. So maybe the name should include millis, ms or whatever other convention is used for another setting?
  2. You call this "cancellation timeout" in a bunch of places in tests. This seems like what this feature does, time out after a certain time. This feels like possibly a much more sensible name (RequestTimeout or RequestTimeoutMillis?).

Copy link
Contributor

@AmiStrn AmiStrn left a comment

Choose a reason for hiding this comment

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

Cool!
I liked the listener wrapper solution in particular.
@dblock - regarding the use of the OriginSettingClient in TimeoutTaskCancellationUtility - will this be a problem with the changes going to be introduced in the security module?
Regarding the params - I suggest using just the one for the time interval itself without the boolean.
Regarding the cluster settings, there is concern over the surface area of the settings in general in the project getting too big. No good solution for that, however, these settings may not be a "must" for this feature at this point. Consider using only the time interval setting anyway, since adding another that basically says "use the other setting" may be confusing.
All in all I enjoyed reviewing this, Thanks @sohami :)

@sohami
Copy link
Collaborator Author

sohami commented Aug 2, 2021

The change looks good to me, but we should spend some time talking about the name of this setting. I see two problems with the current name.

  1. It does not tell me whether it's seconds, milliseconds or something else, while we do have absoluteStartMillis that is in milliseconds. So maybe the name should include millis, ms or whatever other convention is used for another setting?
  2. You call this "cancellation timeout" in a bunch of places in tests. This seems like what this feature does, time out after a certain time. This feels like possibly a much more sensible name (RequestTimeout or RequestTimeoutMillis?).
  1. The parameter can accept the time in any unit since it is of TimeValue type. Input can be 10s or 10ms or any other format. Similar parameters are defined here: search.keep_alive_interval or default_search_timeout
  2. The feature basically cancels the search request after a defined time interval to avoid any resource consumption by search request. There is already a timeout parameter in search request which is honored at shard level instead at search request level. To be more explicit and avoid any confusion, I am naming the parameter as cancel_after_time_interval. I am fine with @AmiStrn suggestion on splitting it to time_interval

@opensearch-ci-bot
Copy link
Collaborator

❌   DCO Check Failed 6f5208042b6ee2441904b469578f761d6dee4568
Run ./dev-tools/signoff-check.sh remotes/origin/main 6f5208042b6ee2441904b469578f761d6dee4568 to check locally
Use git commit with -s to add 'Signed-of-by: {EMAIL}' on impacted commits

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Wrapper Validation success 6f5208042b6ee2441904b469578f761d6dee4568

@sohami
Copy link
Collaborator Author

sohami commented Aug 2, 2021

Cool!
I liked the listener wrapper solution in particular.
@dblock - regarding the use of the OriginSettingClient in TimeoutTaskCancellationUtility - will this be a problem with the changes going to be introduced in the security module?
Regarding the params - I suggest using just the one for the time interval itself without the boolean.
Regarding the cluster settings, there is concern over the surface area of the settings in general in the project getting too big. No good solution for that, however, these settings may not be a "must" for this feature at this point. Consider using only the time interval setting anyway, since adding another that basically says "use the other setting" may be confusing.
All in all I enjoyed reviewing this, Thanks @sohami :)

@AmiStrn and @dblock : Thanks for the review and feedback. Have responded to majority of it and updated the PR with changes related to some of the items. For now have taken @AmiStrn feedback to name it as cancel_after_time_interval. Will wait for your next round of input :)

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Wrapper Validation success c54f40fd5b3a2d187960fe2c8985df3cf1ced35f

@opensearch-ci-bot
Copy link
Collaborator

✅   DCO Check Passed c54f40fd5b3a2d187960fe2c8985df3cf1ced35f

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Precommit success 6f5208042b6ee2441904b469578f761d6dee4568

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Precommit success c54f40fd5b3a2d187960fe2c8985df3cf1ced35f

@opensearch-ci-bot
Copy link
Collaborator

✅   DCO Check Passed 9d48da524dced80a8b4b3844c0687426fefc2b61

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Wrapper Validation success 9d48da524dced80a8b4b3844c0687426fefc2b61

@sohami
Copy link
Collaborator Author

sohami commented Aug 3, 2021

@dblock and @AmiStrn : Thanks for your active review! To summarize, most of the comments are taken care of and following are still open:

  1. Cluster setting to control enabling/disabling this feature. Being discussed here. My preference would be to keep it in v1 and slowly deprecate over next couple of releases. I can own that issue.
  2. Concern over using OriginSettingClient in context of security model changes discussed here. I don't see issue with this usage though, given the same mechanism is used in other places (Example here). If it needs some changes then it can be done as separate PR across all the usages.

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Precommit success 9d48da524dced80a8b4b3844c0687426fefc2b61

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

This LGTM!

@dblock
Copy link
Member

dblock commented Aug 4, 2021

start gradle check

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Check success 9d48da524dced80a8b4b3844c0687426fefc2b61
Log 368

Reports 368

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Wrapper Validation success eb39a6ab8b76e92d2ed30abb38ac401e7a9bba28

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Precommit success cf11529

@dblock
Copy link
Member

dblock commented Aug 11, 2021

start gradle check

@dblock
Copy link
Member

dblock commented Aug 11, 2021

@malpani @AmiStrn can I please have a final pair of 👀 from you on this?

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Check success cf11529
Log 385

Reports 385

@malpani
Copy link
Contributor

malpani commented Aug 12, 2021

@malpani @AmiStrn can I please have a final pair of 👀 from you on this?

lgtm! this is good to go!

@AmiStrn
Copy link
Contributor

AmiStrn commented Aug 12, 2021

@malpani @AmiStrn can I please have a final pair of 👀 from you on this?

Done, LGTM as well. This was interesting and I learned a lot from this approach @sohami
Also, I like the refactoring of the tests in SearchCancellationIT for readability:)

Can't wait to use this feature!
@dblock @malpani is the next step for this feature to add support for this in the different clients?

@dblock dblock merged commit 9b6e621 into opensearch-project:main Aug 12, 2021
@dblock
Copy link
Member

dblock commented Aug 12, 2021

I merged this. I imagine we want this in 1.1. @sohami can you please make a backport PR into the 1.x branch? (cherry pick the squashed commit and make a PR)

@dblock dblock added the pending backport Identifies an issue or PR that still needs to be backported label Aug 12, 2021
@dblock
Copy link
Member

dblock commented Aug 12, 2021

+++ this needs to be documented, open an issue or make PRs into https://github.com/opensearch-project/documentation-website please @sohami

@dblock dblock added the documentation pending Tracks issues which have PRs merged but documentation changes pending label Aug 12, 2021
sohami added a commit to sohami/OpenSearch that referenced this pull request Aug 12, 2021
… msearch request (opensearch-project#986)

* Part 1: Support for cancel_after_timeinterval parameter in search and msearch request

This commit introduces the new request level parameter to configure the timeout interval after which
a search request will be cancelled. For msearch request the parameter is supported both at parent
request and at sub child search requests. If it is provided at parent level and child search request
doesn't have it then the parent level value is set at such child request. The parent level msearch
is not used to cancel the parent request as it may be tricky to come up with correct value in cases
when child search request can have different runtimes

TEST: Added test for ser/de with new parameter

Signed-off-by: Sorabh Hamirwasia <[email protected]>

* Part 2: Support for cancel_after_timeinterval parameter in search and msearch request

This commit adds the handling of the new request level parameter and schedule cancellation task. It
also adds a cluster setting to set a global cancellation timeout for search request which will be
used in absence of request level timeout.

TEST: Added new tests in SearchCancellationIT
Signed-off-by: Sorabh Hamirwasia <[email protected]>

* Address Review feedback for Part 1

Signed-off-by: Sorabh Hamirwasia <[email protected]>

* Address review feedback for Part 2

Signed-off-by: Sorabh Hamirwasia <[email protected]>

* Update CancellableTask to remove the cancelOnTimeout boolean flag

Signed-off-by: Sorabh Hamirwasia <[email protected]>

* Replace search.cancellation.timeout cluster setting with search.enforce_server.timeout.cancellation to control if cluster level cancel_after_time_interval should take precedence over request level cancel_after_time_interval value

Signed-off-by: Sorabh Hamirwasia <[email protected]>

* Removing the search.enforce_server.timeout.cancellation cluster setting and just keeping search.cancel_after_time_interval setting with request level parameter taking the precedence.

Signed-off-by: Sorabh Hamirwasia <[email protected]>

Co-authored-by: Sorabh Hamirwasia <[email protected]>
@sohami
Copy link
Collaborator Author

sohami commented Aug 12, 2021

I merged this. I imagine we want this in 1.1. @sohami can you please make a backport PR into the 1.x branch? (cherry pick the squashed commit and make a PR)

Created a PR on 1.1: #1085

@sohami
Copy link
Collaborator Author

sohami commented Aug 12, 2021

+++ this needs to be documented, open an issue or make PRs into https://github.com/opensearch-project/documentation-website please @sohami

Corresponding documentation issue: opensearch-project/documentation-website#128

dblock pushed a commit that referenced this pull request Aug 12, 2021
… msearch request (#986) (#1085)

* Part 1: Support for cancel_after_timeinterval parameter in search and msearch request

This commit introduces the new request level parameter to configure the timeout interval after which
a search request will be cancelled. For msearch request the parameter is supported both at parent
request and at sub child search requests. If it is provided at parent level and child search request
doesn't have it then the parent level value is set at such child request. The parent level msearch
is not used to cancel the parent request as it may be tricky to come up with correct value in cases
when child search request can have different runtimes

TEST: Added test for ser/de with new parameter

Signed-off-by: Sorabh Hamirwasia <[email protected]>

* Part 2: Support for cancel_after_timeinterval parameter in search and msearch request

This commit adds the handling of the new request level parameter and schedule cancellation task. It
also adds a cluster setting to set a global cancellation timeout for search request which will be
used in absence of request level timeout.

TEST: Added new tests in SearchCancellationIT
Signed-off-by: Sorabh Hamirwasia <[email protected]>

* Address Review feedback for Part 1

Signed-off-by: Sorabh Hamirwasia <[email protected]>

* Address review feedback for Part 2

Signed-off-by: Sorabh Hamirwasia <[email protected]>

* Update CancellableTask to remove the cancelOnTimeout boolean flag

Signed-off-by: Sorabh Hamirwasia <[email protected]>

* Replace search.cancellation.timeout cluster setting with search.enforce_server.timeout.cancellation to control if cluster level cancel_after_time_interval should take precedence over request level cancel_after_time_interval value

Signed-off-by: Sorabh Hamirwasia <[email protected]>

* Removing the search.enforce_server.timeout.cancellation cluster setting and just keeping search.cancel_after_time_interval setting with request level parameter taking the precedence.

Signed-off-by: Sorabh Hamirwasia <[email protected]>

Co-authored-by: Sorabh Hamirwasia <[email protected]>

Co-authored-by: Sorabh Hamirwasia <[email protected]>
@dblock dblock removed the pending backport Identifies an issue or PR that still needs to be backported label Dec 6, 2021
Bukhtawar added a commit that referenced this pull request Jul 23, 2023
I have nominated and maintainers have agreed to invite Sorabh Hamirwasia(@sohami) to be a co-maintainer. Sorabh has kindly accepted.

Sorabh has led the design and implementation of multiple features like query cancellation, concurrent segment search for aggregations and snapshot inter-operability w/remote storage in OpenSearch. Some significant issues and PRs authored by Sorabh for this effort are as follows:

Feature proposals
Concurrent segment search for aggregations : #6798
Searchable Remote Index : #2900

Implementations
Concurrent segment search for aggregations: #7514
Lucene changes to leaf slices for concurrent search: apache/lucene#12374
Moving concurrent search to core : #7203
Query cancellation support : #986
In total, Sorabh has authored 14 PRs going back to Aug 2021. He also frequently reviews contributions from others and has reviewed nearly 20 PRs in the same time frame. I believe Sorabh will be a valuable addition as a maintainer of OpenSearch and will continue to contribute to the success of the project going forward.

Signed-off-by: Bukhtawar Khan <[email protected]>
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jul 23, 2023
I have nominated and maintainers have agreed to invite Sorabh Hamirwasia(@sohami) to be a co-maintainer. Sorabh has kindly accepted.

Sorabh has led the design and implementation of multiple features like query cancellation, concurrent segment search for aggregations and snapshot inter-operability w/remote storage in OpenSearch. Some significant issues and PRs authored by Sorabh for this effort are as follows:

Feature proposals
Concurrent segment search for aggregations : #6798
Searchable Remote Index : #2900

Implementations
Concurrent segment search for aggregations: #7514
Lucene changes to leaf slices for concurrent search: apache/lucene#12374
Moving concurrent search to core : #7203
Query cancellation support : #986
In total, Sorabh has authored 14 PRs going back to Aug 2021. He also frequently reviews contributions from others and has reviewed nearly 20 PRs in the same time frame. I believe Sorabh will be a valuable addition as a maintainer of OpenSearch and will continue to contribute to the success of the project going forward.

Signed-off-by: Bukhtawar Khan <[email protected]>
(cherry picked from commit 7769682)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Bukhtawar pushed a commit that referenced this pull request Jul 24, 2023
I have nominated and maintainers have agreed to invite Sorabh Hamirwasia(@sohami) to be a co-maintainer. Sorabh has kindly accepted.

Sorabh has led the design and implementation of multiple features like query cancellation, concurrent segment search for aggregations and snapshot inter-operability w/remote storage in OpenSearch. Some significant issues and PRs authored by Sorabh for this effort are as follows:

Feature proposals
Concurrent segment search for aggregations : #6798
Searchable Remote Index : #2900

Implementations
Concurrent segment search for aggregations: #7514
Lucene changes to leaf slices for concurrent search: apache/lucene#12374
Moving concurrent search to core : #7203
Query cancellation support : #986
In total, Sorabh has authored 14 PRs going back to Aug 2021. He also frequently reviews contributions from others and has reviewed nearly 20 PRs in the same time frame. I believe Sorabh will be a valuable addition as a maintainer of OpenSearch and will continue to contribute to the success of the project going forward.


(cherry picked from commit 7769682)

Signed-off-by: Bukhtawar Khan <[email protected]>
baba-devv pushed a commit to baba-devv/OpenSearch that referenced this pull request Jul 29, 2023
I have nominated and maintainers have agreed to invite Sorabh Hamirwasia(@sohami) to be a co-maintainer. Sorabh has kindly accepted.

Sorabh has led the design and implementation of multiple features like query cancellation, concurrent segment search for aggregations and snapshot inter-operability w/remote storage in OpenSearch. Some significant issues and PRs authored by Sorabh for this effort are as follows:

Feature proposals
Concurrent segment search for aggregations : opensearch-project#6798
Searchable Remote Index : opensearch-project#2900

Implementations
Concurrent segment search for aggregations: opensearch-project#7514
Lucene changes to leaf slices for concurrent search: apache/lucene#12374
Moving concurrent search to core : opensearch-project#7203
Query cancellation support : opensearch-project#986
In total, Sorabh has authored 14 PRs going back to Aug 2021. He also frequently reviews contributions from others and has reviewed nearly 20 PRs in the same time frame. I believe Sorabh will be a valuable addition as a maintainer of OpenSearch and will continue to contribute to the success of the project going forward.

Signed-off-by: Bukhtawar Khan <[email protected]>
kaushalmahi12 pushed a commit to kaushalmahi12/OpenSearch that referenced this pull request Sep 12, 2023
I have nominated and maintainers have agreed to invite Sorabh Hamirwasia(@sohami) to be a co-maintainer. Sorabh has kindly accepted.

Sorabh has led the design and implementation of multiple features like query cancellation, concurrent segment search for aggregations and snapshot inter-operability w/remote storage in OpenSearch. Some significant issues and PRs authored by Sorabh for this effort are as follows:

Feature proposals
Concurrent segment search for aggregations : opensearch-project#6798
Searchable Remote Index : opensearch-project#2900

Implementations
Concurrent segment search for aggregations: opensearch-project#7514
Lucene changes to leaf slices for concurrent search: apache/lucene#12374
Moving concurrent search to core : opensearch-project#7203
Query cancellation support : opensearch-project#986
In total, Sorabh has authored 14 PRs going back to Aug 2021. He also frequently reviews contributions from others and has reviewed nearly 20 PRs in the same time frame. I believe Sorabh will be a valuable addition as a maintainer of OpenSearch and will continue to contribute to the success of the project going forward.

Signed-off-by: Bukhtawar Khan <[email protected]>
Signed-off-by: Kaushal Kumar <[email protected]>
brusic pushed a commit to brusic/OpenSearch that referenced this pull request Sep 25, 2023
I have nominated and maintainers have agreed to invite Sorabh Hamirwasia(@sohami) to be a co-maintainer. Sorabh has kindly accepted.

Sorabh has led the design and implementation of multiple features like query cancellation, concurrent segment search for aggregations and snapshot inter-operability w/remote storage in OpenSearch. Some significant issues and PRs authored by Sorabh for this effort are as follows:

Feature proposals
Concurrent segment search for aggregations : opensearch-project#6798
Searchable Remote Index : opensearch-project#2900

Implementations
Concurrent segment search for aggregations: opensearch-project#7514
Lucene changes to leaf slices for concurrent search: apache/lucene#12374
Moving concurrent search to core : opensearch-project#7203
Query cancellation support : opensearch-project#986
In total, Sorabh has authored 14 PRs going back to Aug 2021. He also frequently reviews contributions from others and has reviewed nearly 20 PRs in the same time frame. I believe Sorabh will be a valuable addition as a maintainer of OpenSearch and will continue to contribute to the success of the project going forward.

Signed-off-by: Bukhtawar Khan <[email protected]>
Signed-off-by: Ivan Brusic <[email protected]>
shiv0408 pushed a commit to Gaurav614/OpenSearch that referenced this pull request Apr 25, 2024
I have nominated and maintainers have agreed to invite Sorabh Hamirwasia(@sohami) to be a co-maintainer. Sorabh has kindly accepted.

Sorabh has led the design and implementation of multiple features like query cancellation, concurrent segment search for aggregations and snapshot inter-operability w/remote storage in OpenSearch. Some significant issues and PRs authored by Sorabh for this effort are as follows:

Feature proposals
Concurrent segment search for aggregations : opensearch-project#6798
Searchable Remote Index : opensearch-project#2900

Implementations
Concurrent segment search for aggregations: opensearch-project#7514
Lucene changes to leaf slices for concurrent search: apache/lucene#12374
Moving concurrent search to core : opensearch-project#7203
Query cancellation support : opensearch-project#986
In total, Sorabh has authored 14 PRs going back to Aug 2021. He also frequently reviews contributions from others and has reviewed nearly 20 PRs in the same time frame. I believe Sorabh will be a valuable addition as a maintainer of OpenSearch and will continue to contribute to the success of the project going forward.

Signed-off-by: Bukhtawar Khan <[email protected]>
Signed-off-by: Shivansh Arora <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation pending Tracks issues which have PRs merged but documentation changes pending
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants