Rework ShardSearchContextId to explain use of searcher id better (#135233)

When a ShardSearchContextId has a searcher id, this indicates that the
searcher backing the context is stable so we can retry a shard on a
different node if the original context is gone (e.g. due to node
restarts). We use the searcher ids only for PIT contexts and only if the
underlying engine supports it (e.g. currently only FrozenEngine and
ReadOnlyEngine).
This change moves the related decision logic into ShardSearchContextId itself
because there is no need to expose the search id once its set. By
renaming the access methods we can also communicate the intended use of
these ids better.

server/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java

@@ -81,6 +81,7 @@ import org.elasticsearch.search.builder.PointInTimeBuilder;
 import org.elasticsearch.search.builder.SearchSourceBuilder;
 import org.elasticsearch.search.internal.AliasFilter;
 import org.elasticsearch.search.internal.SearchContext;
+import org.elasticsearch.search.internal.ShardSearchContextId;
 import org.elasticsearch.search.profile.SearchProfileResults;
 import org.elasticsearch.search.profile.SearchProfileShardResult;
 import org.elasticsearch.tasks.Task;
@@ -1271,7 +1272,8 @@ public class TransportSearchAction extends HandledTransportAction<SearchRequest,
                     // Otherwise, we add the shard iterator without a target node, allowing a partial search failure to
                     // be thrown when a search phase attempts to access it.
                     targetNodes.add(perNode.getNode());
-                    if (perNode.getSearchContextId().getSearcherId() != null) {
+                    ShardSearchContextId shardSearchContextId = perNode.getSearchContextId();
+                    if (shardSearchContextId != null && shardSearchContextId.isRetryable()) {
                         for (String node : group.allocatedNodes()) {
                             if (node.equals(perNode.getNode()) == false) {
                                 targetNodes.add(node);
@@ -1947,7 +1949,8 @@ public class TransportSearchAction extends HandledTransportAction<SearchRequest,
                         if (projectState.cluster().nodes().nodeExists(perNode.getNode())) {
                             targetNodes.add(perNode.getNode());
                         }
-                        if (perNode.getSearchContextId().getSearcherId() != null) {
+                        ShardSearchContextId shardSearchContextId = perNode.getSearchContextId();
+                        if (shardSearchContextId.isRetryable()) {
                             for (ShardRouting shard : shards) {
                                 if (shard.currentNodeId().equals(perNode.getNode()) == false) {
                                     targetNodes.add(shard.currentNodeId());

server/src/main/java/org/elasticsearch/search/SearchService.java

@@ -1264,18 +1264,18 @@ public class SearchService extends AbstractLifecycleComponent implements IndexEv
     }
 
     final ReaderContext createOrGetReaderContext(ShardSearchRequest request) {
+        ShardSearchContextId readerId = request.readerId();
         if (request.readerId() != null) {
             try {
-                return findReaderContext(request.readerId(), request);
+                return findReaderContext(readerId, request);
             } catch (SearchContextMissingException e) {
-                final String searcherId = request.readerId().getSearcherId();
-                if (searcherId == null) {
+                if (readerId.isRetryable() == false) {
                     throw e;
                 }
                 final IndexService indexService = indicesService.indexServiceSafe(request.shardId().getIndex());
                 final IndexShard shard = indexService.getShard(request.shardId().id());
                 final Engine.SearcherSupplier searcherSupplier = shard.acquireSearcherSupplier();
-                if (searcherId.equals(searcherSupplier.getSearcherId()) == false) {
+                if (readerId.sameSearcherIdsAs(searcherSupplier.getSearcherId()) == false) {
                     searcherSupplier.close();
                     throw e;
                 }
@@ -1298,8 +1298,8 @@ public class SearchService extends AbstractLifecycleComponent implements IndexEv
         ReaderContext readerContext = null;
         Releasable decreaseScrollContexts = null;
         try {
-            final ShardSearchContextId id = new ShardSearchContextId(sessionId, idGenerator.incrementAndGet());
             if (request.scroll() != null) {
+                final ShardSearchContextId id = new ShardSearchContextId(sessionId, idGenerator.incrementAndGet());
                 decreaseScrollContexts = openScrollContexts::decrementAndGet;
                 if (openScrollContexts.incrementAndGet() > maxOpenScrollContext) {
                     throw new TooManyScrollContextsException(maxOpenScrollContext, MAX_OPEN_SCROLL_CONTEXT.getKey());
@@ -1308,6 +1308,7 @@ public class SearchService extends AbstractLifecycleComponent implements IndexEv
                 readerContext.addOnClose(decreaseScrollContexts);
                 decreaseScrollContexts = null;
             } else {
+                final ShardSearchContextId id = new ShardSearchContextId(sessionId, idGenerator.incrementAndGet(), reader.getSearcherId());
                 readerContext = new ReaderContext(id, indexService, shard, reader, keepAliveInMillis, true);
             }
             reader = null;
@@ -1399,7 +1400,7 @@ public class SearchService extends AbstractLifecycleComponent implements IndexEv
         final IndexService indexService = indicesService.indexServiceSafe(request.shardId().getIndex());
         final IndexShard indexShard = indexService.getShard(request.shardId().getId());
         final Engine.SearcherSupplier reader = indexShard.acquireSearcherSupplier();
-        final ShardSearchContextId id = new ShardSearchContextId(sessionId, idGenerator.incrementAndGet());
+        final ShardSearchContextId id = new ShardSearchContextId(sessionId, idGenerator.incrementAndGet(), reader.getSearcherId());
         try (ReaderContext readerContext = new ReaderContext(id, indexService, indexShard, reader, -1L, true)) {
             DefaultSearchContext searchContext = createSearchContext(readerContext, request, timeout, ResultsType.NONE);
             searchContext.addReleasable(readerContext.markAsUsed(0L));
@@ -2037,8 +2038,7 @@ public class SearchService extends AbstractLifecycleComponent implements IndexEv
                     }
                     searcher = readerContext.acquireSearcher(Engine.CAN_MATCH_SEARCH_SOURCE);
                 } catch (SearchContextMissingException e) {
-                    final String searcherId = canMatchContext.request.readerId().getSearcherId();
-                    if (searcherId == null) {
+                    if (canMatchContext.request.readerId().isRetryable() == false) {
                         return new CanMatchShardResponse(true, null);
                     }
                     if (queryStillMatchesAfterRewrite(
@@ -2048,7 +2048,7 @@ public class SearchService extends AbstractLifecycleComponent implements IndexEv
                         return new CanMatchShardResponse(false, null);
                     }
                     final Engine.SearcherSupplier searcherSupplier = canMatchContext.getShard().acquireSearcherSupplier();
-                    if (searcherId.equals(searcherSupplier.getSearcherId()) == false) {
+                    if (canMatchContext.request.readerId().sameSearcherIdsAs(searcherSupplier.getSearcherId()) == false) {
                         searcherSupplier.close();
                         return new CanMatchShardResponse(true, null);
                     }

server/src/main/java/org/elasticsearch/search/internal/LegacyReaderContext.java

@@ -36,7 +36,7 @@ public final class LegacyReaderContext extends ReaderContext {
         super(id, indexService, indexShard, reader, keepAliveInMillis, false);
         assert shardSearchRequest.readerId() == null;
         assert shardSearchRequest.keepAlive() == null;
-        assert id.getSearcherId() == null : "Legacy reader context must not have searcher id";
+        assert id.isRetryable() == false : "Legacy reader context is not retryable";
         this.shardSearchRequest = Objects.requireNonNull(shardSearchRequest, "ShardSearchRequest must be provided");
         if (shardSearchRequest.scroll() != null) {
             // Search scroll requests are special, they don't hold indices names so we have

server/src/main/java/org/elasticsearch/search/internal/ShardSearchContextId.java

@@ -21,7 +21,6 @@ public final class ShardSearchContextId implements Writeable {
     private final long id;
     private final String searcherId;
 
-    // TODO: Remove this constructor
     public ShardSearchContextId(String sessionId, long id) {
         this(sessionId, id, null);
     }
@@ -43,7 +42,6 @@ public final class ShardSearchContextId implements Writeable {
         out.writeLong(id);
         out.writeString(sessionId);
         out.writeOptionalString(searcherId);
-
     }
 
     public String getSessionId() {
@@ -54,8 +52,12 @@ public final class ShardSearchContextId implements Writeable {
         return id;
     }
 
-    public String getSearcherId() {
-        return searcherId;
+    public boolean isRetryable() {
+        return this.searcherId != null;
+    }
+
+    public boolean sameSearcherIdsAs(String otherSearcherId) {
+        return this.isRetryable() && this.searcherId.equals(otherSearcherId);
     }
 
     @Override

server/src/test/java/org/elasticsearch/action/search/TransportSearchActionTests.java

@@ -1702,7 +1702,7 @@ public class TransportSearchActionTests extends ESTestCase {
             final ShardId shardId = new ShardId(indexMetadata.getIndex(), id);
             final SearchShardIterator shardIterator = shardIterators.get(id);
             final SearchContextIdForNode context = contexts.get(shardId);
-            if (context.getSearchContextId().getSearcherId() == null) {
+            if (context.getSearchContextId().isRetryable() == false) {
                 assertThat(shardIterator.getTargetNodeIds(), hasSize(1));
             } else {
                 final List<String> targetNodes = clusterState.routingTable(project)