-
Notifications
You must be signed in to change notification settings - Fork 690
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
SOLR-17141: Create CpuQueryLimit implementation #2244
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this. Seems useful reporting or API uses cases but maybe not for a SERP with a tight timeAllowed.
solr/core/src/java/org/apache/solr/search/CpuQueryTimeLimit.java
Outdated
Show resolved
Hide resolved
solr/core/src/java/org/apache/solr/search/CpuQueryTimeLimit.java
Outdated
Show resolved
Hide resolved
solr/core/src/test-files/solr/configsets/query-limits/conf/schema.xml
Outdated
Show resolved
Hide resolved
import org.apache.solr.handler.component.ResponseBuilder; | ||
import org.apache.solr.handler.component.SearchComponent; | ||
|
||
public class ExpensiveSearchComponent extends SearchComponent { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking forward to seeing this on https://solr.cool/ :-)
solr/core/src/test/org/apache/solr/search/TestCpuQueryTimeLimit.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the logging and limiting features are too tightly coupled still... It should be possible to set a timeout without logging the cpu time or vice-a-versa
solr/core/src/java/org/apache/solr/request/SolrRequestInfo.java
Outdated
Show resolved
Hide resolved
solr/core/src/java/org/apache/solr/search/CpuQueryTimeLimit.java
Outdated
Show resolved
Hide resolved
solr/core/src/java/org/apache/solr/handler/RequestHandlerBase.java
Outdated
Show resolved
Hide resolved
solr/core/src/java/org/apache/solr/handler/RequestHandlerBase.java
Outdated
Show resolved
Hide resolved
@@ -622,8 +622,8 @@ public void handleRequestBody(SolrQueryRequest req, SolrQueryResponse rsp) throw | |||
} while (nextStage != Integer.MAX_VALUE); | |||
|
|||
if (publishCpuTime) { | |||
rsp.getResponseHeader().add(ThreadStats.CPU_TIME, totalShardCpuTime); | |||
rsp.addToLog(ThreadStats.CPU_TIME, totalShardCpuTime); | |||
rsp.getResponseHeader().add(ThreadCpuTime.CPU_TIME, totalShardCpuTime); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No null check here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean for the responseHeader?
* Improve the ExpensiveSearchComponent to produce consistent CPU load * Use SolrJettyTestRule instead of full mini cloud cluster.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
Clean up and document the ExpensiveSearchComponent. Support adding load at different stages of distrib processing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good to me; I just have one small comment.
public static void setup() throws Exception { | ||
System.setProperty(ThreadCpuTimer.ENABLE_CPU_TIME, "true"); | ||
Path configset = createConfigSet(); | ||
configureCluster(2).addConfig("conf", configset).configure(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we actually need 2 nodes to test this? (Trying to keep tests as lean as needed to test what they need to test; no more)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably not at the moment... My thinking was that if/when we want to add more complex tests that trigger termination at various stages in the distrib processing then it's best to also include inter-node communication in the setup. We're not there yet so I can reduce it to 1 node.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've found it to be very rare where we actually need more than one node to test functionality.
* Refactor to fix ThreadStats / ThreadCpuTime nesting and use it in CpuQueryTimeLimit. * Rename classes to better reflect the type of limit.
* SOLR-17141: Create CpuQueryLimit implementation (#2244) * Refactor to fix ThreadStats / ThreadCpuTimer nesting and use it in CpuQueryTimeLimit. * Rename classes to better reflect the type of limit.
Details in Jira. This builds on top of changes in #2237.