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

Introduce new setting search.concurrent.max_slice to control the slice computation for concurrent segment search #8847

Closed
wants to merge 1 commit into from

Conversation

sohami
Copy link
Collaborator

@sohami sohami commented Jul 24, 2023

Description

Introduces a static setting to control slice computation using lucene default or max target slice mechanism for concurrent segment search.

Related Issues

Resolves #[Issue number to be closed when this PR is merged]
#7358

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
  • Commit changes are listed out in CHANGELOG.md file (See: Changelog)

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.

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

…e computation for concurrent

segment search. It uses lucene default mechanism if the setting value is <=0 otherwise uses custom max target slice mechanism

Signed-off-by: Sorabh Hamirwasia <sohami.apache@gmail.com>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

import org.opensearch.common.settings.Settings;

/**
* Keeps track of all the search related node level settings which can be accessed via static methods
Copy link
Member

Choose a reason for hiding this comment

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

Please add @opensearch.internal

@@ -518,4 +537,19 @@ private boolean shouldReverseLeafReaderContexts() {
}
return false;
}

// package-private for testing
LeafSlice[] slicesInternal(List<LeafReaderContext> leaves, int target_max_slices) {
Copy link
Member

Choose a reason for hiding this comment

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

targetMaxSlices to follow convention

settings = openSearchSettings;
}

public static int getValueAsInt(String settingName, int defaultValue) {
Copy link
Member

Choose a reason for hiding this comment

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

This is a bit weird because any setting can be passed in here even if unrelated to search. I'd probably implement this as SearchBootstrapSettings.getTargetMaxSliceCount(). I'd consider using Optional<Integer> or a nullable Integer as the return type as opposed to using the magic value of -1.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  1. I thought about adding the getTargetMaxSliceCount method but then it will not keep this class generic which can be utilized for other search settings. For each new setting we will need to add specific method. But I see your point about being able to access any settings. But that is true for any access to ClusterSettings from different components as well. Let me know if you still prefer adding explicit method getTargetMaxSliceCount here.
  2. -1 is the default value for this setting as it doesn't allow null for it. When the setting will be converted to dynamic then it will return the default value when setting value is looked up using clusterSettings.get(CONCURRENT_SEGMENT_SEARCH_TARGET_MAX_SLICE_COUNT_SETTING) and it is not explicitly set. It will not return null so caller needs to handle this default of -1 to fallback to lucene behavior in that case. Hence keeping it as is without adding nullability check for now. The cases will be:
  • Not set explicitly --> default value will be returned so use lucene slice computation
  • Set explicitly to -1/0 --> use lucene slice computation
  • Set explicitly to >0 --> use custom slice computation

* Keeps track of all the search related node level settings which can be accessed via static methods
*/
public class SearchBootstrapSettings {
// settings to configure maximum slice created per search request using OS custom slice computation mechanism. Default lucene
Copy link
Member

Choose a reason for hiding this comment

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

Is there any question as to whether we will switch away from the static method to the dynamic method when the next release of Lucene is available? If not, I'd go ahead and create a GitHub issue to track it and link the issue in a comment in the code where appropriate.

Copy link
Collaborator

@reta reta Jul 25, 2023

Choose a reason for hiding this comment

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

@sohami to this point, why this setting is static? As far as I can tell, it is used in non-static context and could be implemented as a regular search-related setting.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@reta The slices method here is called from the constructor of the IndexSearcher in 9.7. Due to this it doesn't allow to make it configurable using any member variable of ContextIndexSearcher. This is changed in the lucene PR here and will be available in 9.8 which is when we can move to a dynamic setting.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks @sohami the main is moving to 9.8 #8668

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thats interesting, do we usually update main with the lucene snapshot builds as well ? Was wondering if we take any dependency on a new change in unreleased lucene version and that gets changed in released lucene version then the change will break.

But nonetheless we can keep this change as is and backport to 2.x until 9.8 is officially released. Then add a follow-up commit to move it to dynamic setting as part of #8870

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since now main is already moved to 9.8.0-snapshot version of lucene, I will add a follow-up PR to clean this up in main and have a dynamic mechanism but not backport that to 2.x.

I am trying to understand why do you want to get this change in main? We know this is not the way to go forward, and we do have the solution, you will have to redo this work in two branches instead of just cherry picking one simple change into 2.x.

Copy link
Collaborator Author

@sohami sohami Jul 25, 2023

Choose a reason for hiding this comment

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

@reta Let me try explaining :) . I do get your solution and can use that but would like to first try explaining why I am trying to merge it in main as well.

As an example in this PR, we have this new class SearchBootstrapSettings which provides static access to this new node setting. If I were to create a separate PR to make this setting as dynamic this class will not be needed.

  • With approach 1, if I merge this PR (say pr_1) only in 2.x this class will be present in 2.x only. Now the new PR (say pr_2) with dynamic setting will be built independent of this PR (pr_1) and will not know anything about this class as well and will go to main. When me move this new PR (pr_2) from main to 2.x, we will need to ensure the cleanup like for SearchBootstrapSettings and other conflicts happens properly and not get missed (which I was thinking can be messy as these 2 PRs are not built on top of each other).
  • With approach 2, if we merge pr_1 both in 2.x and main. Then add pr_2 in main on top of pr_1, then all the cleanup will be done as part of this pr_2 itself and it can be merged in same way in 2.x as well. I was thinking this backport to 2.x will be cleaner compared to approach 1 and hence was going with this approach. I may be missing something here but thanks for the discussion ?

Copy link
Collaborator

@reta reta Jul 25, 2023

Choose a reason for hiding this comment

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

When me move this new PR (pr_2) from main to 2.x, we will need to ensure the cleanup like for SearchBootstrapSettings and other conflicts happens properly and not get missed (which I was thinking can be messy as these 2 PRs are not built on top of each other).

This is inevitable in any case - we will be backporting all changes related to Lucene 9.8.0 (as we did for all previous Apache Lucene versions), the argument here is: keep main clean by using the new Apache Lucene snapshots (this is why we've been always doing that, giving the ride to the feature before the release - bugs do happen), do backport as a best effort.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will close this PR and open a new one against 2.x. Will make the dynamic setting change as separate PR for main branch. And will create another tracking backport issue for merging the dynamic setting change to 2.x when lucene 9.8 gets released. As part of that backport we can revert the commit for static setting in 2.x and then apply the commit from main.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@reta @andrross @jed326 Created PR #8884 for 2.x branch. Will be working on dynamic setting change for main branch.

* experiment results as shared in <a href=https://github.com/opensearch-project/OpenSearch/issues/7358>issue-7358</a>
* we can see this mechanism helps to achieve better tail/median latency over default lucene slice computation.
*/
public class MaxTargetSliceSupplier {
Copy link
Member

Choose a reason for hiding this comment

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

Add @opensearch.internal

*/
public class MaxTargetSliceSupplier {

public static IndexSearcher.LeafSlice[] getSlices(List<LeafReaderContext> leaves, int target_max_slice) {
Copy link
Member

Choose a reason for hiding this comment

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

targetMaxSlice

}

// slice count should not exceed the segment count
int target_slice_count = Math.min(target_max_slice, leaves.size());
Copy link
Member

Choose a reason for hiding this comment

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

targetSliceCount

Comment on lines +49 to +50
group = groupedLeaves.get(currentGroup);
group.add(sortedLeaves.get(idx));
Copy link
Member

Choose a reason for hiding this comment

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

The group local variable confused me when first reading this. Any reason not to just do groupedLeaves.get(currentGroup).add(sortedLeaves.get(idx)) instead?

Comment on lines +53 to +59
IndexSearcher.LeafSlice[] slices = new IndexSearcher.LeafSlice[target_slice_count];
int upto = 0;
for (List<LeafReaderContext> currentLeaf : groupedLeaves) {
slices[upto] = new IndexSearcher.LeafSlice(currentLeaf);
++upto;
}
return slices;
Copy link
Member

Choose a reason for hiding this comment

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

Can you replace this with the following?

return groupedLeaves.stream()
    .map(IndexSearcher.LeafSlice::new)
    .toArray(IndexSearcher.LeafSlice[]::new);

Comment on lines +215 to +218
// By default, for tests we will put the target slice count of 2. This will increase the probability of having multiple slices
// when tests are run with concurrent segment search enabled. When concurrent segment search is disabled then it's a no-op as
// slices are not used
return Settings.builder().put(SearchBootstrapSettings.CONCURRENT_SEGMENT_SEARCH_TARGET_MAX_SLICE_COUNT_KEY, 2).build();
Copy link
Member

Choose a reason for hiding this comment

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

Maybe move this to newNode() where the other defaults are defined and leave this implementation as returning EMPTY?

@@ -211,7 +212,10 @@ protected final Collection<Class<? extends Plugin>> pluginList(Class<? extends P

/** Additional settings to add when creating the node. Also allows overriding the default settings. */
protected Settings nodeSettings() {
return Settings.EMPTY;
// By default, for tests we will put the target slice count of 2. This will increase the probability of having multiple slices
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you think there is still value in testing the concurrent segment search case with 1 slice?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

was thinking that code path is mostly same between 1 or more slices like reducing and creating the final results. So it is more a subset of the slice > 1 case. Also the max slice of 2 will still have coverage for single slice if the number of segments created by the tests is 1.

leafSlices = super.slices(leaves);
logger.debug("Slice count using lucene default [{}]", leafSlices.length);
} else {
// use the custom slice calculation based on target_max_slices. It will sort
Copy link
Collaborator

Choose a reason for hiding this comment

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

This comment looks cut off

Comment on lines +24 to +28
public static final Setting<Integer> CONCURRENT_SEGMENT_SEARCH_TARGET_MAX_SLICE_COUNT_SETTING = Setting.intSetting(
CONCURRENT_SEGMENT_SEARCH_TARGET_MAX_SLICE_COUNT_KEY,
CONCURRENT_SEGMENT_SEARCH_TARGET_MAX_SLICE_COUNT_DEFAULT_VALUE,
Setting.Property.NodeScope
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should use the intSetting with minValue (and maybe max value):

public static Setting<Integer> intSetting(String key, int defaultValue, int minValue, int maxValue, Property... properties) {
return intSetting(key, defaultValue, minValue, maxValue, v -> {}, properties);
}

For max value, it is naturally bounded by the number of segments, which shouldn't grow unbounded due to Lucene merges so I'm inclined to say that is not necessary.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't see this as a must have since there is no true bounds we are enforcing for now. Anything < 0 is treated as to use the lucene mechanism vs custom mechanism. And if a large +ve value is set that will be tuned down to the segment count in the slice computer logic.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thinking about this some more, I agree that this is not a must have from a functional perspective since the <0 case is handled in slicesInternal, but since there's no valid use case for anyone to set, for example -200 for this setting (and I don't foresee this becoming a valid use case in the future either), it's better to just disallow it entirely.
Agree that max value is not necessary though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

but since there's no valid use case for anyone to set, for example -200 for this setting

This setting is used in 2 ways. If value > 0 then use the custom slice mechanism and use the value as target max slice. If value is <=0 then use the lucene slice mechanism. So here the actual negative value or 0 is not relevant other than meaning fallback to lucene behavior. It is used for enabling/disabling the feature (which is custom slicer vs lucene slicer). I will prefer min/max range for settings where there is clear range defined. Here <=0 is used a boolean flag to control using one feature over other. Also didn't want to add a new setting to control that, as I would expect based on the test we will default to one behavior.

@sohami
Copy link
Collaborator Author

sohami commented Jul 25, 2023

Closing this PR as discussed here and will address feedback here in the new PR against 2.x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants