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

Revise search backpressure documentation for 2.6 #2989

Merged
merged 7 commits into from
Feb 28, 2023

Conversation

kolchfa-aws
Copy link
Collaborator

Fixes #2629

Checklist

  • By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and subject to the Developers Certificate of Origin.
    For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Fanit Kolchina <kolchfa@amazon.com>
@kolchfa-aws kolchfa-aws requested a review from a team as a code owner February 20, 2023 22:55
@kolchfa-aws kolchfa-aws self-assigned this Feb 20, 2023
Signed-off-by: Fanit Kolchina <kolchfa@amazon.com>
@kolchfa-aws
Copy link
Collaborator Author

@PritLadani Thanks so much for the updates. Could you look over this final version of the PR for technical accuracy please? Thanks!

Copy link

@PritLadani PritLadani left a comment

Choose a reason for hiding this comment

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

LGTM.

search_backpressure.search_task.total_heap_percent_threshold | 5% | The heap usage threshold (as a percentage) required for the sum of heap usages of all search tasks before cancellation is applied.
search_backpressure.search_task.heap_variance | 2.0 | The heap usage variance required for an individual parent task before it is considered for cancellation. A task is considered for cancellation when `taskHeapUsage` is greater than or equal to `heapUsageMovingAverage` * `variance`.
search_backpressure.search_task.heap_moving_average_window_size | 10 | The window size to calculate the rolling average of the heap usage for the completed parent tasks.
search_backpressure.search_task.cpu_time_millis_threshold | 30,000 | The CPU usage threshold (in milliseconds) required for an individual parent task before it is considered for cancellation.
Copy link
Contributor

@carolxob carolxob Feb 23, 2023

Choose a reason for hiding this comment

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

Global suggestion: remove the parentheses and add commas to instances of this in this section.

Suggested change
search_backpressure.search_task.cpu_time_millis_threshold | 30,000 | The CPU usage threshold (in milliseconds) required for an individual parent task before it is considered for cancellation.
search_backpressure.search_task.cpu_time_millis_threshold | 30,000 | The CPU usage threshold, in milliseconds, required for an individual parent task before it is considered for cancellation.

Copy link
Contributor

Choose a reason for hiding this comment

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

I second this

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the old wording from the original PR that has been through editorial. Let's ask @natebower's opinion.

Copy link
Collaborator

Choose a reason for hiding this comment

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

They are both grammatically correct and essentially serve the same purpose, so this is a matter of subjective preference.

Copy link
Contributor

@carolxob carolxob left a comment

Choose a reason for hiding this comment

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

LGTM other than one minor suggestion.

Copy link
Contributor

@ariamarble ariamarble left a comment

Choose a reason for hiding this comment

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

Looks good other than single comment.

Copy link
Collaborator

@natebower natebower left a comment

Choose a reason for hiding this comment

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

@kolchfa-aws Just one added word :-)

search_backpressure.search_task.heap_percent_threshold | 2% | The heap usage threshold (as a percentage) required for an individual parent task before it is considered for cancellation.
search_backpressure.search_task.total_heap_percent_threshold | 5% | The heap usage threshold (as a percentage) required for the sum of heap usages of all search tasks before cancellation is applied.
search_backpressure.search_task.heap_variance | 2.0 | The heap usage variance required for an individual parent task before it is considered for cancellation. A task is considered for cancellation when `taskHeapUsage` is greater than or equal to `heapUsageMovingAverage` * `variance`.
search_backpressure.search_task.heap_moving_average_window_size | 10 | The window size to calculate the rolling average of the heap usage for the completed parent tasks.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
search_backpressure.search_task.heap_moving_average_window_size | 10 | The window size to calculate the rolling average of the heap usage for the completed parent tasks.
search_backpressure.search_task.heap_moving_average_window_size | 10 | The window size used to calculate the rolling average of the heap usage for the completed parent tasks.

search_backpressure.search_task.total_heap_percent_threshold | 5% | The heap usage threshold (as a percentage) required for the sum of heap usages of all search tasks before cancellation is applied.
search_backpressure.search_task.heap_variance | 2.0 | The heap usage variance required for an individual parent task before it is considered for cancellation. A task is considered for cancellation when `taskHeapUsage` is greater than or equal to `heapUsageMovingAverage` * `variance`.
search_backpressure.search_task.heap_moving_average_window_size | 10 | The window size to calculate the rolling average of the heap usage for the completed parent tasks.
search_backpressure.search_task.cpu_time_millis_threshold | 30,000 | The CPU usage threshold (in milliseconds) required for an individual parent task before it is considered for cancellation.
Copy link
Collaborator

Choose a reason for hiding this comment

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

They are both grammatically correct and essentially serve the same purpose, so this is a matter of subjective preference.

Signed-off-by: Fanit Kolchina <kolchfa@amazon.com>
@kolchfa-aws kolchfa-aws added release-notes PR: Include this PR in the automated release notes v2.6.0 labels Feb 24, 2023
@kolchfa-aws kolchfa-aws added this to the v2.6 milestone Feb 24, 2023
@kolchfa-aws kolchfa-aws merged commit 826b46b into main Feb 28, 2023
@Naarcha-AWS Naarcha-AWS deleted the Fix2629-search-backpressure branch March 28, 2024 23:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes PR: Include this PR in the automated release notes v2.6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DOC] Search backpressure cluster settings documentation
5 participants