You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "gus-asf (via GitHub)" <gi...@apache.org> on 2024/07/26 00:18:56 UTC
[PR] SOLR-17298 - ThreadCpuTimer safe for multi-threaded search [solr]
gus-asf opened a new pull request, #2595:
URL: https://github.com/apache/solr/pull/2595
https://issues.apache.org/jira/browse/SOLR-17298
This introduces a version of ThreadCpuTimer class that does not need to depend on the request context or be instantiated to track timing. This approach turns ThreadCpuTimer into a "stopwatch" service that simply answers the question of how long has it been since the last time this thread asked for time... I added an override of beforeExecute() to MDCAwareThreadPoolExecutor and some related methods to ExecutorUtil to allow the "stopwatch" to be reset just before lucene asks the executor to execute a task. Every shouldExit() causes a read operation which returns the time since last read by that thread and resets the timer. An atomic long is used to accumulate the total and test the timeout.
Additionally the ThreadCpuTimer also can track multiple timings within the same thread by use of a "context" denoted by a string parameter, this allows it to continue to serve as a timer for cpu usage logging as well.
The Tests for this also required some tweaks to ensure that there were multiple segments in the index being searched, and enough segments were generated to ensure more than one task is assigned to the executor by Lucene (It groups segments up to a max total size or a maximum of 5 segments, whichever comes first, see [IndexSearcher#MAX_SEGMENTS_PER_SLICE](https://github.com/apache/lucene/blob/8d4f7a6e99d2da802b7019247b0f8f305d71c024/lucene/core/src/java/org/apache/lucene/search/IndexSearcher.java#L105), and also I added a TestInjection with code mimicking the ExpensiveSearchComponent code to allow us to waste some CPU before each call to shouldExit() for better simulation of actual load.
Tests pass including TestCpuAllowedLimit with both multiThreaded=true and multiThreaded=false cases enabled. We should however observe how it behaves in Jenkins/Crave test runs.
Also it would be super good to get more eyes on this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17298 - ThreadCpuTimer safe for multi-threaded search [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #2595:
URL: https://github.com/apache/solr/pull/2595#discussion_r1693362912
##########
solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java:
##########
@@ -1921,9 +1921,7 @@ public ScoreMode scoreMode() {
final TopDocs topDocs;
final ScoreMode scoreModeUsed;
if (!MultiThreadedSearcher.allowMT(pf.postFilter, cmd, query)) {
- if (log.isDebugEnabled()) {
- log.debug("skipping collector manager");
- }
+ log.debug("SINGLE THREADED search,skipping collector manager");
Review Comment:
IMO this is trace level
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17298 - ThreadCpuTimer safe for multi-threaded search [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #2595:
URL: https://github.com/apache/solr/pull/2595#discussion_r1693382984
##########
solr/solrj/src/java/org/apache/solr/common/util/ExecutorUtil.java:
##########
@@ -313,6 +340,37 @@ public MDCAwareThreadPoolExecutor(
RejectedExecutionHandler handler) {
super(corePoolSize, maximumPoolSize, keepAliveTime, unit, workQueue, handler);
this.enableSubmitterStackTrace = true;
+ this.beforeExecuteTask = NOOP;
+ }
+
+ public MDCAwareThreadPoolExecutor(
+ int corePoolSize,
+ int maximumPoolSize,
+ int keepAliveTime,
+ TimeUnit timeUnit,
+ BlockingQueue<Runnable> blockingQueue,
+ SolrNamedThreadFactory httpShardExecutor,
+ boolean enableSubmitterStackTrace) {
+ super(
+ corePoolSize, maximumPoolSize, keepAliveTime, timeUnit, blockingQueue, httpShardExecutor);
+ this.enableSubmitterStackTrace = enableSubmitterStackTrace;
+ this.beforeExecuteTask = NOOP;
+ }
+
+ public MDCAwareThreadPoolExecutor(
+ int i,
+ int maxValue,
+ long l,
+ TimeUnit timeUnit,
+ BlockingQueue<Runnable> es,
+ SolrNamedThreadFactory testExecutor,
+ boolean b) {
+ this(i, maxValue, l, timeUnit, es, testExecutor, b, NOOP);
+ }
+
+ @Override
+ protected void beforeExecute(Thread t, Runnable r) {
+ super.beforeExecute(t, this.beforeExecuteTask);
Review Comment:
I think you mean to actually invoke this.beforeExecuteTask.run()
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17298 - ThreadCpuTimer safe for multi-threaded search [solr]
Posted by "gus-asf (via GitHub)" <gi...@apache.org>.
gus-asf commented on code in PR #2595:
URL: https://github.com/apache/solr/pull/2595#discussion_r1693385066
##########
solr/solrj/src/java/org/apache/solr/common/util/ExecutorUtil.java:
##########
@@ -313,6 +340,37 @@ public MDCAwareThreadPoolExecutor(
RejectedExecutionHandler handler) {
super(corePoolSize, maximumPoolSize, keepAliveTime, unit, workQueue, handler);
this.enableSubmitterStackTrace = true;
+ this.beforeExecuteTask = NOOP;
+ }
+
+ public MDCAwareThreadPoolExecutor(
+ int corePoolSize,
+ int maximumPoolSize,
+ int keepAliveTime,
+ TimeUnit timeUnit,
+ BlockingQueue<Runnable> blockingQueue,
+ SolrNamedThreadFactory httpShardExecutor,
+ boolean enableSubmitterStackTrace) {
+ super(
+ corePoolSize, maximumPoolSize, keepAliveTime, timeUnit, blockingQueue, httpShardExecutor);
+ this.enableSubmitterStackTrace = enableSubmitterStackTrace;
+ this.beforeExecuteTask = NOOP;
+ }
+
+ public MDCAwareThreadPoolExecutor(
+ int i,
+ int maxValue,
+ long l,
+ TimeUnit timeUnit,
+ BlockingQueue<Runnable> es,
+ SolrNamedThreadFactory testExecutor,
+ boolean b) {
+ this(i, maxValue, l, timeUnit, es, testExecutor, b, NOOP);
+ }
+
+ @Override
+ protected void beforeExecute(Thread t, Runnable r) {
+ super.beforeExecute(t, this.beforeExecuteTask);
Review Comment:
quite right, that's an ide insertion I forgot to change. Good catch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17298 - ThreadCpuTimer safe for multi-threaded search [solr]
Posted by "gus-asf (via GitHub)" <gi...@apache.org>.
gus-asf merged PR #2595:
URL: https://github.com/apache/solr/pull/2595
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17298 - ThreadCpuTimer safe for multi-threaded search [solr]
Posted by "cpoerschke (via GitHub)" <gi...@apache.org>.
cpoerschke commented on code in PR #2595:
URL: https://github.com/apache/solr/pull/2595#discussion_r1692900328
##########
solr/core/src/test/org/apache/solr/search/TestCpuAllowedLimit.java:
##########
@@ -61,7 +65,12 @@ private static Path createConfigSet() throws Exception {
}
@BeforeClass
- public static void setup() throws Exception {
+ public static void setupClass() throws Exception {
+ // Using NoMergePolicy and 100 commits we should get 100 segments (across all shards).
+ // At this point of writing MAX_SEGMENTS_PER_SLICE in lucene is 5, so we should be
Review Comment:
(not yet merged) https://github.com/apache/lucene/pull/13508 includes making Lucene's `IndexSearcher.MAX_SEGMENTS_PER_SLICE` public.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17298 - ThreadCpuTimer safe for multi-threaded search [solr]
Posted by "gus-asf (via GitHub)" <gi...@apache.org>.
gus-asf commented on PR #2595:
URL: https://github.com/apache/solr/pull/2595#issuecomment-2256114568
Review feedback addressed, Thinking of committing to main tomorrow if no further feedback. Backport to 9x intended a few days after that assuming that goes well...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17298 - ThreadCpuTimer safe for multi-threaded search [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #2595:
URL: https://github.com/apache/solr/pull/2595#discussion_r1693381153
##########
solr/solrj/src/java/org/apache/solr/common/util/ExecutorUtil.java:
##########
@@ -313,6 +340,37 @@ public MDCAwareThreadPoolExecutor(
RejectedExecutionHandler handler) {
super(corePoolSize, maximumPoolSize, keepAliveTime, unit, workQueue, handler);
this.enableSubmitterStackTrace = true;
+ this.beforeExecuteTask = NOOP;
+ }
+
+ public MDCAwareThreadPoolExecutor(
+ int corePoolSize,
+ int maximumPoolSize,
+ int keepAliveTime,
+ TimeUnit timeUnit,
+ BlockingQueue<Runnable> blockingQueue,
+ SolrNamedThreadFactory httpShardExecutor,
+ boolean enableSubmitterStackTrace) {
+ super(
+ corePoolSize, maximumPoolSize, keepAliveTime, timeUnit, blockingQueue, httpShardExecutor);
+ this.enableSubmitterStackTrace = enableSubmitterStackTrace;
+ this.beforeExecuteTask = NOOP;
+ }
+
+ public MDCAwareThreadPoolExecutor(
+ int i,
+ int maxValue,
+ long l,
+ TimeUnit timeUnit,
+ BlockingQueue<Runnable> es,
+ SolrNamedThreadFactory testExecutor,
+ boolean b) {
+ this(i, maxValue, l, timeUnit, es, testExecutor, b, NOOP);
+ }
+
+ @Override
+ protected void beforeExecute(Thread t, Runnable r) {
+ super.beforeExecute(t, this.beforeExecuteTask);
Review Comment:
super.beforeExecute doesn't do anything; it's a callback -- something the underlying ThreadPoolExecutor will call, not something we should call..
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17298 - ThreadCpuTimer safe for multi-threaded search [solr]
Posted by "gus-asf (via GitHub)" <gi...@apache.org>.
gus-asf commented on code in PR #2595:
URL: https://github.com/apache/solr/pull/2595#discussion_r1693382945
##########
solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java:
##########
@@ -1921,9 +1921,7 @@ public ScoreMode scoreMode() {
final TopDocs topDocs;
final ScoreMode scoreModeUsed;
if (!MultiThreadedSearcher.allowMT(pf.postFilter, cmd, query)) {
- if (log.isDebugEnabled()) {
- log.debug("skipping collector manager");
- }
+ log.debug("SINGLE THREADED search,skipping collector manager");
Review Comment:
I'm fine with whatever level, but see also https://github.com/apache/solr/pull/2596#pullrequestreview-2201721099
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17298 - ThreadCpuTimer safe for multi-threaded search [solr]
Posted by "gus-asf (via GitHub)" <gi...@apache.org>.
gus-asf commented on code in PR #2595:
URL: https://github.com/apache/solr/pull/2595#discussion_r1692984958
##########
solr/core/src/test/org/apache/solr/search/TestCpuAllowedLimit.java:
##########
@@ -61,7 +65,12 @@ private static Path createConfigSet() throws Exception {
}
@BeforeClass
- public static void setup() throws Exception {
+ public static void setupClass() throws Exception {
+ // Using NoMergePolicy and 100 commits we should get 100 segments (across all shards).
+ // At this point of writing MAX_SEGMENTS_PER_SLICE in lucene is 5, so we should be
Review Comment:
> (not yet merged) [apache/lucene#13508](https://github.com/apache/lucene/pull/13508) includes making Lucene's `IndexSearcher.MAX_SEGMENTS_PER_SLICE` public.
I see, though still final. Until such time as we override [IndexSearcher#slices(List<LeafReaderContext>)](https://github.com/apache/lucene/blob/7b421d75c9ee829c9745f0f7ad8f6282535cbe5d/lucene/core/src/java/org/apache/lucene/search/IndexSearcher.java#L324) this is still a constant.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17298 - ThreadCpuTimer safe for multi-threaded search [solr]
Posted by "cpoerschke (via GitHub)" <gi...@apache.org>.
cpoerschke commented on code in PR #2595:
URL: https://github.com/apache/solr/pull/2595#discussion_r1692996096
##########
solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java:
##########
@@ -1921,9 +1921,7 @@ public ScoreMode scoreMode() {
final TopDocs topDocs;
final ScoreMode scoreModeUsed;
if (!MultiThreadedSearcher.allowMT(pf.postFilter, cmd, query)) {
- if (log.isDebugEnabled()) {
- log.debug("skipping collector manager");
- }
+ log.debug("SINGLE THREADED search,skipping collector manager");
Review Comment:
minor: `getDocListAndSetNC` could benefit from similar logging.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org