address review

elastic · Mar 9, 2020 · 415c910 · javanna · Mar 10, 2020 · 415c910
1 parent 05e2209
commit 415c910
Show file tree

Hide file tree

Showing 8 changed files with 123 additions and 20 deletions.
diff --git a/server/src/main/java/org/elasticsearch/action/search/SearchRequest.java b/server/src/main/java/org/elasticsearch/action/search/SearchRequest.java
@@ -57,7 +57,7 @@
  */
 public class SearchRequest extends ActionRequest implements IndicesRequest.Replaceable {
 
-    private static final ToXContent.Params FORMAT_PARAMS = new ToXContent.MapParams(Collections.singletonMap("pretty", "false"));
+    public static final ToXContent.Params FORMAT_PARAMS = new ToXContent.MapParams(Collections.singletonMap("pretty", "false"));
 
     public static final int DEFAULT_PRE_FILTER_SHARD_SIZE = 128;
     public static final int DEFAULT_BATCHED_REDUCE_SIZE = 512;

diff --git a/x-pack/plugin/async-search/qa/rest/src/test/resources/rest-api-spec/test/search/10_basic.yml b/x-pack/plugin/async-search/qa/rest/src/test/resources/rest-api-spec/test/search/10_basic.yml
@@ -67,8 +67,6 @@
         wait_for_completion: 10s
         clean_on_completion: false
         body:
-          query:
-            match_all: {}
           aggs:
             1:
               max:
@@ -82,6 +80,28 @@
   - match:  { response.hits.hits.0._source.max: 1 }
   - match:  { response.aggregations.1.value:  3.0 }
 
+  # test with typed_keys:
+  - do:
+      async_search.submit:
+        index: test-*
+        batched_reduce_size: 2
+        wait_for_completion: 10s
+        clean_on_completion: false
+        typed_keys: true
+        body:
+          aggs:
+            1:
+              max:
+                field: max
+          sort: max
+
+  - set:    { id:                                  id }
+  - match:  { version:                              6 }
+  - match:  { is_partial:                       false }
+  - length: { response.hits.hits:                   3 }
+  - match:  { response.hits.hits.0._source.max:     1 }
+  - match:  { response.aggregations.max#1.value:  3.0 }
+
   - do:
       async_search.get:
         id: "$id"
@@ -92,6 +112,18 @@
   - match:  { response.hits.hits.0._source.max:   1 }
   - match:  { response.aggregations.1.value:    3.0 }
 
+  # test with typed_keys:
+  - do:
+      async_search.get:
+        id: "$id"
+        typed_keys: true
+
+  - match:  { version:                              6 }
+  - match:  { is_partial:                       false }
+  - length: { response.hits.hits:                   3 }
+  - match:  { response.hits.hits.0._source.max:     1 }
+  - match:  { response.aggregations.max#1.value:  3.0 }
+
   - do:
       async_search.delete:
         id: "$id"

diff --git a/...n/async-search/src/main/java/org/elasticsearch/xpack/search/RestGetAsyncSearchAction.java b/...n/async-search/src/main/java/org/elasticsearch/xpack/search/RestGetAsyncSearchAction.java
@@ -5,19 +5,19 @@
  */
 package org.elasticsearch.xpack.search;
 
-import org.elasticsearch.action.ActionRequestValidationException;
 import org.elasticsearch.client.node.NodeClient;
 import org.elasticsearch.rest.BaseRestHandler;
-import org.elasticsearch.rest.RestHandler.Route;
 import org.elasticsearch.rest.RestRequest;
 import org.elasticsearch.rest.action.RestStatusToXContentListener;
 import org.elasticsearch.xpack.core.search.action.GetAsyncSearchAction;
 
 import java.util.List;
+import java.util.Set;
 
 import static java.util.Arrays.asList;
 import static java.util.Collections.unmodifiableList;
 import static org.elasticsearch.rest.RestRequest.Method.GET;
+import static org.elasticsearch.xpack.search.RestSubmitAsyncSearchAction.RESPONSE_PARAMS;
 
 public class RestGetAsyncSearchAction extends BaseRestHandler  {
     @Override
@@ -43,10 +43,11 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli
         if (request.hasParam("last_version")) {
             get.setLastVersion(request.paramAsInt("last_version", get.getLastVersion()));
         }
-        ActionRequestValidationException validationException = get.validate();
-        if (validationException != null) {
-            throw validationException;
-        }
         return channel -> client.execute(GetAsyncSearchAction.INSTANCE, get, new RestStatusToXContentListener<>(channel));
     }
+
+    @Override
+    protected Set<String> responseParams() {
+        return RESPONSE_PARAMS;
+    }
 }
diff --git a/...sync-search/src/main/java/org/elasticsearch/xpack/search/RestSubmitAsyncSearchAction.java b/...sync-search/src/main/java/org/elasticsearch/xpack/search/RestSubmitAsyncSearchAction.java
@@ -5,10 +5,8 @@
  */
 package org.elasticsearch.xpack.search;
 
-import org.elasticsearch.action.ActionRequestValidationException;
 import org.elasticsearch.client.node.NodeClient;
 import org.elasticsearch.rest.BaseRestHandler;
-import org.elasticsearch.rest.RestHandler.Route;
 import org.elasticsearch.rest.RestRequest;
 import org.elasticsearch.rest.action.RestCancellableNodeClient;
 import org.elasticsearch.rest.action.RestStatusToXContentListener;
@@ -17,6 +15,8 @@
 import org.elasticsearch.xpack.core.search.action.SubmitAsyncSearchRequest;
 
 import java.io.IOException;
+import java.util.Collections;
+import java.util.Set;
 import java.util.function.IntConsumer;
 import java.util.List;
 
@@ -27,6 +27,9 @@
 import static org.elasticsearch.rest.action.search.RestSearchAction.parseSearchRequest;
 
 public final class RestSubmitAsyncSearchAction extends BaseRestHandler {
+    static final String TYPED_KEYS_PARAM = "typed_keys";
+    static final Set<String> RESPONSE_PARAMS = Collections.singleton(TYPED_KEYS_PARAM);
+
     @Override
     public List<Route> routes() {
         return unmodifiableList(asList(
@@ -57,14 +60,15 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli
         if (request.hasParam("clean_on_completion")) {
             submit.setCleanOnCompletion(request.paramAsBoolean("clean_on_completion", submit.isCleanOnCompletion()));
         }
-        ActionRequestValidationException validationException = submit.validate();
-        if (validationException != null) {
-            throw validationException;
-        }
         return channel -> {
             RestStatusToXContentListener<AsyncSearchResponse> listener = new RestStatusToXContentListener<>(channel);
             RestCancellableNodeClient cancelClient = new RestCancellableNodeClient(client, request.getHttpChannel());
             cancelClient.execute(SubmitAsyncSearchAction.INSTANCE, submit, listener);
         };
     }
+
+    @Override
+    protected Set<String> responseParams() {
+        return RESPONSE_PARAMS;
+    }
 }
diff --git a/...nc-search/src/test/java/org/elasticsearch/xpack/search/SubmitAsyncSearchRequestTests.java b/...nc-search/src/test/java/org/elasticsearch/xpack/search/SubmitAsyncSearchRequestTests.java
@@ -5,16 +5,21 @@
  */
 package org.elasticsearch.xpack.search;
 
+import org.elasticsearch.action.ActionRequestValidationException;
 import org.elasticsearch.action.search.SearchType;
 import org.elasticsearch.action.support.IndicesOptions;
 import org.elasticsearch.common.io.stream.Writeable;
 import org.elasticsearch.common.unit.TimeValue;
 import org.elasticsearch.index.query.QueryBuilders;
 import org.elasticsearch.search.aggregations.AggregationBuilders;
 import org.elasticsearch.search.builder.SearchSourceBuilder;
+import org.elasticsearch.search.suggest.SuggestBuilder;
 import org.elasticsearch.xpack.core.search.action.SubmitAsyncSearchRequest;
 import org.elasticsearch.xpack.core.transform.action.AbstractWireSerializingTransformTestCase;
 
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.equalTo;
+
 public class SubmitAsyncSearchRequestTests extends AbstractWireSerializingTransformTestCase<SubmitAsyncSearchRequest> {
     @Override
     protected Writeable.Reader<SubmitAsyncSearchRequest> instanceReader() {
@@ -66,4 +71,42 @@ protected SearchSourceBuilder randomSearchSourceBuilder() {
         }
         return source;
     }
+
+    public void testValidateCssMinimizeRoundtrips() {
+        SubmitAsyncSearchRequest req = new SubmitAsyncSearchRequest();
+        req.getSearchRequest().setCcsMinimizeRoundtrips(true);
+        ActionRequestValidationException exc = req.validate();
+        assertNotNull(exc);
+        assertThat(exc.validationErrors().size(), equalTo(1));
+        assertThat(exc.validationErrors().get(0), containsString("[ccs_minimize_roundtrips]"));
+    }
+
+    public void testValidateScroll() {
+        SubmitAsyncSearchRequest req = new SubmitAsyncSearchRequest();
+        req.getSearchRequest().scroll(TimeValue.timeValueMinutes(5));
+        ActionRequestValidationException exc = req.validate();
+        assertNotNull(exc);
+        assertThat(exc.validationErrors().size(), equalTo(2));
+        // request_cache is activated by default
+        assertThat(exc.validationErrors().get(0), containsString("[request_cache]"));
+        assertThat(exc.validationErrors().get(1), containsString("[scroll]"));
+    }
+
+    public void testValidateKeepAlive() {
+        SubmitAsyncSearchRequest req = new SubmitAsyncSearchRequest();
+        req.setKeepAlive(TimeValue.timeValueSeconds(randomIntBetween(1, 59)));
+        ActionRequestValidationException exc = req.validate();
+        assertNotNull(exc);
+        assertThat(exc.validationErrors().size(), equalTo(1));
+        assertThat(exc.validationErrors().get(0), containsString("[keep_alive]"));
+    }
+
+    public void testValidateSuggestOnly() {
+        SubmitAsyncSearchRequest req = new SubmitAsyncSearchRequest();
+        req.getSearchRequest().source(new SearchSourceBuilder().suggest(new SuggestBuilder()));
+        ActionRequestValidationException exc = req.validate();
+        assertNotNull(exc);
+        assertThat(exc.validationErrors().size(), equalTo(1));
+        assertThat(exc.validationErrors().get(0), containsString("suggest"));
+    }
 }
diff --git a/...in/core/src/main/java/org/elasticsearch/xpack/core/search/action/AsyncSearchResponse.java b/...in/core/src/main/java/org/elasticsearch/xpack/core/search/action/AsyncSearchResponse.java
@@ -193,7 +193,10 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
         builder.field("start_time_in_millis", startTimeMillis);
         builder.field("expiration_time_in_millis", expirationTimeMillis);
 
-        builder.field("response", searchResponse);
+        if (searchResponse != null) {
+            builder.field("response");
+            searchResponse.toXContent(builder, params);
+        }
         if (error != null) {
             builder.startObject("error");
             error.toXContent(builder, params);

diff --git a/...re/src/main/java/org/elasticsearch/xpack/core/search/action/SubmitAsyncSearchRequest.java b/...re/src/main/java/org/elasticsearch/xpack/core/search/action/SubmitAsyncSearchRequest.java
@@ -8,6 +8,7 @@
 import org.elasticsearch.action.ActionRequest;
 import org.elasticsearch.action.ActionRequestValidationException;
 import org.elasticsearch.action.search.SearchRequest;
+import org.elasticsearch.common.Strings;
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
 import org.elasticsearch.common.unit.TimeValue;
@@ -21,14 +22,15 @@
 import java.util.Objects;
 
 import static org.elasticsearch.action.ValidateActions.addValidationError;
+import static org.elasticsearch.action.search.SearchRequest.FORMAT_PARAMS;
 
 /**
  * A request to track asynchronously the progress of a search against one or more indices.
  *
  * @see AsyncSearchResponse
  */
 public class SubmitAsyncSearchRequest extends ActionRequest {
-    public static long MIN_KEEP_ALIVE = TimeValue.timeValueHours(1).millis();
+    public static long MIN_KEEP_ALIVE = TimeValue.timeValueMinutes(1).millis();
 
     private TimeValue waitForCompletion = TimeValue.timeValueSeconds(1);
     private boolean cleanOnCompletion = true;
@@ -128,22 +130,26 @@ public boolean isCleanOnCompletion() {
     public ActionRequestValidationException validate() {
         ActionRequestValidationException validationException = request.validate();
         if (request.scroll() != null) {
-            addValidationError("scroll queries are not supported", validationException);
+            addValidationError("[scroll] queries are not supported", validationException);
         }
         if (request.isSuggestOnly()) {
             validationException = addValidationError("suggest-only queries are not supported", validationException);
         }
         if (keepAlive.getMillis() < MIN_KEEP_ALIVE) {
             validationException =
-                addValidationError("keep_alive must be greater than 1 minute, got:" + keepAlive.toString(), validationException);
+                addValidationError("[keep_alive] must be greater than 1 minute, got:" + keepAlive.toString(), validationException);
+        }
+        if (request.isCcsMinimizeRoundtrips()) {
+            validationException =
+                addValidationError("[ccs_minimize_roundtrips] is not supported on async search queries", validationException);
         }
 
         return validationException;
     }
 
     @Override
     public Task createTask(long id, String type, String action, TaskId parentTaskId, Map<String, String> headers) {
-        return new CancellableTask(id, type, action, "", parentTaskId, headers) {
+        return new CancellableTask(id, type, action, toString(), parentTaskId, headers) {
             @Override
             public boolean shouldCancelChildrenOnCancellation() {
                 return true;
@@ -166,4 +172,14 @@ public boolean equals(Object o) {
     public int hashCode() {
         return Objects.hash(waitForCompletion, cleanOnCompletion, keepAlive, request);
     }
+
+    @Override
+    public String toString() {
+        return "SubmitAsyncSearchRequest{" +
+            "waitForCompletion=" + waitForCompletion +
+            ", cleanOnCompletion=" + cleanOnCompletion +
+            ", keepAlive=" + keepAlive +
+            ", request=" + request +
+            '}';
+    }
 }
diff --git a/x-pack/plugin/src/test/resources/rest-api-spec/api/async_search.get.json b/x-pack/plugin/src/test/resources/rest-api-spec/api/async_search.get.json
@@ -32,6 +32,10 @@
       "keep_alive": {
         "type": "time",
         "description": "Specify the time that the request should remain reachable in the cluster."
+      },
+      "typed_keys":{
+        "type":"boolean",
+        "description":"Specify whether aggregation and suggester names should be prefixed by their respective types in the response"
       }
     }
   }