Delay the request size calculation until required by the indexing pressure framework #1592
Conversation
…ssure framework. (opensearch-project#1560) Signed-off-by: Saurabh Singh <sisurab@amazon.com>
|
✅ Gradle Wrapper Validation success 14cbd72 |
|
Can one of the admins verify this patch? |
|
✅ Gradle Precommit success 14cbd72 |
| @@ -25,16 +27,18 @@ public IndexingPressureService(Settings settings, ClusterService clusterService) | |||
| shardIndexingPressure = new ShardIndexingPressure(settings, clusterService); | |||
| } | |||
|
|
|||
| public Releasable markCoordinatingOperationStarted(long bytes, boolean forceExecution) { | |||
| public Releasable markCoordinatingOperationStarted(BulkRequest bulkRequest, boolean forceExecution) { | |||
There was a problem hiding this comment.
I see this is only used by TransportBulkAction. Is that the only place this is ever intended to be used? Shouldn't we just move this method out of the service if that's the case? If not then lets refactor the method to take the interface?
| public Releasable markCoordinatingOperationStarted(BulkRequest bulkRequest, boolean forceExecution) { | |
| public Releasable markCoordinatingOperationStarted(Accountable accountable, boolean forceExecution) { |
There was a problem hiding this comment.
Yes, it makes perfect sense to use an Interface instead and keep the contract clean. Refactored to use a Supplier.
| return shardIndexingPressure.markCoordinatingOperationStarted(bytes, forceExecution); | ||
| } else { | ||
| return () -> {}; | ||
| } | ||
| } | ||
|
|
||
| public Releasable markCoordinatingOperationStarted(ShardId shardId, long bytes, boolean forceExecution) { | ||
| public Releasable markCoordinatingOperationStarted(ShardId shardId, BulkShardRequest bulkShardRequest, boolean forceExecution) { |
There was a problem hiding this comment.
Same here:
| public Releasable markCoordinatingOperationStarted(ShardId shardId, BulkShardRequest bulkShardRequest, boolean forceExecution) { | |
| public Releasable markCoordinatingOperationStarted(ShardId shardId, Accountable accountable, boolean forceExecution) { |
| @@ -631,7 +630,7 @@ protected void doRun() { | |||
| final boolean isOnlySystem = isOnlySystem(bulkRequest, clusterService.state().metadata().getIndicesLookup(), systemIndices); | |||
There was a problem hiding this comment.
Can we delay this isOnlySystem call as well? Like the ramBytesUsed() it's not needed for anything other than the releasable.
There was a problem hiding this comment.
Keeping this consistent with the other interface for Node level accounting markCoordinatingOperationStarted(LongSupplier bytes, boolean forceExecution) as it is supplied with a precomputed boolean, being used elsewhere too.
Also given isOnlySystem streams over a precomputed collection (unmodifiable set) of different indices being updated in that bulk request, we don’t expect this number (indices in a single request) to grow too high to be an overhead.
|
✅ Gradle Wrapper Validation success 365b20af630fb040863ae2e80e6c9f3bd27db723 |
…es calculation. Signed-off-by: Saurabh Singh <sisurab@amazon.com>
|
✅ Gradle Precommit success 365b20af630fb040863ae2e80e6c9f3bd27db723 |
|
✅ Gradle Wrapper Validation success 0accbbb |
|
✅ Gradle Precommit success 0accbbb |
|
Thank you @nknize for taking the look. I have updated the PR and added some java doc too. |
|
✅ Gradle Check success 365b20af630fb040863ae2e80e6c9f3bd27db723 |
|
stale PR. Reinvoking gradle check |
|
start gradle check |
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.