Removing hard coded value of max concurrent shard requests

[jainankitk]

Signed-off-by: Ankit Jain jain.ankitk@gmail.com

Description

Removes the hard coding for max concurrent shard requests

Issues Resolved

#3363

Check List

• New functionality includes testing.
  • All tests pass
• New functionality has been documented.
  • New functionality has javadoc added
• 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]

✅   Gradle Check success bd99b0b
Log 5415

Reports 5415

[Bukhtawar]

Thanks @jainankitk should we also add a test to ensure that we catch it if it accidentally gets reset to 1

[nknize]

Thanks for doing this. It puts the action in sync w/ AbstractSearchAsyncActionTests#createAction. It's up to you if you want to add a test to make sure the maxConcurrentRequestsPerNode member variable keeps the value. I don't think it's necessary and will likely require you to add a getter method just for the test.

[jainankitk]

@nknize @Bukhtawar - Yeah, I could not think of good way to add test. Maybe we can merge the PR, to fix the regression asap and I will keep the issue open to add test whenever I have time?

[nknize]

Maybe we can merge the PR, to fix the regression asap and I will keep the issue open to add test whenever I have time?

+1 So we don't miss (and in case another contributor wants to pick it up) go ahead and open an issue and throw a TODO in the code w/ a link to the issue.

[reta]

@nknize backport to 2.0, wdyt?

[nknize]

backport to 2.0, wdyt?

yes

[nknize]

+1 So we don't miss (and in case another contributor wants to pick it up) go ahead and open an issue and throw a TODO in the code w/ a link to the issue.

don't worry about the TODO in the code (they're often overlooked and rarely taken out).. let's just open a low hanging fruit issue to track the test.

[jainankitk]

don't worry about the TODO in the code (they're often overlooked and rarely taken out).. let's just open a low hanging fruit issue to track the test.

@nknize Thank you for promptly merging this fix. Created separate issue for adding test #3384

[dblock]

It's up to you if you want to add a test to make sure the maxConcurrentRequestsPerNode member variable keeps the value. I don't think it's necessary and will likely require you to add a getter method just for the test.

FWIW, I don't agree, it's a bug, the value here isn't propagating properly in some scenario, so we should have a test. Otherwise I'll accidentally change it back to 1 and nothing will break.

[jainankitk]

FWIW, I don't agree, it's a bug, the value here isn't propagating properly in some scenario, so we should have a test. Otherwise I'll accidentally change it back to 1 and nothing will break.

I agree with @dblock that we must have test (not optional) for preventing this regression from happening again. Though, I agree with @nknize to not hold the check-in back for test if it takes few days to get the tests in. This code is not new, but rather artifact of regression due to other commit.

[nknize]

I'll accidentally change it back to 1 and nothing will break.

Nothing should break. 1 is a valid value.

the value here isn't propagating properly in some scenario

The value was straight hard coded to 1.

While the test should be as simple as ensuring the value matches the input, we need to avoid relaxing modifiers just for tests. So that needs a little more thought. It can wait to not hold up the fix and addressed in a follow up.

[Bukhtawar]

@nknize Totally agree. Sure 1 is a valid value however we cannot allow the change to be reverted by a future commit for the same reason that it was tagged as a BUG.
I am glad we have a follow up issue but what I am afraid is if it doesn't get picked up now unlikely we would see that getting picked up in near future

[nknize]

if it doesn't get picked up now unlikely we would see that getting picked up in near future

That's a fair concern. Label as a good first issue and high severity and it shouldn't linger. @jainankitk do you also want to assign it to yourself to prioritize it now that the fix is merged?

[jainankitk]

@nknize @Bukhtawar I took first stab at it and after spending some time came up with below assertion. I liked this approach as we are not adding relaxing modifiers just for testing. Unfortunately, none of our tests trigger this code path at all! :( So, I guess this is going to take more time than expected.

Open to any suggestions for expediting this.

diff --git a/server/src/main/java/org/opensearch/action/search/AbstractSearchAsyncAction.java b/server/src/main/java/org/opensearch/action/search/AbstractSearchAsyncAction.java
index 1d6d3f284d5..40070827082 100644
--- a/server/src/main/java/org/opensearch/action/search/AbstractSearchAsyncAction.java
+++ b/server/src/main/java/org/opensearch/action/search/AbstractSearchAsyncAction.java
@@ -152,6 +152,7 @@ abstract class AbstractSearchAsyncAction<Result extends SearchPhaseResult> exten
         // on a per shards level we use shardIt.remaining() to increment the totalOps pointer but add 1 for the current shard result
         // we process hence we add one for the non active partition here.
         this.expectedTotalOps = shardsIts.totalSizeWith1ForEmpty();
+        assert maxConcurrentRequestsPerNode == request.getMaxConcurrentShardRequests() || maxConcurrentRequestsPerNode == shardsIts.size();
         this.maxConcurrentRequestsPerNode = maxConcurrentRequestsPerNode;
         // in the case were we have less shards than maxConcurrentRequestsPerNode we don't need to throttle
         this.throttleConcurrentRequests = maxConcurrentRequestsPerNode < shardsIts.size();