dsmiley commented on code in PR #2244:
URL: https://github.com/apache/solr/pull/2244#discussion_r1488505233
##########
solr/core/src/java/org/apache/solr/search/QueryLimits.java:
##########
@@ -41,11 +43,15 @@ private QueryLimits() {}
* statement will hinge on hasXXXLimit() static method attached to the
implementation class.
*
* @param req the current SolrQueryRequest.
+ * @param threadCpuTime current thread CPU time monitor.
*/
- public QueryLimits(SolrQueryRequest req) {
+ public QueryLimits(SolrQueryRequest req, ThreadCpuTime threadCpuTime) {
if (hasTimeLimit(req)) {
limits.add(new SolrQueryTimeLimit(req));
}
+ if (hasCpuLimit(req)) {
+ limits.add(new CpuQueryTimeLimit(req, threadCpuTime));
Review Comment:
Still sad to see "Query" embedded in these names but okay
##########
solr/core/src/java/org/apache/solr/search/CpuQueryTimeLimit.java:
##########
@@ -0,0 +1,72 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.search;
+
+import com.google.common.annotations.VisibleForTesting;
+import java.lang.invoke.MethodHandles;
+import java.util.concurrent.TimeUnit;
+import org.apache.lucene.index.QueryTimeout;
+import org.apache.solr.common.params.CommonParams;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.util.ThreadCpuTime;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class CpuQueryTimeLimit implements QueryTimeout {
+ private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ private final long limitAt;
Review Comment:
It's not evident this is in nanoseconds; incorporate as a name suffix.
##########
solr/core/src/test/org/apache/solr/search/TestCpuQueryTimeLimit.java:
##########
@@ -0,0 +1,126 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.search;
+
+import java.lang.invoke.MethodHandles;
+import java.util.concurrent.TimeUnit;
+import org.apache.solr.client.solrj.SolrClient;
+import org.apache.solr.client.solrj.request.CollectionAdminRequest;
+import org.apache.solr.client.solrj.response.QueryResponse;
+import org.apache.solr.cloud.CloudUtil;
+import org.apache.solr.cloud.MiniSolrCloudCluster;
+import org.apache.solr.cloud.SolrCloudTestCase;
+import org.apache.solr.util.ThreadCpuTime;
+import org.junit.Assume;
+import org.junit.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class TestCpuQueryTimeLimit extends SolrCloudTestCase {
Review Comment:
Is this really SolrCloud related? Notice how TestCpuTimeSearch does some
distributed search yet doesn't need SolrCloud to do it. I'm trying to keep our
tests leaner.
##########
solr/core/src/java/org/apache/solr/search/CpuQueryTimeLimit.java:
##########
@@ -0,0 +1,72 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.search;
+
+import com.google.common.annotations.VisibleForTesting;
+import java.lang.invoke.MethodHandles;
+import java.util.concurrent.TimeUnit;
+import org.apache.lucene.index.QueryTimeout;
+import org.apache.solr.common.params.CommonParams;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.util.ThreadCpuTime;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class CpuQueryTimeLimit implements QueryTimeout {
+ private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ private final long limitAt;
+ private final ThreadCpuTime threadCpuTime;
+
+ public CpuQueryTimeLimit(SolrQueryRequest req, ThreadCpuTime threadCpuTime) {
+ if (!ThreadCpuTime.isSupported()) {
+ throw new IllegalArgumentException("Thread CPU time monitoring is not
available.");
+ }
+ if (threadCpuTime == null) {
+ throw new IllegalArgumentException("Thread CPU time monitor must not be
null.");
+ }
+ this.threadCpuTime = threadCpuTime;
Review Comment:
Try java.util.Objects#requireNonNull(T, java.lang.String) which is designed
for this and is succinct
##########
solr/core/src/test-files/solr/configsets/query-limits/conf/schema.xml:
##########
Review Comment:
Please don't add more test configs unless we truly need it! See
`org.apache.solr.SolrTestCaseJ4#copyMinConf(java.io.File)` and
`HideStackTraceTest` for an example of how to use it.
##########
solr/core/src/test/org/apache/solr/search/ExpensiveSearchComponent.java:
##########
@@ -0,0 +1,58 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.search;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import org.apache.lucene.tests.util.LuceneTestCase;
+import org.apache.lucene.tests.util.TestUtil;
+import org.apache.solr.handler.component.ResponseBuilder;
+import org.apache.solr.handler.component.SearchComponent;
+
+public class ExpensiveSearchComponent extends SearchComponent {
Review Comment:
Looking forward to seeing this on https://solr.cool/ :-)
--
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-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
