SOLR-13350, SOLR-17298: use multiThreaded=false default; document multiThreaded parameter; #2596

cpoerschke · 2024-07-26T10:46:37Z

https://issues.apache.org/jira/browse/SOLR-13350
https://issues.apache.org/jira/browse/SOLR-17298

…; add responseHeader.multiThreadedUsed element;

cpoerschke · 2024-07-26T11:07:20Z

https://github.com/apache/solr/actions/runs/10110021801/job/27959091476?pr=2596 CI failure looks unrelated

./gradlew :solr:core:test --tests "org.apache.solr.cloud.ReindexCollectionTest.testAbort" -Ptests.jvms=96 "-Ptests.jvmargs=-XX:TieredStopAtLevel=1 -XX:+UseParallelGC -XX:ActiveProcessorCount=1 -XX:ReservedCodeCacheSize=120m" -Ptests.seed=40BE4D7EC8B75511 -Ptests.timeoutSuite=600000! -Ptests.file.encoding=UTF-8

gus-asf · 2024-07-26T11:50:30Z

I'm not sure about the conversion to Boolean and only reporting if it was requested. That will still leave the default decision unclear if it wasn't requested explicitly. I think the logging of the query should be sufficient to identify if it was requested, and the logging in the code should report what was ultimately decided regardless of the request.

Co-authored-by: Gus Heck <[email protected]>

cpoerschke · 2024-07-26T12:30:40Z

solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java

-        if (log.isDebugEnabled()) {
-          log.debug("skipping collector manager");
+        if (cmd.getMultiThreaded() != null) {
+          // report multi-threading (non-)use only if multi-threading was explicitly (not) requested
+          log.info("SINGLE THREADED search, skipping collector manager");
+        } else {
+          log.debug("SINGLE THREADED search, skipping collector manager");


variation of @gus-asf's https://github.com/apache/solr/pull/2595/files#diff-99978700f1c69d6a5f6c2190f89c98cfe20c441161db5a183ec002e15cb1be28R1924 change.

chatman · 2024-07-29T13:20:43Z

Looks good, thanks @cpoerschke. +1 to merge.

solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java

dsmiley · 2024-07-29T23:00:13Z

solr/solr-ref-guide/modules/query-guide/pages/common-query-parameters.adoc

+If unset or set to `false`, then the multi-threaded search implementation will not be used for the query.
+If set to `true`, then use the multi-threaded search implementation if it is supported for the query.


Suggested simple wording:

'true' or 'false' controls if Solr may use more than one thread to satisfy the request.
It presently is only considered for QueryComponent to search across Lucene's segments in parallel.

I could see us using this parameter in the future for indexing.

Suggested simple wording:

'true' or 'false' controls if Solr may use more than one thread to satisfy the request.
It presently is only considered for QueryComponent to search across Lucene's segments in parallel.

Thanks! Will revise. I'd also intended to cross-reference the configuring-solr-xml.adoc documentation for indexSearcherExecutorThreads but lost track of that due to .adoc syntax.

…ultiThreaded

…adoc#indexSearcherExecutorThreads

gus-asf

Looks good, some word-smithing suggestions for the docs and probably we should refer to this as experimental.

gus-asf · 2024-08-01T13:24:49Z

solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java

@@ -371,7 +371,7 @@ public void process(ResponseBuilder rb) throws IOException {
      return;
    }

-    final boolean multiThreaded = params.getBool("multiThreaded", true);
+    final boolean multiThreaded = params.getBool(CommonParams.MULTI_THREADED, false);


gus-asf · 2024-08-01T13:25:09Z

solr/CHANGES.txt

@@ -103,7 +103,7 @@ Other Changes
 ==================  9.7.0 ==================
 New Features
 ---------------------
-* SOLR-13350: Multithreaded search execution (Ishan Chattopadhyaya, Mark Miller, Christine Poerschke, David Smiley, noble)
+* SOLR-13350, SOLR-17298: Opt-in multithreaded search execution (Ishan Chattopadhyaya, Mark Miller, Christine Poerschke, David Smiley, noble, Gus Heck)


Maybe call it experimental too?

Shouldn't this be in Optimizations? There is no new capability here; it's about performance. I realize a lot of work went into this but a user doesn't care; it's just a performance thing.

I think it's fine to leave it in as a feature for 2 reasons: 1) It's actively enabled/disabled by the user, and 2) This might affect how one thinks about sizing and sharding strategies. Previously on a cluster with a small to moderate sized index that isn't changing size quickly one might have added shards to be sure to take advantage of many cpu cores. Though I'm not sure if we expose it, one could now think of going single shard, multi-thread and limiting the segment size to ensure there are enough segments to fill up your cpu-cores... One could even think of trying a merge policy that targets maintaining a specific number of segments so that every search is fully utilizing the cores available (or utilizing a target fraction) on boxes with many cores?

It's a performance thing, but it's not an invisible touch free performance thing, which is what I think of when I use the word optimization.

gus-asf · 2024-08-01T13:30:04Z

solr/solr-ref-guide/modules/query-guide/pages/common-query-parameters.adoc

+|===
+
+This parameter set to `true` or `false` controls if Solr may use more than one thread to satisfy the request.
+It presently is only considered for QueryComponent to search across Lucene's segments in parallel and the xref:configuration-guide:configuring-solr-xml.adoc#indexSearcherExecutorThreads[indexSearcherExecutorThreads] value can be customised in the `solr.xml` file.


"It presently is only considered for" is unclear/confusing. Perhaps "Specifically this enables"

Also:
"segments in parallel and the xref:..." should either have a comma after parallel or be broken into two sentences (I prefer 2 sentences)

Also do we have a standardized way to mark things experimental in the docs (if not just mention it?)

It was even more unclear before :-). The "it presently" portion conveys it may have expanded uses later. After all, we chose a super generic name here instead of something referencing segment parallelism.

Indeed, it'd be nice to have a standard way, maybe an icon, to label experimental stuff. Ah well, even just textually [EXPERIMENTAL]

dsmiley · 2024-08-02T05:25:37Z

solr/CHANGES.txt

@@ -103,7 +103,7 @@ Other Changes
 ==================  9.7.0 ==================
 New Features
 ---------------------
-* SOLR-13350: Multithreaded search execution (Ishan Chattopadhyaya, Mark Miller, Christine Poerschke, David Smiley, noble)
+* SOLR-13350, SOLR-17298: Opt-in multithreaded search execution (Ishan Chattopadhyaya, Mark Miller, Christine Poerschke, David Smiley, noble, Gus Heck)


@markrmiller I didn't know that you contributed to this effort; what did you do?

cpoerschke · 2024-08-05T08:47:26Z

Thanks everyone for your inputs here so far!

Unfortunately I won't have bandwidth to iterate on this today or so. "Allow edits and access to secrets by maintainers" is enabled on the PR i.e. everyone please feel free to make edits and/or merge as you see fit. Thank you.

cc/FYI @anshumg as 9.7 release manager

dsmiley

Understood -- the "New Feature" category has merit.

solr/solr-ref-guide/modules/query-guide/pages/common-query-parameters.adoc

Update index search document punting of segmentsTerminateEarly to future release

…tiThreaded parameter; (#2596) Co-authored-by: Gus Heck <[email protected]> (cherry picked from commit 7ebbcd3)

…tiThreaded parameter; (#2596) Co-authored-by: Gus Heck <[email protected]> (cherry picked from commit 7ebbcd3) (cherry picked from commit 84d1fbc)

…tiThreaded parameter; (apache#2596) Co-authored-by: Gus Heck <[email protected]> (cherry picked from commit 7ebbcd3) (cherry picked from commit 84d1fbc)

SOLR-13350, SOLR-17298: document multiThreaded=(true,false) parameter…

9f9341b

…; add responseHeader.multiThreadedUsed element;

cpoerschke requested review from sigram, dsmiley, chatman and gus-asf July 26, 2024 10:46

github-actions bot added documentation Improvements or additions to documentation client:solrj cat:search labels Jul 26, 2024

cpoerschke mentioned this pull request Jul 26, 2024

SOLR-13350, SOLR-17298: multi-threaded search: don't (yet) support query limits #2590

Closed

cpoerschke marked this pull request as ready for review July 26, 2024 11:19

replace responseHeader.multiThreadedUsed element with INFO logging

380489a

Co-authored-by: Gus Heck <[email protected]>

cpoerschke changed the title ~~SOLR-13350, SOLR-17298: document multiThreaded=(true,false) parameter; add responseHeader.multiThreadedUsed element;~~ SOLR-13350, SOLR-17298: document multiThreaded=(true,false) parameter; log multiThreaded use; Jul 26, 2024

cpoerschke commented Jul 26, 2024

View reviewed changes

gus-asf mentioned this pull request Jul 26, 2024

SOLR-17298 - ThreadCpuTimer safe for multi-threaded search #2595

Merged

default multiThreaded to 'false'

6556546

cpoerschke changed the title ~~SOLR-13350, SOLR-17298: document multiThreaded=(true,false) parameter; log multiThreaded use;~~ SOLR-13350, SOLR-17298: use multiThreaded=false default; document multiThreaded parameter; log multiThreaded use; Jul 29, 2024

chatman approved these changes Jul 29, 2024

View reviewed changes

dsmiley reviewed Jul 29, 2024

View reviewed changes

cpoerschke added 2 commits July 30, 2024 14:32

undo SolrIndexSearcher and QueryCommand changes

6ea6a60

Merge remote-tracking branch 'origin/main' into SOLR-17298-document-m…

1b3b39d

…ultiThreaded

cpoerschke changed the title ~~SOLR-13350, SOLR-17298: use multiThreaded=false default; document multiThreaded parameter; log multiThreaded use;~~ SOLR-13350, SOLR-17298: use multiThreaded=false default; document multiThreaded parameter; Jul 30, 2024

cpoerschke added 3 commits July 30, 2024 14:47

draft solr/CHANGES.txt edit

1830993

revise common-query-parameters.adoc and link to configuring-solr-xml.…

39a8b82

…adoc#indexSearcherExecutorThreads

Update configuring-solr-xml.adoc

a10fdef

cpoerschke requested review from chatman and dsmiley July 30, 2024 16:28

gus-asf reviewed Aug 1, 2024

View reviewed changes

dsmiley reviewed Aug 2, 2024

View reviewed changes

cpoerschke requested a review from anshumg August 5, 2024 08:47

SOLR-17298 word-smith docs, note experimental status

fb9e33f

dsmiley reviewed Aug 5, 2024

View reviewed changes

solr/solr-ref-guide/modules/query-guide/pages/common-query-parameters.adoc Outdated Show resolved Hide resolved

Update common-query-parameters.adoc

5319a17

Update index search document punting of segmentsTerminateEarly to future release

dsmiley approved these changes Aug 6, 2024

View reviewed changes

gus-asf merged commit 7ebbcd3 into apache:main Aug 6, 2024
4 checks passed

asfgit pushed a commit that referenced this pull request Aug 14, 2024

SOLR-13350, SOLR-17298: use multiThreaded=false default; document mul…

84d1fbc

…tiThreaded parameter; (#2596) Co-authored-by: Gus Heck <[email protected]> (cherry picked from commit 7ebbcd3)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SOLR-13350, SOLR-17298: use multiThreaded=false default; document multiThreaded parameter; #2596

SOLR-13350, SOLR-17298: use multiThreaded=false default; document multiThreaded parameter; #2596

cpoerschke commented Jul 26, 2024

cpoerschke commented Jul 26, 2024

gus-asf commented Jul 26, 2024

cpoerschke Jul 26, 2024

chatman commented Jul 29, 2024

dsmiley Jul 29, 2024

dsmiley Jul 29, 2024

cpoerschke Jul 30, 2024

gus-asf left a comment

gus-asf Aug 1, 2024

gus-asf Aug 1, 2024

dsmiley Aug 2, 2024

gus-asf Aug 5, 2024

gus-asf Aug 1, 2024 •

edited

Loading

dsmiley Aug 2, 2024

dsmiley Aug 2, 2024

cpoerschke commented Aug 5, 2024

dsmiley left a comment

		If unset or set to `false`, then the multi-threaded search implementation will not be used for the query.
		If set to `true`, then use the multi-threaded search implementation if it is supported for the query.

SOLR-13350, SOLR-17298: use multiThreaded=false default; document multiThreaded parameter; #2596

SOLR-13350, SOLR-17298: use multiThreaded=false default; document multiThreaded parameter; #2596

Conversation

cpoerschke commented Jul 26, 2024

cpoerschke commented Jul 26, 2024

gus-asf commented Jul 26, 2024

Choose a reason for hiding this comment

chatman commented Jul 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gus-asf 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

gus-asf Aug 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpoerschke commented Aug 5, 2024

dsmiley left a comment

Choose a reason for hiding this comment

gus-asf Aug 1, 2024 •

edited

Loading