Make SliceExecutor extensible and include method to computeSlices. This will allow different custom implementation to be plugged in with IndexSearcher to compute and execute slices on provided executor #12348

sohami · 2023-06-05T22:02:09Z

Description

As mentioned in the issue #12347, this PR is allowing to extend SliceExecutor and move the slice computation in it. This will allow the extensions of IndexSearcher to provide custom SliceExecutor and also custom logic to compute and execute the slices for an index.

…is will allow different custom implementation to be plugged in with IndexSearcher to compute and execute slices on provided executor

javanna

Thanks for working on this, I left some comments. I am curious to hear what others think about it as well, for instance @jpountz .

javanna · 2023-06-08T21:07:33Z

lucene/core/src/java/org/apache/lucene/search/IndexSearcher.java

+   *
+   * @lucene.experimental
+   */
+  public IndexSearcher(IndexReaderContext context, Executor executor, SliceExecutor sliceExecutor) {


I wonder if we should make this constructor public, or instead make the getSliceExecutionControlPlane method public. Would this second option work for what you need or would you still run into issues as it's called from the IndexSearcher constructor a bit like slices?

Otherwise I'd think we should look at having a single way, either provide an Executor or a SliceExecutor.

I wonder if we should make this constructor public, or instead make the getSliceExecutionControlPlane method public. Would this second option work for what you need or would you still run into issues as it's called from the IndexSearcher constructor a bit like slices?

Yes that would result in same issue

Otherwise I'd think we should look at having a single way, either provide an Executor or a SliceExecutor.

There are already constructors which takes in Executor. So was unable to modify this one to just take in SliceExecutor. I also added an assertion for this constructor that executor instance is same as one in SliceExecutor

javanna · 2023-06-08T21:09:33Z

lucene/core/src/java/org/apache/lucene/search/SliceExecutor.java

@@ -55,4 +74,8 @@ boolean shouldExecuteOnCallerThread(int index, int numTasks) {
    // Execute last task on caller thread
    return index == numTasks - 1;
  }
+
+  public Executor getExecutor() {


looks like there would be no need for this method if we made getSliceExecutionControlPlane public

javanna · 2023-06-08T21:10:08Z

lucene/core/src/java/org/apache/lucene/search/SliceExecutor.java


 /**
 * Executor which is responsible for execution of slices based on the current status of the system
 * and current system load
 */
-class SliceExecutor {
+public class SliceExecutor {


I am thinking this class should be marked experimental if it's made public

javanna · 2023-06-08T21:14:51Z

lucene/core/src/java/org/apache/lucene/search/SliceExecutor.java

+   * @return computed slices
+   */
+  public LeafSlice[] computeSlices(List<LeafReaderContext> leaves) {
+    return IndexSearcher.slices(leaves, MAX_DOCS_PER_SLICE, MAX_SEGMENTS_PER_SLICE);


I wonder if it'd make sense to move the slices static method from IndexSearcher to SliceExecutor, so that we don't have to call IndexSearcher.slices from here.

Didn't moved it to avoid breaking consumers as static slices method is public

you could still have the existing method call the one exposed by the SliceExecutor.

javanna · 2023-06-08T21:18:02Z

lucene/core/src/java/org/apache/lucene/search/IndexSearcher.java

@@ -319,11 +323,10 @@ public QueryCachingPolicy getQueryCachingPolicy() {

  /**
   * Expert: Creates an array of leaf slices each holding a subset of the given leaves. Each {@link
-   * LeafSlice} is executed in a single thread. By default, segments with more than
-   * MAX_DOCS_PER_SLICE will get their own thread
+   * LeafSlice} is executed in a single thread.
   */
  protected LeafSlice[] slices(List<LeafReaderContext> leaves) {


I don't see the point of overriding this if we are making SliceExecutor public in order for it to be extended. I would remove this method from IndexSearcher.

I kept it like this to avoid breaking extensions already overriding this method. I wanted to backport this change to 9.x branch as well so wanted to avoid any breaking change.

That's good thinking, but it may be ok to make this breaking change, depending on how the discussion progresses in the issue, and then make this backwards compatible when backporting to 9.x

javanna · 2023-06-08T21:19:13Z

lucene/core/src/java/org/apache/lucene/search/IndexSearcher.java

-      Collections.sort(leavesList, Comparator.comparingInt(l -> l.docBase));
-      this.leaves = leavesList.toArray(new LeafReaderContext[0]);
-    }
-  }


I wonder if it's necessary to move this class to be an outer class. It is marked experimental, so we should be free to move it, yet I would tend to avoid breaking consumers if possible.

Ack I can move it back since it was getting used by IndexSearcher and SliceExecutor, I thought of moving it out. But you are right it can break the consumers.

javanna · 2023-06-08T21:23:42Z

lucene/core/src/java/org/apache/lucene/search/SliceExecutor.java

+    return IndexSearcher.slices(leaves, MAX_DOCS_PER_SLICE, MAX_SEGMENTS_PER_SLICE);
+  }
+
+  public void invokeAll(Collection<? extends Runnable> tasks) {


Could you add javadocs now that the method is public?

javanna · 2023-06-08T21:24:21Z

lucene/core/src/java/org/apache/lucene/search/SliceExecutor.java

@@ -55,4 +74,8 @@ boolean shouldExecuteOnCallerThread(int index, int numTasks) {
    // Execute last task on caller thread


should we make shouldExecuteOnCallerThread also public?

Didn't see a need for that as it is internal to the implementation of SliceExecutor. A custom SliceExecutor can define its own implementation of invokeAll as needed.

sohami · 2023-06-08T21:51:55Z

Thanks for working on this, I left some comments. I am curious to hear what others think about it as well, for instance @jpountz .

@javanna Thanks for taking a look, have responded to your feedback.

jpountz · 2023-06-09T07:51:31Z

I am curious to hear what others think about it as well, for instance @jpountz .

FYI I dropped a comment on the linked issue as I have questions on the general approach we should take.

sohami · 2023-07-11T20:58:38Z

Closing this PR as it is solved by #12374

Make SliceExecutor extensible and include method to computeSlices. Th…

bff88dc

…is will allow different custom implementation to be plugged in with IndexSearcher to compute and execute slices on provided executor

sohami force-pushed the main branch from cb5163d to bff88dc Compare June 6, 2023 01:11

sohami mentioned this pull request Jun 7, 2023

[Concurrent Segment Search] Mechanism to have custom slice calculation for each shard opensearch-project/OpenSearch#7358

Closed

javanna self-requested a review June 7, 2023 07:54

javanna reviewed Jun 8, 2023

View reviewed changes

sohami closed this Jul 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make SliceExecutor extensible and include method to computeSlices. This will allow different custom implementation to be plugged in with IndexSearcher to compute and execute slices on provided executor #12348

Make SliceExecutor extensible and include method to computeSlices. This will allow different custom implementation to be plugged in with IndexSearcher to compute and execute slices on provided executor #12348

sohami commented Jun 5, 2023

javanna left a comment

javanna Jun 8, 2023

sohami Jun 8, 2023

javanna Jun 8, 2023

javanna Jun 8, 2023

javanna Jun 8, 2023

sohami Jun 8, 2023

javanna Jun 9, 2023

javanna Jun 8, 2023

sohami Jun 8, 2023 •

edited

Loading

javanna Jun 9, 2023

javanna Jun 8, 2023

sohami Jun 8, 2023 •

edited

Loading

javanna Jun 8, 2023

javanna Jun 8, 2023

sohami Jun 8, 2023

sohami commented Jun 8, 2023

jpountz commented Jun 9, 2023

sohami commented Jul 11, 2023

		@@ -55,4 +74,8 @@ boolean shouldExecuteOnCallerThread(int index, int numTasks) {
		// Execute last task on caller thread

Make SliceExecutor extensible and include method to computeSlices. This will allow different custom implementation to be plugged in with IndexSearcher to compute and execute slices on provided executor #12348

Make SliceExecutor extensible and include method to computeSlices. This will allow different custom implementation to be plugged in with IndexSearcher to compute and execute slices on provided executor #12348

Conversation

sohami commented Jun 5, 2023

Description

javanna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sohami Jun 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sohami Jun 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sohami commented Jun 8, 2023

jpountz commented Jun 9, 2023

sohami commented Jul 11, 2023

sohami Jun 8, 2023 •

edited

Loading

sohami Jun 8, 2023 •

edited

Loading