-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Delay the request size calculation until required by the indexing pre… #1589
Conversation
…ssure framework. (opensearch-project#1560) Signed-off-by: Saurabh Singh <sisurab@amazon.com>
Can one of the admins verify this patch? |
✅ Gradle Wrapper Validation success b4488c3 |
✅ Gradle Precommit success b4488c3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's a good change either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should probably make this PR into main
now and we can merge, and go to 1.2 in backports like we always do if we decide to.
✅ Gradle Wrapper Validation success b4488c3 |
✅ Gradle Wrapper Validation success b4488c3 |
✅ Gradle Precommit success b4488c3 |
1 similar comment
✅ Gradle Precommit success b4488c3 |
Description
Delay the
ramBytesUsed
computation for Indexing requests on the coordinator until reached a point, where absolutely necessary for IndexingPressure or ShardIndexingPressure.Issues Resolved
This optimises the additional request size calculation, which otherwise would have been no-op. See (#1560)
The ramBytesUsed computations are done repeatedly at multiple places (replica, primary and coordinator), and are present even in the older version of IndexingPressure. Here in OS1.2 for every request overhead would be the request size computations for the number of documents in the batch. Given that this call has neither showed up in any CPU profiling data and was neither was called out as regression when first added in the indexing path during 7.9 release, this might not be the smoking gun, however this definitely optimises it down further.
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.