Support JSON request body in scroll, clear scroll, and analyze APIs #9076

johtani · 2014-12-27T04:22:46Z

Add json support to scroll, clear scroll, and analyze
And change documents

martijnvg · 2015-01-06T13:08:23Z

src/main/java/org/elasticsearch/rest/action/admin/indices/analyze/RestAnalyzeAction.java

@@ -64,4 +73,58 @@ public void handleRequest(final RestRequest request, final RestChannel channel,
        analyzeRequest.charFilters(request.paramAsStringArray("char_filters", analyzeRequest.charFilters()));
        client.admin().indices().analyze(analyzeRequest, new RestToXContentListener<AnalyzeResponse>(channel));
    }
+
+    public static void parseContent(AnalyzeRequest request, String text, BytesReference content) {


Maybe the the parsing logic should be in TransportAnalyzeAction instead? This way both rest and regular transport level requests deal with the same parameter parsing logic.

The AnalyzeRequest just holds a source field , like search and percolate request. To make usage easier from transport client, there can be a AnalyzeSourceBuilder (similar to SearchSourceBuilder).

@martijnvg Question about BWC.

Your comment means that we remove other fields from AnalyzeRequest and add a source field, right?
If we do that, I think we will break the backward compatibility.
Is it correct?

martijnvg · 2015-01-06T13:15:30Z

@johtani Thanks for opening this PR! I left a couple of comments regarding the fact that it seems better to do the parsing that is now in the rest layer in the transport action layer.

johtani · 2015-01-14T08:41:13Z

@martijnvg I pushed some changes.

The parsing move to transport action layer
Add AnalyzeSourceBuilder and SearchScrollSourceBuilder

I have a question about SearchScrollRequest.
It is difficult to remove scrollId and scroll because SearchScrollRequest is used in TransportSearchScrollQueryAndFetchAction and TransportSearchScrollQueryThenFetchAction.
Now, I re-use SearchScrollRequest in TransportSearchScrollAction.
Do you have any idea?

martijnvg · 2015-01-19T10:06:10Z

src/main/java/org/elasticsearch/action/search/TransportSearchScrollAction.java

@@ -78,6 +84,34 @@ protected void doExecute(SearchScrollRequest request, ActionListener<SearchRespo
        }
    }

+    private void parseSource(SearchScrollRequest request) {
+        if (request.source() != null) {


I think we should have a helper method that does the scroll id parsing. Now clear scroll and scroll apis have dedicated parsing logic. Even though the parsing logic is slightly differently (array vs. single value), it would be nice to reuse the parsing logic. The parsing helper method should support parsing a single scroll id and multiple scroll ids.

@martijnvg sorry, late reply. I push the change that I move parse methods to Helper class.
Does it make sense?

martijnvg · 2015-01-19T10:32:17Z

@johtani I see, yes removing that option would make things more complicated.

I think we should then keep the scrollId and scroll fields on the SearchScrollRequest class. But then in TransportSearchScrollAction we should just parse the source and and set these option on the SearchScrollRequest class, so that the other subsequent actions should only use these options. These fields on the SearchScrollRequest should be internal/private for these other subsequent actions, even though these are public options. Any other usages should use the scroll source build instead.

Regarding to the rest of the PR: the last change looks good and I left one comment.

s1monw · 2015-03-16T01:54:55Z

@martijnvg are you picking this up?

johtani · 2015-03-18T15:40:21Z

@martijnvg Rebased a branch. Please review it.

martijnvg · 2015-03-18T17:39:43Z

src/main/java/org/elasticsearch/action/search/ClearScrollRequest.java


 /**
 */
 public class ClearScrollRequest extends ActionRequest<ClearScrollRequest> {

-    private List<String> scrollIds;
+    private BytesReference source;
+    private boolean unsafe;


the unsafe field can be removed from ClearScrollRequest, because in TransportClearScrollAction we don't fork the request. Everything happens on the calling thread.

martijnvg · 2015-03-18T18:11:09Z

@johtani looks good. I left a couple of comments. Can you also squash the existing commits into a single commit?

johtani · 2015-03-20T07:39:03Z

@martijnvg Thanks. Add comment and remove unsafe property. And squash into a single commit.

rjernst · 2015-03-20T22:57:00Z

src/main/java/org/elasticsearch/action/admin/indices/analyze/AnalyzeRequest.java

-
-    private String field;
+    private BytesReference source;
+    private boolean unsafe;


Is this "unsafe" member really needed? Aside from having a very confusing name, can't we just check source == null?

Nevermind, I see why it is "unsafe" now. But do we really need to have all these crazy options on the request? Why can't we just always copy?

I think we can. The reusing of the request body has always been there and it used in other apis too, maybe instead we should open a different issue and discuss if we should drop this? This would simplify the code and remove a potential pitfall (the unsafe support isn't a straightforward pattern)

martijnvg · 2015-03-23T05:56:34Z

@johtani LGTM. I think the point @rjernst raised about unsafe in general should be discussed in a different issue.

johtani · 2015-03-23T07:16:50Z

merged 16083d4

bleskes · 2015-03-23T07:51:04Z

awesome @johtani

bleskes · 2015-03-23T12:09:20Z

@johtani sorry, I had to back this one out because it failed many client tests . Not saying it's not good but we should consider adding a BWC layer. Will reach out to discuss this. I'm re-opening this PR to make sure it's not lost.

bleskes · 2015-04-20T06:53:00Z

src/main/java/org/elasticsearch/rest/action/admin/indices/analyze/RestAnalyzeAction.java

+                        analyzeRequest.field(parser.text());
+                    } else if ("tokenizer".equals(currentFieldName) && token == XContentParser.Token.VALUE_STRING) {
+                        analyzeRequest.tokenizer(parser.text());
+                    }else if (("token_filters".equals(currentFieldName) || "filters".equals(currentFieldName)) && token == XContentParser.Token.START_ARRAY) {


extra space

bleskes · 2015-04-20T07:00:28Z

Thx @johtani , this looks great. I left some minor comments...

Change analyze.asciidoc and scroll.asciidoc Remove a constructor with two param in AnalyzeRequest Fix exception messages Closes elastic#5866

johtani · 2015-04-21T05:08:48Z

Thanks for your comments, @bleskes
I change analyzer.asciidoc and scroll.asciidoc, please check my English...
And fix the message of exception and remove AnalyzeRequest constructor with two parameters.

bleskes · 2015-04-21T06:27:40Z

rest-api-spec/test/scroll/10_basic.yaml

@@ -131,3 +130,63 @@
      clear_scroll:
        scroll_id: $scroll_id

+---
+"Basic scroll orverride param by JSON":


maybe call this "Body params override query string"?

bleskes · 2015-04-21T06:43:45Z

Left some documentation comments. Other than that, LGTM

Fix documentations Closes elastic#5866

johtani · 2015-04-21T10:10:30Z

Pushed. Fix documentations

bleskes · 2015-04-22T07:26:10Z

LGTM. Thx

johtani · 2015-04-22T08:24:28Z

@bleskes Thanks. Should I get LGTM one more?

bleskes · 2015-04-22T08:27:43Z

I think it's not needed. Up to you.

On Wed, Apr 22, 2015 at 10:24 AM, Jun Ohtani notifications@github.com
wrote:

@bleskes Thanks. Should I get LGTM one more?

Reply to this email directly or view it on GitHub:
#9076 (comment)

johtani · 2015-04-22T09:12:00Z

merged!

johtani added :Core/Infra/REST API REST infrastructure and utilities >enhancement v2.0.0-beta1 review labels Dec 27, 2014

martijnvg reviewed Jan 6, 2015
View reviewed changes

martijnvg reviewed Jan 19, 2015
View reviewed changes

martijnvg self-assigned this Jan 22, 2015

drewr force-pushed the master branch from dcc3da0 to 7c20a8a Compare February 20, 2015 16:48

johtani force-pushed the fix/support_json_scroll_and_analyze_api branch from 4de5abb to 6787a5e Compare March 18, 2015 15:36

martijnvg reviewed Mar 18, 2015
View reviewed changes

johtani force-pushed the fix/support_json_scroll_and_analyze_api branch from 6787a5e to 5cdc347 Compare March 20, 2015 07:36

rjernst reviewed Mar 20, 2015
View reviewed changes

johtani closed this Mar 23, 2015

bleskes reviewed Apr 20, 2015
View reviewed changes

Rest: Add json in request body to scroll, clear scroll, and analyze API

4f055c6

Change analyze.asciidoc and scroll.asciidoc Remove a constructor with two param in AnalyzeRequest Fix exception messages Closes elastic#5866

bleskes reviewed Apr 21, 2015
View reviewed changes

Rest: Add json in request body to scroll, clear scroll, and analyze API

771d26d

Fix documentations Closes elastic#5866

johtani closed this Apr 22, 2015

kevinkluge removed the review label Apr 22, 2015

clintongormley changed the title ~~Rest: Add json in request body to scroll, clear scroll, and analyze API~~ Support JSON request body in scroll, clear scroll, and analyze APIs Jun 7, 2015

johtani deleted the fix/support_json_scroll_and_analyze_api branch April 25, 2016 00:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support JSON request body in scroll, clear scroll, and analyze APIs #9076

Support JSON request body in scroll, clear scroll, and analyze APIs #9076

johtani commented Dec 27, 2014

martijnvg Jan 6, 2015

johtani Jan 8, 2015

martijnvg commented Jan 6, 2015

johtani commented Jan 14, 2015

martijnvg Jan 19, 2015

johtani Feb 4, 2015

martijnvg commented Jan 19, 2015

s1monw commented Mar 16, 2015

johtani commented Mar 18, 2015

martijnvg Mar 18, 2015

martijnvg commented Mar 18, 2015

johtani commented Mar 20, 2015

rjernst Mar 20, 2015

rjernst Mar 20, 2015

martijnvg Mar 23, 2015

martijnvg commented Mar 23, 2015

johtani commented Mar 23, 2015

bleskes commented Mar 23, 2015

bleskes commented Mar 23, 2015

bleskes Apr 20, 2015

bleskes commented Apr 20, 2015

johtani commented Apr 21, 2015

bleskes Apr 21, 2015

bleskes commented Apr 21, 2015

johtani commented Apr 21, 2015

bleskes commented Apr 22, 2015

johtani commented Apr 22, 2015

bleskes commented Apr 22, 2015

@bleskes Thanks. Should I get LGTM one more?

johtani commented Apr 22, 2015

Support JSON request body in scroll, clear scroll, and analyze APIs #9076

Support JSON request body in scroll, clear scroll, and analyze APIs #9076

Conversation

johtani commented Dec 27, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martijnvg commented Jan 6, 2015

johtani commented Jan 14, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martijnvg commented Jan 19, 2015

s1monw commented Mar 16, 2015

johtani commented Mar 18, 2015

Choose a reason for hiding this comment

martijnvg commented Mar 18, 2015

johtani commented Mar 20, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martijnvg commented Mar 23, 2015

johtani commented Mar 23, 2015

bleskes commented Mar 23, 2015

bleskes commented Mar 23, 2015

Choose a reason for hiding this comment

bleskes commented Apr 20, 2015

johtani commented Apr 21, 2015

Choose a reason for hiding this comment

bleskes commented Apr 21, 2015

johtani commented Apr 21, 2015

bleskes commented Apr 22, 2015

johtani commented Apr 22, 2015

bleskes commented Apr 22, 2015

@bleskes Thanks. Should I get LGTM one more?

johtani commented Apr 22, 2015