dsmiley commented on code in PR #5238:
URL: https://github.com/apache/solr/pull/5238#discussion_r4455633867
##########
solr/core/src/java/org/apache/solr/core/SolrCore.java:
##########
@@ -3007,6 +3007,10 @@ public static void postDecorateResponse(
responseHeader.add("params", new SimpleOrderedMap<>(req.getParams()));
}
}
+ CoreContainer cc = req.getCoreContainer();
+ if (cc != null) {
+ cc.searchRequestDispatcher.afterResponse(req, rsp, qtime);
Review Comment:
qtime should be on `rsp`; I think it's inelegant to pass this 3rd param.
##########
solr/core/src/java/org/apache/solr/handler/RequestHandlerBase.java:
##########
@@ -67,6 +67,7 @@ public abstract class RequestHandlerBase
PermissionNameProvider {
public static final String REQUEST_CPU_TIMER_CONTEXT = "publishCpuTime";
+ private static final String DISPATCHED_HANDLER = "dispatchedHandler";
Review Comment:
Is this mechanism you're adding here redundant with
SolrRequestInfo.isNestedRequest()?
##########
solr/core/src/java/org/apache/solr/core/CoreContainer.java:
##########
@@ -212,6 +214,27 @@ public JerseyAppHandlerCache getJerseyAppHandlerCache() {
return appHandlersByConfigSetId;
}
+ final SearchRequestDispatcher searchRequestDispatcher = new
SearchRequestDispatcher();
Review Comment:
I'd rather we have one getter for this instead of several methods. The
surface area of CoreContainer has gotten out of control.
##########
solr/core/src/java/org/apache/solr/handler/component/SearchRequestDispatcher.java:
##########
@@ -0,0 +1,234 @@
+/*
+ * 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.handler.component;
+
+import java.io.Closeable;
+import java.lang.invoke.MethodHandles;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.concurrent.ArrayBlockingQueue;
+import java.util.concurrent.BlockingQueue;
+import java.util.concurrent.CopyOnWriteArrayList;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLong;
+import java.util.concurrent.atomic.LongAdder;
+import org.apache.solr.common.util.EnvUtils;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.response.SolrQueryResponse;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/** Holds the {@link SearchRequestListener}s of a node and delivers each
reported search to them. */
+public class SearchRequestDispatcher implements Closeable {
Review Comment:
The name seems to not match what it does. Why not simply
SearchRequestListenerManager?
##########
solr/core/src/java/org/apache/solr/handler/component/SearchRequestInfo.java:
##########
@@ -0,0 +1,103 @@
+/*
+ * 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.handler.component;
+
+import java.security.Principal;
+import java.util.List;
+import org.apache.solr.logging.MDCLoggingContext;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.response.SolrQueryResponse;
+import org.apache.solr.util.ThreadCpuTimer;
+
+/**
+ * A completed search request, as passed to {@link SearchRequestListener}. It
holds only values
+ * copied from the request, so it can be kept and read on any thread.
+ *
+ * @param handlerPath the path the request log uses
+ * @param collection the collection of {@code core}, even when an alias or
several collections were
+ * searched; null in standalone mode
+ * @param core the name of the core that coordinated the search
+ * @param user the authenticated user, or null
+ * @param traceId the trace id, or null
+ * @param startTimeMs when the request started, in epoch milliseconds
+ * @param qTimeMs equal to the QTime in the response header
+ * @param cpuTimeMs null unless {@code solr.log.cputime=true}; includes the
CPU shards reported when
+ * distributed, which can undercount shard work still finishing when the
response is built
+ * @param hits as in the request log; null when it has none, such as for
distributed grouping or a
+ * search stopped by a query limit
+ * @param shardCount number of shards targeted; 0 for a non-distributed request
+ * @param failureType the class name of the exception set on the response, or
null
+ * @param failureMessage its message, cut to {@link
#MAX_FAILURE_MESSAGE_CHARS}, or null
+ * @param partialResults whether the response header marks the results as
partial, for example after
+ * a timeout
+ * @lucene.experimental
+ */
+public record SearchRequestInfo(
+ String handlerPath,
+ String collection,
+ String core,
+ String user,
+ String traceId,
+ long startTimeMs,
+ long qTimeMs,
+ Long cpuTimeMs,
+ Long hits,
+ int shardCount,
+ String failureType,
+ String failureMessage,
+ boolean partialResults) {
+
+ public static final int MAX_FAILURE_MESSAGE_CHARS = 256;
+
+ static SearchRequestInfo from(
+ SolrQueryRequest req, SolrQueryResponse rsp, ResponseBuilder rb, long
qTimeMs) {
+ boolean distributed = rb.isDistributed() && rb.shards != null;
+ Principal user = req.getUserPrincipal();
+ Exception failure = rsp.getException();
+ return new SearchRequestInfo(
+ rsp.getToLog().get("path") instanceof String path ? path : null,
+ req.getCore().getCoreDescriptor().getCollectionName(),
+ req.getCore().getName(),
+ user == null ? null : user.getName(),
+ MDCLoggingContext.getTraceId(),
+ req.getStartTime(),
+ qTimeMs,
+ cpuTimeMs(rsp),
+ rsp.getToLog().get("hits") instanceof Number hits ? hits.longValue() :
null,
+ distributed ? rb.shards.length : 0,
+ failure == null ? null : failure.getClass().getName(),
+ failure == null ? null : truncate(failure.getMessage()),
+ SolrQueryResponse.isPartialResults(rsp.getResponseHeader()));
+ }
+
+ private static String truncate(String message) {
+ return message == null || message.length() <= MAX_FAILURE_MESSAGE_CHARS
+ ? message
+ : message.substring(0, MAX_FAILURE_MESSAGE_CHARS);
+ }
+
+ private static Long cpuTimeMs(SolrQueryResponse rsp) {
Review Comment:
let's move to `rsp`
##########
solr/core/src/java/org/apache/solr/handler/component/SearchRequestInfo.java:
##########
@@ -0,0 +1,103 @@
+/*
+ * 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.handler.component;
+
+import java.security.Principal;
+import java.util.List;
+import org.apache.solr.logging.MDCLoggingContext;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.response.SolrQueryResponse;
+import org.apache.solr.util.ThreadCpuTimer;
+
+/**
+ * A completed search request, as passed to {@link SearchRequestListener}. It
holds only values
+ * copied from the request, so it can be kept and read on any thread.
+ *
+ * @param handlerPath the path the request log uses
+ * @param collection the collection of {@code core}, even when an alias or
several collections were
+ * searched; null in standalone mode
+ * @param core the name of the core that coordinated the search
+ * @param user the authenticated user, or null
+ * @param traceId the trace id, or null
+ * @param startTimeMs when the request started, in epoch milliseconds
+ * @param qTimeMs equal to the QTime in the response header
+ * @param cpuTimeMs null unless {@code solr.log.cputime=true}; includes the
CPU shards reported when
+ * distributed, which can undercount shard work still finishing when the
response is built
+ * @param hits as in the request log; null when it has none, such as for
distributed grouping or a
+ * search stopped by a query limit
+ * @param shardCount number of shards targeted; 0 for a non-distributed request
+ * @param failureType the class name of the exception set on the response, or
null
+ * @param failureMessage its message, cut to {@link
#MAX_FAILURE_MESSAGE_CHARS}, or null
+ * @param partialResults whether the response header marks the results as
partial, for example after
+ * a timeout
+ * @lucene.experimental
+ */
+public record SearchRequestInfo(
+ String handlerPath,
+ String collection,
+ String core,
+ String user,
+ String traceId,
+ long startTimeMs,
+ long qTimeMs,
+ Long cpuTimeMs,
+ Long hits,
+ int shardCount,
+ String failureType,
+ String failureMessage,
+ boolean partialResults) {
+
+ public static final int MAX_FAILURE_MESSAGE_CHARS = 256;
+
+ static SearchRequestInfo from(
+ SolrQueryRequest req, SolrQueryResponse rsp, ResponseBuilder rb, long
qTimeMs) {
+ boolean distributed = rb.isDistributed() && rb.shards != null;
+ Principal user = req.getUserPrincipal();
+ Exception failure = rsp.getException();
+ return new SearchRequestInfo(
+ rsp.getToLog().get("path") instanceof String path ? path : null,
+ req.getCore().getCoreDescriptor().getCollectionName(),
+ req.getCore().getName(),
+ user == null ? null : user.getName(),
+ MDCLoggingContext.getTraceId(),
+ req.getStartTime(),
+ qTimeMs,
+ cpuTimeMs(rsp),
+ rsp.getToLog().get("hits") instanceof Number hits ? hits.longValue() :
null,
+ distributed ? rb.shards.length : 0,
+ failure == null ? null : failure.getClass().getName(),
+ failure == null ? null : truncate(failure.getMessage()),
+ SolrQueryResponse.isPartialResults(rsp.getResponseHeader()));
Review Comment:
let's move to instance method on `rsp`
##########
solr/core/src/java/org/apache/solr/handler/component/SearchRequestInfo.java:
##########
@@ -0,0 +1,103 @@
+/*
+ * 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.handler.component;
+
+import java.security.Principal;
+import java.util.List;
+import org.apache.solr.logging.MDCLoggingContext;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.response.SolrQueryResponse;
+import org.apache.solr.util.ThreadCpuTimer;
+
+/**
+ * A completed search request, as passed to {@link SearchRequestListener}. It
holds only values
+ * copied from the request, so it can be kept and read on any thread.
+ *
+ * @param handlerPath the path the request log uses
+ * @param collection the collection of {@code core}, even when an alias or
several collections were
+ * searched; null in standalone mode
+ * @param core the name of the core that coordinated the search
+ * @param user the authenticated user, or null
+ * @param traceId the trace id, or null
+ * @param startTimeMs when the request started, in epoch milliseconds
+ * @param qTimeMs equal to the QTime in the response header
+ * @param cpuTimeMs null unless {@code solr.log.cputime=true}; includes the
CPU shards reported when
+ * distributed, which can undercount shard work still finishing when the
response is built
+ * @param hits as in the request log; null when it has none, such as for
distributed grouping or a
+ * search stopped by a query limit
+ * @param shardCount number of shards targeted; 0 for a non-distributed request
+ * @param failureType the class name of the exception set on the response, or
null
+ * @param failureMessage its message, cut to {@link
#MAX_FAILURE_MESSAGE_CHARS}, or null
+ * @param partialResults whether the response header marks the results as
partial, for example after
+ * a timeout
+ * @lucene.experimental
+ */
+public record SearchRequestInfo(
+ String handlerPath,
+ String collection,
+ String core,
+ String user,
+ String traceId,
+ long startTimeMs,
+ long qTimeMs,
+ Long cpuTimeMs,
+ Long hits,
+ int shardCount,
+ String failureType,
+ String failureMessage,
+ boolean partialResults) {
+
+ public static final int MAX_FAILURE_MESSAGE_CHARS = 256;
+
+ static SearchRequestInfo from(
+ SolrQueryRequest req, SolrQueryResponse rsp, ResponseBuilder rb, long
qTimeMs) {
+ boolean distributed = rb.isDistributed() && rb.shards != null;
+ Principal user = req.getUserPrincipal();
+ Exception failure = rsp.getException();
+ return new SearchRequestInfo(
+ rsp.getToLog().get("path") instanceof String path ? path : null,
Review Comment:
deserves to be a getter on `req`
##########
solr/core/src/java/org/apache/solr/handler/component/SearchRequestInfo.java:
##########
@@ -0,0 +1,103 @@
+/*
+ * 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.handler.component;
+
+import java.security.Principal;
+import java.util.List;
+import org.apache.solr.logging.MDCLoggingContext;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.response.SolrQueryResponse;
+import org.apache.solr.util.ThreadCpuTimer;
+
+/**
+ * A completed search request, as passed to {@link SearchRequestListener}. It
holds only values
+ * copied from the request, so it can be kept and read on any thread.
+ *
+ * @param handlerPath the path the request log uses
+ * @param collection the collection of {@code core}, even when an alias or
several collections were
+ * searched; null in standalone mode
+ * @param core the name of the core that coordinated the search
+ * @param user the authenticated user, or null
+ * @param traceId the trace id, or null
+ * @param startTimeMs when the request started, in epoch milliseconds
+ * @param qTimeMs equal to the QTime in the response header
+ * @param cpuTimeMs null unless {@code solr.log.cputime=true}; includes the
CPU shards reported when
+ * distributed, which can undercount shard work still finishing when the
response is built
+ * @param hits as in the request log; null when it has none, such as for
distributed grouping or a
+ * search stopped by a query limit
+ * @param shardCount number of shards targeted; 0 for a non-distributed request
+ * @param failureType the class name of the exception set on the response, or
null
+ * @param failureMessage its message, cut to {@link
#MAX_FAILURE_MESSAGE_CHARS}, or null
+ * @param partialResults whether the response header marks the results as
partial, for example after
+ * a timeout
+ * @lucene.experimental
+ */
+public record SearchRequestInfo(
+ String handlerPath,
+ String collection,
+ String core,
+ String user,
+ String traceId,
+ long startTimeMs,
+ long qTimeMs,
+ Long cpuTimeMs,
+ Long hits,
+ int shardCount,
+ String failureType,
+ String failureMessage,
+ boolean partialResults) {
+
+ public static final int MAX_FAILURE_MESSAGE_CHARS = 256;
+
+ static SearchRequestInfo from(
+ SolrQueryRequest req, SolrQueryResponse rsp, ResponseBuilder rb, long
qTimeMs) {
+ boolean distributed = rb.isDistributed() && rb.shards != null;
+ Principal user = req.getUserPrincipal();
+ Exception failure = rsp.getException();
+ return new SearchRequestInfo(
+ rsp.getToLog().get("path") instanceof String path ? path : null,
+ req.getCore().getCoreDescriptor().getCollectionName(),
+ req.getCore().getName(),
+ user == null ? null : user.getName(),
+ MDCLoggingContext.getTraceId(),
+ req.getStartTime(),
+ qTimeMs,
+ cpuTimeMs(rsp),
+ rsp.getToLog().get("hits") instanceof Number hits ? hits.longValue() :
null,
+ distributed ? rb.shards.length : 0,
Review Comment:
let's move to instance method on `rsp`
--
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]
