Merge remote-tracking branch 'origin/master' into gradle-master

lucene/CHANGES.txt

@@ -137,6 +137,9 @@ Bug Fixes
 
 * LUCENE-9031: UnsupportedOperationException on MatchesIterator.getQuery() (Alan Woodward, Mikhail Khludnev)
 
+* LUCENE-8996: maxScore was sometimes missing from distributed grouped responses.
+  (Julien Massenet, Diego Ceccarelli, Munendra S N, Christine Poerschke)
+
 Other
 
 * LUCENE-8979: Code Cleanup: Use entryset for map iteration wherever possible. - Part 2 (Koen De Groote)

lucene/grouping/src/java/org/apache/lucene/search/grouping/TopGroups.java

@@ -80,6 +80,19 @@ public class TopGroups<T> {
     Avg,
   }
 
+  /**
+   * If either value is NaN then return the other value, otherwise
+   * return the greater of the two values by calling Math.max.
+   * @param a - one value
+   * @param b - another value
+   * @return ignoring any NaN return the greater of a and b
+   */
+  private static float nonNANmax(float a, float b) {
+    if (Float.isNaN(a)) return b;
+    if (Float.isNaN(b)) return a;
+    return Math.max(a, b);
+  }
+
   /** Merges an array of TopGroups, for example obtained
    *  from the second-pass collector across multiple
    *  shards.  Each TopGroups must have been sorted by the
@@ -135,12 +148,12 @@ public class TopGroups<T> {
     } else {
       shardTopDocs = new TopFieldDocs[shardGroups.length];
     }
-    float totalMaxScore = Float.MIN_VALUE;
+    float totalMaxScore = Float.NaN;
 
     for(int groupIDX=0;groupIDX<numGroups;groupIDX++) {
       final T groupValue = shardGroups[0].groups[groupIDX].groupValue;
       //System.out.println("  merge groupValue=" + groupValue + " sortValues=" + Arrays.toString(shardGroups[0].groups[groupIDX].groupSortValues));
-      float maxScore = Float.MIN_VALUE;
+      float maxScore = Float.NaN;
       int totalHits = 0;
       double scoreSum = 0.0;
       for(int shardIDX=0;shardIDX<shardGroups.length;shardIDX++) {
@@ -174,7 +187,7 @@ public class TopGroups<T> {
           shardTopDocs[shardIDX].scoreDocs[i].shardIndex = shardIDX;
         }
 
-        maxScore = Math.max(maxScore, shardGroupDocs.maxScore);
+        maxScore =  nonNANmax(maxScore, shardGroupDocs.maxScore);
         assert shardGroupDocs.totalHits.relation == Relation.EQUAL_TO;
         totalHits += shardGroupDocs.totalHits.value;
         scoreSum += shardGroupDocs.score;
@@ -228,7 +241,7 @@ public class TopGroups<T> {
                                                    mergedScoreDocs,
                                                    groupValue,
                                                    shardGroups[0].groups[groupIDX].groupSortValues);
-      totalMaxScore = Math.max(totalMaxScore, maxScore);
+      totalMaxScore = nonNANmax(totalMaxScore, maxScore);
     }
 
     if (totalGroupCount != null) {

lucene/grouping/src/test/org/apache/lucene/search/grouping/TopGroupsTest.java

@@ -21,16 +21,12 @@ import org.apache.lucene.search.Sort;
 import org.apache.lucene.search.TotalHits;
 import org.apache.lucene.util.LuceneTestCase;
 
-import org.junit.Ignore;
-
 public class TopGroupsTest extends LuceneTestCase {
 
-  @Ignore // https://issues.apache.org/jira/browse/LUCENE-8996
   public void testAllGroupsEmptyInSecondPass() {
     narrativeMergeTestImplementation(false, false, false, false);
   }
 
-  @Ignore // https://issues.apache.org/jira/browse/LUCENE-8996
   public void testSomeGroupsEmptyInSecondPass() {
     narrativeMergeTestImplementation(false, false, false, true);
     narrativeMergeTestImplementation(false, false, true, false);
@@ -166,13 +162,13 @@ public class TopGroupsTest extends LuceneTestCase {
     {
       assertEquals(blueGroupValue, mergedTopGroups.groups[0].groupValue);
       final float expectedBlueMaxScore =
-          (haveBlueWhale ? blueWhaleScore : (haveBlueDragonfly ? blueDragonflyScore : Float.MIN_VALUE));
+          (haveBlueWhale ? blueWhaleScore : (haveBlueDragonfly ? blueDragonflyScore : Float.NaN));
       checkMaxScore(expectedBlueMaxScore, mergedTopGroups.groups[0].maxScore);
     }
     {
       assertEquals(redGroupValue, mergedTopGroups.groups[1].groupValue);
       final float expectedRedMaxScore =
-          (haveRedSquirrel ? redSquirrelScore : (haveRedAnt ? redAntScore : Float.MIN_VALUE));
+          (haveRedSquirrel ? redSquirrelScore : (haveRedAnt ? redAntScore : Float.NaN));
       checkMaxScore(expectedRedMaxScore, mergedTopGroups.groups[1].maxScore);
     }
 
@@ -181,7 +177,7 @@ public class TopGroupsTest extends LuceneTestCase {
             : (haveRedSquirrel ? redSquirrelScore
                 : (haveBlueDragonfly ? blueDragonflyScore
                     : (haveRedAnt ? redAntScore
-                        : Float.MIN_VALUE))));
+                        : Float.NaN))));
     checkMaxScore(expectedMaxScore, mergedTopGroups.maxScore);
   }
 

solr/CHANGES.txt

@@ -165,6 +165,8 @@ Improvements
 
 * SOLR-13987: Admin UI should not rely on javascript eval() (rmuir, Kevin Risden)
 
+* SOLR-13979: Expose separate metrics for distributed and non-distributed requests. (ab)
+
 Optimizations
 ---------------------
 (No changes)

solr/core/src/java/org/apache/solr/handler/RequestHandlerBase.java

@@ -19,7 +19,6 @@ package org.apache.solr.handler;
 import java.lang.invoke.MethodHandles;
 import java.util.Collection;
 import java.util.Map;
-import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
 
 import com.codahale.metrics.Counter;
@@ -30,6 +29,7 @@ import org.apache.solr.api.Api;
 import org.apache.solr.api.ApiBag;
 import org.apache.solr.api.ApiSupport;
 import org.apache.solr.common.SolrException;
+import org.apache.solr.common.params.CommonParams;
 import org.apache.solr.common.params.ShardParams;
 import org.apache.solr.common.params.SolrParams;
 import org.apache.solr.common.util.NamedList;
@@ -68,7 +68,11 @@ public abstract class RequestHandlerBase implements SolrRequestHandler, SolrInfo
   private Counter requests = new Counter();
   private final Map<String, Counter> shardPurposes = new ConcurrentHashMap<>();
   private Timer requestTimes = new Timer();
+  private Timer distribRequestTimes = new Timer();
+  private Timer localRequestTimes = new Timer();
   private Counter totalTime = new Counter();
+  private Counter distribTotalTime = new Counter();
+  private Counter localTotalTime = new Counter();
 
   private final long handlerStart;
 
@@ -76,7 +80,6 @@ public abstract class RequestHandlerBase implements SolrRequestHandler, SolrInfo
 
   private PluginInfo pluginInfo;
 
-  private Set<String> metricNames = ConcurrentHashMap.newKeySet();
   protected SolrMetricsContext solrMetricsContext;
 
 
@@ -156,7 +159,11 @@ public abstract class RequestHandlerBase implements SolrRequestHandler, SolrInfo
         shardPurposes.forEach((k, v) -> map.put(k, v.getCount())));
     solrMetricsContext.gauge(metricsMap, true, "shardRequests", getCategory().toString(), scope);
     requestTimes = solrMetricsContext.timer("requestTimes", getCategory().toString(), scope);
+    distribRequestTimes = solrMetricsContext.timer("requestTimes", getCategory().toString(), scope, "distrib");
+    localRequestTimes = solrMetricsContext.timer("requestTimes", getCategory().toString(), scope, "local");
     totalTime = solrMetricsContext.counter("totalTime", getCategory().toString(), scope);
+    distribTotalTime = solrMetricsContext.counter("totalTime", getCategory().toString(), scope, "distrib");
+    localTotalTime = solrMetricsContext.counter("totalTime", getCategory().toString(), scope, "local");
     solrMetricsContext.gauge(() -> handlerStart, true, "handlerStart", getCategory().toString(), scope);
   }
 
@@ -177,6 +184,9 @@ public abstract class RequestHandlerBase implements SolrRequestHandler, SolrInfo
   @Override
   public void handleRequest(SolrQueryRequest req, SolrQueryResponse rsp) {
     requests.inc();
+    // requests are distributed by default when ZK is in use, unless indicated otherwise
+    boolean distrib = req.getParams().getBool(CommonParams.DISTRIB,
+        req.getCore() != null ? req.getCore().getCoreContainer().isZooKeeperAware() : false);
     if (req.getParams().getBool(ShardParams.IS_SHARD, false)) {
       shardPurposes.computeIfAbsent("total", name -> new Counter()).inc();
       int purpose = req.getParams().getInt(ShardParams.SHARDS_PURPOSE, 0);
@@ -188,6 +198,7 @@ public abstract class RequestHandlerBase implements SolrRequestHandler, SolrInfo
       }
     }
     Timer.Context timer = requestTimes.time();
+    Timer.Context dTimer = distrib ? distribRequestTimes.time() : localRequestTimes.time();
     try {
       if (pluginInfo != null && pluginInfo.attributes.containsKey(USEPARAM))
         req.getContext().put(USEPARAM, pluginInfo.attributes.get(USEPARAM));
@@ -246,8 +257,14 @@ public abstract class RequestHandlerBase implements SolrRequestHandler, SolrInfo
         }
       }
     } finally {
+      dTimer.stop();
       long elapsed = timer.stop();
       totalTime.inc(elapsed);
+      if (distrib) {
+        distribTotalTime.inc(elapsed);
+      } else {
+        localTotalTime.inc(elapsed);
+      }
     }
   }
 

solr/solr-ref-guide/src/performance-statistics-reference.adoc

@@ -94,6 +94,20 @@ The table below shows the metric names and attributes to request:
 `UPDATE./update.handlerStart` |Epoch time when the handler was registered.
 |===
 
+*Distributed vs. Local Request Times*
+
+Processing of a single distributed request in SolrCloud usually requires making several requests to
+other nodes and other replicas. The common statistics listed above lump these timings together, even though
+they are very different in nature, thus making it difficult to measure the latency of distributed and
+local requests separately. Solr 8.4 introduced additional statistics that help to do this.
+
+These metrics are structured the same as `requestTimes` and `totalTime` metrics above but they use
+different full names, eg. `QUERY./select.distrib.requestTimes` and `QUERY./select.local.requestTimes`.
+The metrics under the `distrib` path correspond to the time it takes for a (potentially) distributed
+request to complete all remote calls plus any local processing, and return the result to the caller.
+The metrics under the `local` path correspond to the time it takes for a local call (non-distributed,
+i.e. being processed only by the Solr core where the handler operates) to complete.
+
 == Update Handler
 
 This section has information on the total number of adds and how many commits have been fired against a Solr core.

solr/solrj/src/java/org/apache/solr/client/solrj/request/UpdateRequest.java

@@ -60,6 +60,7 @@ public class UpdateRequest extends AbstractUpdateRequest {
    *   @deprecated Solr now always includes in the response the {@link #REPFACT}, this parameter
    *   doesn't need to be explicitly set
    */
+  @Deprecated // SOLR-14034
   public static final String MIN_REPFACT = "min_rf";
   public static final String VER = "ver";
   public static final String OVERWRITE = "ow";

solr/solrj/src/java/org/apache/solr/common/params/CommonParams.java

@@ -273,6 +273,7 @@ public interface CommonParams {
    * When querying a node, prefer local node's cores for distributed queries.
    * @deprecated Use {@code ShardParams.SHARDS_PREFERENCE}
    */
+  @Deprecated // SOLR-14035
   String PREFER_LOCAL_SHARDS = "preferLocalShards";
 
   String JAVABIN = "javabin";