dsmiley commented on code in PR #2237:
URL: https://github.com/apache/solr/pull/2237#discussion_r1484948048
##########
solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java:
##########
@@ -972,7 +978,8 @@ private DocSet getAndCacheDocSet(Query query) throws
IOException {
}
DocSet answer;
- if (SolrQueryTimeoutImpl.getInstance().isTimeoutEnabled()) {
+ SolrRequestInfo requestInfo = SolrRequestInfo.getRequestInfo();
+ if (requestInfo != null && requestInfo.getLimits().isTimeoutEnabled()) {
Review Comment:
The previous pattern had the nice characteristic that it's a one-liner. We
could put the use of SolrRequestInfo on a limits class convenience method? Not
a big deal anyway.
##########
solr/core/src/java/org/apache/solr/request/SolrRequestInfo.java:
##########
@@ -75,6 +77,9 @@ public static void setRequestInfo(SolrRequestInfo info) {
} else if (stack.size() > MAX_STACK_SIZE) {
assert false : "SolrRequestInfo Stack is full";
log.error("SolrRequestInfo Stack is full");
+ } else if (!stack.isEmpty() && info.req != null) {
+ // if req is null limits will be an empty instance with no limits anyway.
Review Comment:
agreed
##########
solr/core/src/java/org/apache/solr/search/QueryLimits.java:
##########
@@ -0,0 +1,90 @@
+/*
+ * 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 static org.apache.solr.search.SolrQueryTimeLimit.hasTimeLimit;
+
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.lucene.index.QueryTimeout;
+import org.apache.solr.request.SolrQueryRequest;
+
+/**
+ * Represents the limitations on the query. These limits might be wall clock
time, cpu time, memory,
+ * or other resource limits. Exceeding any specified limit will cause {@link
#shouldExit()} to
+ * return true the next time it is checked (it may be checked in either Lucene
code or Solr code)
+ */
+public class QueryLimits implements QueryTimeout {
+ private final List<QueryTimeout> limits =
+ new ArrayList<>(3); // timeAllowed, cpu, and memory anticipated
+
+ public static QueryLimits NONE = new QueryLimits();
+
+ private QueryLimits() {}
+
+ /**
+ * Implementors of a Query Limit should add an if block here to activate it,
and typically this if
+ * statement will hinge on hasXXXLimit() static method attached to the
implementation class.
+ *
+ * @param req the current SolrQueryRequest.
+ */
+ public QueryLimits(SolrQueryRequest req) {
+ if (hasTimeLimit(req)) {
+ limits.add(new SolrQueryTimeLimit(req));
+ }
+ }
+
+ @Override
+ public boolean shouldExit() {
+ return limits.stream().anyMatch(QueryTimeout::shouldExit);
Review Comment:
I hate to suggest this, but can you use a standard loop? This might be
called by low level performance sensitive operations. If you know this here to
be pretty quick then fine I guess.
##########
solr/core/src/java/org/apache/solr/search/QueryLimits.java:
##########
@@ -0,0 +1,90 @@
+/*
+ * 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 static org.apache.solr.search.SolrQueryTimeLimit.hasTimeLimit;
+
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.lucene.index.QueryTimeout;
+import org.apache.solr.request.SolrQueryRequest;
+
+/**
+ * Represents the limitations on the query. These limits might be wall clock
time, cpu time, memory,
+ * or other resource limits. Exceeding any specified limit will cause {@link
#shouldExit()} to
+ * return true the next time it is checked (it may be checked in either Lucene
code or Solr code)
+ */
+public class QueryLimits implements QueryTimeout {
+ private final List<QueryTimeout> limits =
Review Comment:
Where do more limits get added? I only see the time one.
##########
solr/core/src/java/org/apache/solr/request/SolrRequestInfo.java:
##########
@@ -210,6 +216,32 @@ public void addCloseHook(Closeable hook) {
}
}
+ /**
+ * This call creates the QueryLimits object and any required implementations
of {@link
+ * org.apache.lucene.index.QueryTimeout}. Any code before this call will not
be subject to the
+ * limitations set on the request. Note that calling {@link #getLimits()}
has the same effect as
+ * this method.
+ *
+ * @see #getLimits()
+ */
+ public void initQueryLimits() {
Review Comment:
why public; seems internal
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
