Changes ConcurrentSearchTasksIT to use custom slice count #17355
Conversation
rampreeth
requested review from
anasalkouz,
andrross,
ashking94,
Bukhtawar,
CEHENKLE,
cwperks,
dblock,
dbwiddis,
gbbafna,
jainankitk,
kotwanikunal,
linuxpi,
mch2,
msfroh,
nknize,
owaiskazi19,
reta,
Rishikesh1159,
sachinpkale,
saratvemulapalli,
shwetathareja,
sohami and
VachaShah
as code owners
github-actions
bot
added
:test
|
❌ Gradle check result for 8ea75ef: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Signed-off-by: Rampreeth Ethiraj <ethirajrampreeth@gmail.com>
rampreeth
force-pushed
the
issue/9317
branch
from
8ea75ef to
9abade8
Compare
|
❌ Gradle check result for 9abade8: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Signed-off-by: Rampreeth Ethiraj <ethirajrampreeth@gmail.com>
|
❌ Gradle check result for c4d000a: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Signed-off-by: Rampreeth Ethiraj <ethirajrampreeth@gmail.com>
|
❌ Gradle check result for fa04c65: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
| @@ -248,6 +248,7 @@ static Setting<Integer> buildNumberOfShardsSetting() { | |||
| static final String MAX_NUMBER_OF_SHARDS = "opensearch.index.max_number_of_shards"; | |||
| public static final Setting<Integer> INDEX_NUMBER_OF_SHARDS_SETTING = buildNumberOfShardsSetting(); | |||
| public static final String SETTING_NUMBER_OF_REPLICAS = "index.number_of_replicas"; | |||
| public static final String MAX_SLICE_COUNT = "index.search.concurrent.max_slice_count"; | |||
There was a problem hiding this comment.
Can we either also use this same string in the setting name:
OpenSearch/server/src/main/java/org/opensearch/index/IndexSettings.java
Lines 727 to 733 in 442a928
or, we can just do:
INDEX_CONCURRENT_SEGMENT_SEARCH_MAX_SLICE_COUNT.getKey()
That way if we ever need to change the setting there's only 1 place to modify instead of 2.
There was a problem hiding this comment.
I think this comment needs to be addressed still too
| @@ -89,6 +90,7 @@ public void testConcurrentSearchTaskTracking() { | |||
| Settings.builder() | |||
| .put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, NUM_SHARDS) | |||
| .put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 0) | |||
| .put(IndexMetadata.MAX_SLICE_COUNT, 5) | |||
There was a problem hiding this comment.
One main goal of using the setting name is so we can use a random number here instead of hard coding to 5
There was a problem hiding this comment.
@jed326 , from what I understood, we need to assert the segment count by validating against the slice count that i sset. So, this particular line is where we're setting the slice count. Even if we were to use a random number, that particular value would still be hardcoded right? Please correct me if I'm missing something here
There was a problem hiding this comment.
More specifically we're asserting that the stats are being tracked for all the slices. So the idea is if slice count is set to 2 then we check that the stats are tracked for 2 slices + search thread, etc. So setting the slice count to a random number between (1, num_segments) ensures we have full coverage and not some magical dependency on specifically 5 for example.
There was a problem hiding this comment.
Got it, thanks for the clarification, will make the necessary changes
Signed-off-by: Rampreeth Ethiraj <ethirajrampreeth@gmail.com>
|
❌ Gradle check result for a519924: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
There was a problem hiding this comment.
Thanks @rampreeth , it looks like the spotless checks are failing too: https://build.ci.opensearch.org/job/gradle-check/53580/
Run './gradlew :server:spotlessApply' to fix these violations.
|
|
||
| Metadata metadata = client().admin().cluster().prepareState().get().getState().metadata(); | ||
| assertEquals( | ||
| Integer.parseInt(metadata.index(INDEX_NAME).getSettings().get(IndexMetadata.MAX_SLICE_COUNT)), |
There was a problem hiding this comment.
I think this should be the min of the slice count setting and the number of segments. Otherwise this test might be a little flaky depending on the refresh interval of the index and the maxSliceCount (for example, if maxSliceCount == 7 but somehow only 5 segments were created, then this should be only 5 still).
Description
Changes ConcurrentSearchTasksIT to use custom slice count
Related Issues
Resolves #9317
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.