Problem with "CPU time" name is that multithreaded search (SOLR-13350) will need to do justice to this nomenclature by calculating CPU time from all the threads, and that would be non trivial.
On Thu, 21 Dec, 2023, 11:28 pm dsmiley (via GitHub), <g...@apache.org> wrote: > > dsmiley commented on code in PR #2118: > URL: https://github.com/apache/solr/pull/2118#discussion_r1434358593 > > > ########## > solr/core/src/java/org/apache/solr/util/ThreadStats.java: > ########## > @@ -0,0 +1,94 @@ > +/* > + * 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.util; > + > +import java.lang.invoke.MethodHandles; > +import java.lang.management.ManagementFactory; > +import java.lang.management.ThreadMXBean; > +import java.util.Optional; > +import java.util.concurrent.TimeUnit; > +import org.slf4j.Logger; > +import org.slf4j.LoggerFactory; > + > +/** > + * Allows tracking information about the current thread using the JVM's > built-in management bean > + * {@link java.lang.management.ThreadMXBean}. > + * > + * <p>Calling code should create an instance of this class when starting > the operation, and then can > + * get the {@link #getCpuTimeMs()} at any time thereafter. > + */ > +public class ThreadStats { > + private static final long UNSUPPORTED = -1; > + public static final String SHARDS_CPU_TIME = "cpuTime"; > + public static final String LOCAL_CPU_TIME = "localCpuTime"; > + public static final String ENABLE_CPU_TIME = "solr.enableCpuTime"; > + > + private static ThreadMXBean THREAD_MX_BEAN; > + private static final Logger log = > LoggerFactory.getLogger(MethodHandles.lookup().lookupClass()); > + > + static { > + try { > + java.lang.management.ThreadMXBean threadBean = > ManagementFactory.getThreadMXBean(); > > Review Comment: > hasn't changed > > > > ########## > solr/core/src/java/org/apache/solr/handler/RequestHandlerBase.java: > ########## > @@ -212,6 +216,14 @@ public abstract void > handleRequestBody(SolrQueryRequest req, SolrQueryResponse r > > @Override > public void handleRequest(SolrQueryRequest req, SolrQueryResponse rsp) { > + ThreadStats cpuStats = null; > + if (publishCpuTime == null) { > + publishCpuTime = Boolean.getBoolean(ThreadStats.ENABLE_CPU_TIME); > + } > > Review Comment: > Can't this be added to the field's declaration instead of here? And > would then be a primitive `boolean` > > > > ########## > solr/core/src/test/org/apache/solr/CpuTimeTestCase.java: > ########## > @@ -0,0 +1,95 @@ > +/* > + * 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; > + > +import java.util.List; > +import org.apache.solr.client.solrj.SolrQuery; > +import org.apache.solr.client.solrj.impl.CloudSolrClient; > +import org.apache.solr.client.solrj.request.CollectionAdminRequest; > +import org.apache.solr.client.solrj.request.UpdateRequest; > +import org.apache.solr.client.solrj.response.QueryResponse; > +import org.apache.solr.cloud.SolrCloudTestCase; > +import org.apache.solr.common.SolrInputDocument; > +import org.apache.solr.common.util.NamedList; > +import org.apache.solr.util.ThreadStats; > +import org.junit.BeforeClass; > +import org.junit.Test; > + > +/** Test that cpuTime is generated a */ > +public class CpuTimeTestCase extends SolrCloudTestCase { > > Review Comment: > We should avoid SolrCloudTestCase if the feature in question isn't > actually related to SolrCloud. Distributed search works without it and was > implemented before SolrCloud. A SolrCloud test will always be more > heavyweight. > > Consider looking at SolrJettyTestRule, a new test mechanism to manage a > Solr/Jetty instance with test lifecycle. > > > > ########## > solr/core/src/java/org/apache/solr/handler/RequestHandlerBase.java: > ########## > @@ -212,6 +216,14 @@ public abstract void > handleRequestBody(SolrQueryRequest req, SolrQueryResponse r > > @Override > public void handleRequest(SolrQueryRequest req, SolrQueryResponse rsp) { > + ThreadStats cpuStats = null; > + if (publishCpuTime == null) { > + publishCpuTime = Boolean.getBoolean(ThreadStats.ENABLE_CPU_TIME); > + } > > Review Comment: > If it's then protected, could be accessed by SearchHandler, removing > the field there. > > > > ########## > solr/core/src/java/org/apache/solr/handler/component/SearchHandler.java: > ########## > @@ -599,6 +602,28 @@ public void handleRequestBody(SolrQueryRequest req, > SolrQueryResponse rsp) throw > for (SearchComponent c : components) { > c.handleResponses(rb, srsp.getShardRequest()); > } > + > + // Compute total CpuTime used by all shards. > + List<ShardResponse> responses = > srsp.getShardRequest().responses; > + for (ShardResponse response : responses) { > + if ((response.getSolrResponse() != null) > + && (response.getSolrResponse().getResponse() != null) > + && > (response.getSolrResponse().getResponse().get("responseHeader") != null)) { > + @SuppressWarnings("unchecked") > + SimpleOrderedMap<Object> header = > + (SimpleOrderedMap<Object>) > + response > + .getSolrResponse() > + .getResponse() > + .get(SolrQueryResponse.RESPONSE_HEADER_KEY); > + if (header != null) { > + Long shardCpuTime = (Long) > header.get(ThreadStats.CPU_TIME); > + if (shardCpuTime != null) { > + totalShardCpuTime += shardCpuTime; > + } > + } > + } > + } > > Review Comment: > could you extract out a method for this to try to keep > handleRequestBody less long? > Gate by "publishCpuTime" as 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-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 > >
