SOLR-18330: RTG requests to TLOG can short-circuit

changelog/unreleased/SOLR-18330-TLOG-RTG-shortCircuit.yml

@@ -0,0 +1,8 @@
+title: >
+  Optimization: RTG requests to a TLOG leader replica shouldn't use an extra local HTTP hop.
+type: changed
+authors:
+  - name: David Smiley
+links:
+  - name: SOLR-18330
+    url: https://issues.apache.org/jira/browse/SOLR-18330

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

@@ -38,8 +38,9 @@ protected List<String> getDefaultComponents() {
 
   @Override
   public void handleRequestBody(SolrQueryRequest req, SolrQueryResponse rsp) throws Exception {
-    // Tell HttpShardHandlerthat this request should only be distributed to NRT replicas
-    req.getContext().put(HttpShardHandler.ONLY_NRT_REPLICAS, Boolean.TRUE);
+    // Tell HttpShardHandler that this request should only be distributed to replicas that serve the
+    // most up-to-date view of the data
+    req.getContext().put(HttpShardHandler.ONLY_RTG_REPLICAS, Boolean.TRUE);
     super.handleRequestBody(req, rsp);
   }
 

solr/core/src/java/org/apache/solr/handler/component/CloudReplicaSource.java

@@ -171,10 +171,8 @@ private List<String> findReplicas(
               .filter(replica -> replica.isActive(clusterState.getLiveNodes()))
               .filter(
                   replica ->
-                      !builder.onlyNrt
-                          || (replica.getType() == Replica.Type.NRT
-                              || (replica.getType() == Replica.Type.TLOG
-                                  && isShardLeader.test(replica))))
+                      !builder.onlyRtg
+                          || (replica.getType() == Replica.Type.NRT || isShardLeader.test(replica)))
               .collect(Collectors.toList());
       builder.replicaListTransformer.transform(list);
       List<String> coreUrls = list.stream().map(Replica::getCoreUrl).collect(Collectors.toList());
@@ -275,7 +273,7 @@ static class Builder {
     private String collection;
     private ZkStateReader zkStateReader;
     private SolrParams params;
-    private boolean onlyNrt;
+    private boolean onlyRtg;
     private ReplicaListTransformer replicaListTransformer;
     private AllowListUrlChecker urlChecker;
 
@@ -294,8 +292,9 @@ public Builder params(SolrParams params) {
       return this;
     }
 
-    public Builder onlyNrt(boolean onlyNrt) {
-      this.onlyNrt = onlyNrt;
+    /** Replica can serve the most recent data (RealTimeGet capable). */
+    public Builder onlyRtg(boolean onlyRtg) {
+      this.onlyRtg = onlyRtg;
       return this;
     }
 

solr/core/src/java/org/apache/solr/handler/component/HttpShardHandler.java

@@ -75,13 +75,10 @@ public class HttpShardHandler extends ShardHandler {
 
   /**
    * If the request context map has an entry with this key and Boolean.TRUE as value, {@link
-   * #prepDistributed(ResponseBuilder)} will only include {@link
-   * org.apache.solr.common.cloud.Replica.Type#NRT} replicas as possible destination of the
-   * distributed request (or a leader replica of type {@link
-   * org.apache.solr.common.cloud.Replica.Type#TLOG}). This is used by the RealtimeGet handler,
-   * since other types of replicas shouldn't respond to RTG requests
+   * #prepDistributed(ResponseBuilder)} will only include replicas that serve the latest data. This
+   * is used by the {@link org.apache.solr.handler.RealTimeGetHandler} (RTG).
    */
-  public static final String ONLY_NRT_REPLICAS = "distribOnlyRealtime";
+  public static final String ONLY_RTG_REPLICAS = "distribOnlyRealtime";
 
   /**
    * This is a fake ShardResponse used internally to trigger the {@link #take(boolean)} method to
@@ -496,7 +493,7 @@ public void prepDistributed(ResponseBuilder rb) {
 
     ReplicaSource replicaSource;
     if (zkController != null) {
-      boolean onlyNrt = Boolean.TRUE.equals(req.getContext().get(ONLY_NRT_REPLICAS));
+      boolean onlyRtg = Boolean.TRUE.equals(req.getContext().get(ONLY_RTG_REPLICAS));
 
       replicaSource =
           new CloudReplicaSource.Builder()
@@ -505,11 +502,11 @@ public void prepDistributed(ResponseBuilder rb) {
               .allowListUrlChecker(urlChecker)
               .replicaListTransformer(replicaListTransformer)
               .collection(cloudDescriptor.getCollectionName())
-              .onlyNrt(onlyNrt)
+              .onlyRtg(onlyRtg)
               .build();
       rb.slices = replicaSource.getSliceNames().toArray(new String[replicaSource.getSliceCount()]);
 
-      if (!rb.isForcedDistrib() && canShortCircuit(rb.slices, onlyNrt, params, cloudDescriptor)) {
+      if (!rb.isForcedDistrib() && canShortCircuit(rb.slices, onlyRtg, params, cloudDescriptor)) {
         rb.isDistrib = false;
         rb.shortCircuitedURL =
             ZkCoreNodeProps.getCoreUrl(zkController.getBaseUrl(), coreDescriptor.getName());
@@ -528,7 +525,7 @@ public void prepDistributed(ResponseBuilder rb) {
                     .allowListUrlChecker(AllowListUrlChecker.ALLOW_ALL)
                     .replicaListTransformer(NoOpReplicaListTransformer.INSTANCE)
                     .collection(cloudDescriptor.getCollectionName())
-                    .onlyNrt(false)
+                    .onlyRtg(false)
                     .build();
             final String adjective =
                 (allActiveReplicaSource.getReplicasBySlice(i).isEmpty() ? "active" : "eligible");
@@ -574,21 +571,24 @@ private static String createSliceShardsStr(final List<String> shardUrls) {
   /** Can we avoid distributed search / coordinator? */
   private boolean canShortCircuit(
       String[] slices,
-      boolean onlyNrtReplicas,
+      boolean onlyRtgReplicas,
       SolrParams params,
       CloudDescriptor cloudDescriptor) {
     // Are we hosting the shard that this request is for, and are we active? If so, then handle it
     // ourselves and make it a non-distributed request.
     String ourSlice = cloudDescriptor.getShardId();
     String ourCollection = cloudDescriptor.getCollectionName();
-    // Some requests may only be fulfilled by replicas of type Replica.Type.NRT
+    // Real-time requests may only be fulfilled by an NRT replica or the shard leader (e.g. a TLOG
+    // leader), matching the replica selection in CloudReplicaSource.
     if (slices.length == 1
         && slices[0] != null
         && (slices[0].equals(ourSlice)
             || slices[0].equals(
                 ourCollection + "_" + ourSlice)) // handle the <collection>_<slice> format
         && cloudDescriptor.getLastPublished() == Replica.State.ACTIVE
-        && (!onlyNrtReplicas || cloudDescriptor.getReplicaType() == Replica.Type.NRT)) {
+        && (!onlyRtgReplicas
+            || cloudDescriptor.getReplicaType() == Replica.Type.NRT
+            || cloudDescriptor.isLeader())) {
       // currently just a debugging parameter to check distrib search on a single node
       boolean shortCircuit = params.getBool("shortCircuit", true);
 

solr/core/src/test/org/apache/solr/handler/component/CloudReplicaSourceTest.java

@@ -48,7 +48,7 @@ public void testSimple_ShardsParam() {
       CloudReplicaSource cloudReplicaSource =
           new CloudReplicaSource.Builder()
               .collection("collection1")
-              .onlyNrt(false)
+              .onlyRtg(false)
               .zkStateReader(zkStateReader)
               .replicaListTransformer(replicaListTransformer)
               .allowListUrlChecker(checker)
@@ -79,7 +79,7 @@ public void testShardsParam_DeadNode() {
       CloudReplicaSource cloudReplicaSource =
           new CloudReplicaSource.Builder()
               .collection("collection1")
-              .onlyNrt(false)
+              .onlyRtg(false)
               .zkStateReader(zkStateReader)
               .replicaListTransformer(replicaListTransformer)
               .allowListUrlChecker(checker)
@@ -108,7 +108,7 @@ public void testShardsParam_DownReplica() {
       CloudReplicaSource cloudReplicaSource =
           new CloudReplicaSource.Builder()
               .collection("collection1")
-              .onlyNrt(false)
+              .onlyRtg(false)
               .zkStateReader(zkStateReader)
               .replicaListTransformer(replicaListTransformer)
               .allowListUrlChecker(checker)
@@ -139,7 +139,7 @@ public void testMultipleCollections() {
       CloudReplicaSource cloudReplicaSource =
           new CloudReplicaSource.Builder()
               .collection("collection1")
-              .onlyNrt(false)
+              .onlyRtg(false)
               .zkStateReader(zkStateReader)
               .replicaListTransformer(replicaListTransformer)
               .allowListUrlChecker(checker)
@@ -186,7 +186,7 @@ public void testSimple_UsingClusterState() {
       CloudReplicaSource cloudReplicaSource =
           new CloudReplicaSource.Builder()
               .collection("collection1")
-              .onlyNrt(false)
+              .onlyRtg(false)
               .zkStateReader(zkStateReader)
               .replicaListTransformer(replicaListTransformer)
               .allowListUrlChecker(checker)
@@ -228,7 +228,7 @@ public void testSimple_OnlyNrt() {
       CloudReplicaSource cloudReplicaSource =
           new CloudReplicaSource.Builder()
               .collection("collection1")
-              .onlyNrt(true) // enable only nrt mode
+              .onlyRtg(true) // enable only nrt mode
               .zkStateReader(zkStateReader)
               .replicaListTransformer(replicaListTransformer)
               .allowListUrlChecker(checker)
@@ -273,7 +273,7 @@ public void testMultipleCollections_OnlyNrt() {
       CloudReplicaSource cloudReplicaSource =
           new CloudReplicaSource.Builder()
               .collection("collection1")
-              .onlyNrt(true) // enable only nrt mode
+              .onlyRtg(true) // enable only nrt mode
               .zkStateReader(zkStateReader)
               .replicaListTransformer(replicaListTransformer)
               .allowListUrlChecker(checker)

solr/modules/opentelemetry/src/test-files/solr/tracing/TestDistributedTracing/testRealTimeGetTlogLeaderShortCircuit.json

@@ -0,0 +1,19 @@
+{
+  "phases": [
+    {
+      "description": "phase 0",
+      "spans": [
+        {
+          "name": "get:/{collection}/get",
+          "kind": "SERVER",
+          "db.instance": "tlogCollection",
+          "db.type": "solr",
+          "http.request.method": "GET",
+          "http.response.status_code": 200,
+          "http.url": "http://NORMALIZED/solr/tlogCollection/get",
+          "http.params": "ids=1&wt=javabin"
+        }
+      ]
+    }
+  ]
+}

solr/modules/opentelemetry/src/test/org/apache/solr/opentelemetry/TestDistributedTracing.java

@@ -32,14 +32,17 @@
 import org.apache.solr.client.solrj.SolrRequest;
 import org.apache.solr.client.solrj.request.CollectionAdminRequest;
 import org.apache.solr.client.solrj.request.MetricsRequest;
+import org.apache.solr.client.solrj.request.QueryRequest;
 import org.apache.solr.client.solrj.request.SolrQuery;
 import org.apache.solr.client.solrj.request.UpdateRequest;
 import org.apache.solr.client.solrj.request.V2Request;
 import org.apache.solr.client.solrj.response.CollectionAdminResponse;
 import org.apache.solr.client.solrj.response.InputStreamResponseParser;
+import org.apache.solr.client.solrj.response.QueryResponse;
 import org.apache.solr.client.solrj.response.V2Response;
 import org.apache.solr.cloud.SolrCloudTestCase;
 import org.apache.solr.common.SolrDocumentList;
+import org.apache.solr.common.cloud.Replica;
 import org.apache.solr.common.util.NamedList;
 import org.apache.solr.common.util.RetryUtil;
 import org.apache.solr.util.stats.MetricUtils;
@@ -53,6 +56,7 @@
 public class TestDistributedTracing extends SolrCloudTestCase {
 
   private static final String COLLECTION = "collection1";
+  private static final String TLOG_COLLECTION = "tlogCollection";
 
   @BeforeClass
   public static void setupCluster() throws Exception {
@@ -97,6 +101,18 @@ public static void setupCluster() throws Exception {
         .setNode(node3)
         .process(cluster.getSolrClient());
     cluster.waitForActiveCollection(COLLECTION, 2, 4);
+
+    // A single-shard, single-TLOG-replica collection for testing real-time GET routing. A lone
+    // replica is the leader, so there's no follower replication polling to add nondeterministic
+    // spans. Its leader lives on node0.
+    CollectionAdminRequest.createCollection(TLOG_COLLECTION, "config", 1, 0, 1, 0)
+        .setCreateNodeSet("EMPTY")
+        .process(cluster.getSolrClient());
+    CollectionAdminRequest.addReplicaToShard(TLOG_COLLECTION, "shard1")
+        .setType(Replica.Type.TLOG)
+        .setNode(node0)
+        .process(cluster.getSolrClient());
+    cluster.waitForActiveCollection(TLOG_COLLECTION, 1, 1);
   }
 
   @AfterClass
@@ -129,6 +145,30 @@ public void test() throws Exception {
     verifier.done();
   }
 
+  /**
+   * A real-time GET against a TLOG collection must be served by the shard leader. When the request
+   * lands on the leader (as here), it is served locally with no self-hop to itself; the gold file
+   * therefore has a single span. Were the leader not short-circuited, a child {@code /{core}/get}
+   * span would appear.
+   */
+  @Test
+  public void testRealTimeGetTlogLeaderShortCircuit() throws Exception {
+    var verifier = new GoldFileTraceVerifier(getClass(), "testRealTimeGetTlogLeaderShortCircuit");
+    var leaderClient = cluster.getJettySolrRunner(0).getSolrClient();
+
+    new UpdateRequest().add(sdoc("id", "1")).commit(leaderClient, TLOG_COLLECTION);
+    getAndClearSpans(); // ignore indexing spans
+
+    var req = new QueryRequest(params("ids", "1"));
+    req.setPath("/get");
+    QueryResponse rsp = req.process(leaderClient, TLOG_COLLECTION);
+    assertEquals(1, rsp.getResults().size());
+    assertEquals("1", rsp.getResults().get(0).getFieldValue("id"));
+
+    verifier.verifyPhase();
+    verifier.done();
+  }
+
   @Test
   public void testAdminApi() throws Exception {
     var verifier = new GoldFileTraceVerifier(getClass(), "testAdminApi");