comments and such

Author: original-brownbear

server/src/internalClusterTest/java/org/elasticsearch/action/admin/cluster/node/tasks/TasksIT.java

@@ -26,7 +26,6 @@
 import org.elasticsearch.action.admin.indices.validate.query.ValidateQueryAction;
 import org.elasticsearch.action.bulk.TransportBulkAction;
 import org.elasticsearch.action.index.TransportIndexAction;
-import org.elasticsearch.action.search.SearchQueryThenFetchAsyncAction;
 import org.elasticsearch.action.search.SearchTransportService;
 import org.elasticsearch.action.search.TransportSearchAction;
 import org.elasticsearch.action.support.WriteRequest;
@@ -354,6 +353,7 @@ public void testTransportBulkTasks() {
     }
 
     public void testSearchTaskDescriptions() {
+        // TODO: enhance this test to also check the tasks created by batched query execution
         updateClusterSettings(Settings.builder().put(SearchService.BATCHED_QUERY_PHASE.getKey(), false));
         registerTaskManagerListeners(TransportSearchAction.TYPE.name());  // main task
         registerTaskManagerListeners(TransportSearchAction.TYPE.name() + "[*]");  // shard task
@@ -396,7 +396,6 @@ public void testSearchTaskDescriptions() {
                     taskInfo.description(),
                     Regex.simpleMatch("id[*], size[1], lastEmittedDoc[null]", taskInfo.description())
                 );
-                case SearchQueryThenFetchAsyncAction.NODE_SEARCH_ACTION_NAME -> assertThat(taskInfo.description(), notNullValue());
                 default -> fail("Unexpected action [" + taskInfo.action() + "] with description [" + taskInfo.description() + "]");
             }
             // assert that all task descriptions have non-zero length

server/src/internalClusterTest/java/org/elasticsearch/search/SearchCancellationIT.java

@@ -240,6 +240,7 @@ public void testCancelMultiSearch() throws Exception {
     }
 
     public void testCancelFailedSearchWhenPartialResultDisallowed() throws Exception {
+        // TODO: make this test compatible with batched execution, currently the exceptions are slightly different with batched
         updateClusterSettings(Settings.builder().put(SearchService.BATCHED_QUERY_PHASE.getKey(), false));
         // Have at least two nodes so that we have parallel execution of two request guaranteed even if max concurrent requests per node
         // are limited to 1

server/src/internalClusterTest/java/org/elasticsearch/search/aggregations/bucket/TermsDocCountErrorIT.java

@@ -55,6 +55,8 @@ public static String randomExecutionHint() {
 
     @Before
     public void disableBatchedExecution() {
+        // TODO: it's practically impossible to get a 100% deterministic test with batched execution unfortunately, adjust this test to
+        // still do something useful with batched execution (i.e. use somewhat relaxed assertions)
         updateClusterSettings(Settings.builder().put(SearchService.BATCHED_QUERY_PHASE.getKey(), false));
     }
 
@@ -922,7 +924,6 @@ public void testDoubleValueFieldSubAggDesc() throws Exception {
      * 3 one-shard indices.
      */
     public void testFixedDocs() throws Exception {
-        updateClusterSettings(Settings.builder().put(SearchService.BATCHED_QUERY_PHASE.getKey(), false));
         assertNoFailuresAndResponse(
             prepareSearch("idx_fixed_docs_0", "idx_fixed_docs_1", "idx_fixed_docs_2").addAggregation(
                 terms("terms").executionHint(randomExecutionHint())
@@ -971,7 +972,6 @@ public void testFixedDocs() throws Exception {
                 assertThat(bucket.getDocCountError(), equalTo(29L));
             }
         );
-        updateClusterSettings(Settings.builder().putNull(SearchService.BATCHED_QUERY_PHASE.getKey()));
     }
 
     /**

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

@@ -269,8 +269,7 @@ public boolean hasReferences() {
         public boolean decRef() {
             if (refCounted.decRef()) {
                 for (int i = 0; i < results.length; i++) {
-                    Object result = results[i];
-                    if (result instanceof RefCounted r) {
+                    if (results[i] instanceof RefCounted r) {
                         r.decRef();
                     }
                     results[i] = null;
@@ -530,7 +529,7 @@ private void onNodeQueryFailure(Exception e, NodeQueryRequest request, CanMatchP
 
     private static final CircuitBreaker NOOP_CIRCUIT_BREAKER = new NoopCircuitBreaker("request");
 
-    public static void registerNodeSearchAction(
+    static void registerNodeSearchAction(
         SearchTransportService searchTransportService,
         SearchService searchService,
         SearchPhaseController searchPhaseController
@@ -817,6 +816,7 @@ && isPartOfPIT(searchRequest.searchRequest, q.getContextId()) == false) {
         void consumeResult(QuerySearchResult queryResult) {
             // no need for any cache effects when we're already flipped to ture => plain read + set-release
             hasResponse.compareAndExchangeRelease(false, true);
+            // TODO: dry up the bottom sort collector with the coordinator side logic in the top-level class here
             if (queryResult.isNull() == false
                 // disable sort optims for scroll requests because they keep track of the last bottom doc locally (per shard)
                 && searchRequest.searchRequest.scroll() == null