serialize suggestion responses as named writeables #30284

andyb-elastic · 2018-05-01T01:13:29Z

This is still in-progress but I wanted to get it up for some feedback. I think the exact location of where some things (e.g. response fields) should go in the class hierarchy still needs to be cleaned up, as well as xcontent generation/parsing. The test coverage also probably could use some work.

The first stage of this will make suggestion responses named writeables and adapt CustomSuggesterIT accordingly, and then the second stage will move that test to an example plugin and make it a rest integration test.

This is related to #26585

Suggestion responses were previously serialized as streamables which
made writing suggesters in plugins with custom suggestion response types
impossible. This commit makes them serialized as named writeables and
provides a facility for registering a reader for suggestion responses
when registering a suggester.

This also makes Suggestion responses abstract, requiring a suggester
implementation to provide its own types. Suggesters which do not need
anything additional to what is defined in Suggest.Suggestion should
provide a minimal subclass.

Suggestion responses were previously serialized as streamables which made writing suggesters in plugins with custom suggestion response types impossible. This commit makes them serialized as named writeables and provides a facility for registering a reader for suggestion responses when registering a suggester. This also makes Suggestion responses abstract, requiring a suggester implementation to provide its own types. Suggesters which do not need anything additional to what is defined in Suggest.Suggestion should provide a minimal subclass.

elasticmachine · 2018-05-01T19:56:48Z

Pinging @elastic/es-search-aggs

jimczi

Thanks @andyb-elastic , I left some comments.

jimczi · 2018-05-02T13:56:54Z

server/src/main/java/org/elasticsearch/plugins/SearchPlugin.java

+            addSuggestionReader(name, suggestionReader);
+        }
+
+        public SuggesterSpec addSuggestionReader(String writeableName, Writeable.Reader<? extends Suggest.Suggestion> reader) {


Is it needed to allow multiple readers ? Only one custom suggestion implementation should be needed here ?

My thinking here was that it wouldn't be unreasonable for a suggester to return multiple types of suggestions in different situations. For example it looks like some aggregations do this if I'm not mistaken. But it makes sense and is simpler if they only use one, so I'll do that

jimczi · 2018-05-02T14:06:01Z

server/src/test/java/org/elasticsearch/search/suggest/CustomSuggesterSearchIT.java

@@ -135,7 +149,7 @@ protected XContentBuilder innerToXContent(XContentBuilder builder, Params params

        @Override
        public String getWriteableName() {
-            return "custom";
+            return SUGGESTION_NAME;
        }

        @Override


Is it possible to move this test and the custom suggestion impl in the plugin examples and adds rest tests that checks that xcontent parsing works ? I am not sure that this IT test is enough to validate the plugin integration.
This would be nice to have a real example that shows how to write a custom suggester and the example plugins are the perfect fit for this.

I agree on both, that this doesn't completely validate the plugin integration, and that it would be preferable to have it in an example plugin. I was going to make that a follow up PR but I'll add it to this one

Thanks, +1 to add the example plugin in this pr.

…ttempt-3

Currently works with the Option dummy field, but Entry and Suggestion dummy fields don't make it into xcontent responses right now

andyb-elastic · 2018-07-25T00:24:35Z

Commits I just pushed do the following

Suggester spec only registers one reader for its Suggestion response
Move custom suggester to example plugin
Add yaml rest test for example plugin that mirrors the existing CustomSuggesterSearchIT

Things I need to do next

Change Suggest's xcontent handling so that custom fields in Suggestion and Entry can be written (I'm not 100% sure we want to do this but it seems pretty reasonable for users to expect that they can extend this)
Remove the old custom suggester tests in server-test

to XContentFragments

…ttempt-3

andyb-elastic · 2018-07-26T02:32:25Z

I added a test for the ability to add custom fields for Entry subclasses. Also changed Suggestion/Entry/Option so that they're all xcontent fragments and their subclasses can add fields by overriding #toXContent

@jimczi mind taking another look?

jimczi

It looks great @andyb-elastic ! I left some comments regarding serializations and BWC but otherwise I think it's ready. Can you change the target version to 6.5 and add an entry in the breaking changes ?

jimczi · 2018-07-26T19:38:08Z

server/src/main/java/org/elasticsearch/search/suggest/Suggest.java

@@ -92,6 +90,42 @@ public Suggest(List<Suggestion<? extends Entry<? extends Option>>> suggestions)
        this.hasScoreDocs = filter(CompletionSuggestion.class).stream().anyMatch(CompletionSuggestion::hasScoreDocs);
    }

+    public Suggest(StreamInput in) throws IOException {
+        // in older versions, Suggestion types were serialized as Streamable
+        if (in.getVersion().before(Version.V_6_4_0)) {


you need to start with 7.0 and change it when this is backported to 6.5 ?

jimczi · 2018-07-26T19:38:50Z

server/src/main/java/org/elasticsearch/search/suggest/Suggest.java

+                        suggestion = new CompletionSuggestion(in);
+                        break;
+                    case 2: // CompletionSuggestion.TYPE
+                        throw new IllegalArgumentException("Completion suggester 2.x is not supported anymore");


I think we can remove this, we don't support 2.x here (nor in 6.x).

jimczi · 2018-07-26T19:39:06Z

server/src/main/java/org/elasticsearch/search/suggest/Suggest.java

-            out.writeVInt(command.getWriteableType());
-            command.writeTo(out);
+        // in older versions, Suggestion types were serialized as Streamable
+        if (out.getVersion().before(Version.V_6_4_0)) {


Same here, version should be 7 for now

Raise compatibility version to 7.0, remove serialization for the 2.x version of completion suggestions

…ttempt-3

andyb-elastic · 2018-07-31T16:28:36Z

It looks like there's a bug with serialization across mixed version clusters in the current state of this PR, I'm working on it

Nodes using the old serialization have an innerReadFrom/WriteTo scheme for Suggestion objects and therefore write/read all of their one-off members before writing/reading their suggestion entries. This idiom makes it a little harder to follow and doesn't look like it survived the transition from Streamable to Writeable. This change makes it so that suggestions are still serialized in this old older when talking to older nodes, while using inheritance of just the single <init>(StreamInput) and writeTo(StreamOutput) methods for newer nodes

do this serialization as an obvious hack instead of leaving behind deprecated innerReadFrom/WriteTo methods which could have an ambiguous intent

andyb-elastic · 2018-08-01T16:58:37Z

Jenkins run gradle build tests please

andyb-elastic · 2018-08-01T22:31:16Z

Jenkins run gradle build tests please

andyb-elastic · 2018-08-02T00:01:52Z

Jenkins run gradle build tests please

More complicated equals behavior for options makes things start acting weird because the entry reduce code relies on them being equal if they have the same text. Arguably better to fix the entry reduce code but I'm not sure where else may rely on this behavior

…ttempt-3

andyb-elastic · 2018-08-03T00:35:10Z

@jimczi I pushed the serialization tests we talked about

…ttempt-3

andyb-elastic · 2018-08-06T17:08:14Z

Jenkins run gradle build tests please

jimczi

I left a minor comment regarding the serialization tests but LGTM otherwise. The change is relatively big so let's merge but we still need a follow up with a real BWC rest test.

jimczi · 2018-08-06T17:13:31Z

server/src/test/java/org/elasticsearch/search/suggest/SuggestTests.java

+
+    public void testSerialization() throws IOException {
+        final Version bwcVersion = VersionUtils.randomVersionBetween(random(),
+            Version.CURRENT.minimumCompatibilityVersion(), Version.CURRENT);


We can test the versions before the change explicitly ?

My thinking was that writing it like this, it will still make sense regardless of what the current version is. For example, there are plenty of tests like this hanging around that explicitly test versions that are now outside of the compatibility range, and I was thinking this is a way of avoiding that

andyb-elastic · 2018-08-07T20:28:14Z

Jim and I decided that we don't want to backport this to 6.x because the migration path for plugin authors is a little more involved than I'd originally thought - specifically that authors for who plugin suggesters are currently actually usable are likely using the plain Suggest.Suggestion response type, which this PR makes abstract

andyb-elastic added >feature :Search Relevance/Suggesters "Did you mean" and suggestions as you type v7.0.0 v6.4.0 labels May 1, 2018

andyb-elastic requested a review from jimczi May 1, 2018 01:13

hub-cap added :Search Relevance/Suggesters "Did you mean" and suggestions as you type and removed :Search Relevance/Suggesters "Did you mean" and suggestions as you type labels May 1, 2018

checkstyle fixes

990d546

jimczi requested changes May 2, 2018

View reviewed changes

andyb-elastic added 4 commits July 18, 2018 11:51

Merge branch 'master' into feature-plugin-suggester-named-writeable-a…

09b3ccc

…ttempt-3

wip suggest update tests with new option usage

4f695a7

wip suggest only one suggestion reader

155df07

rest tests for custom suggester example plugin

3add813

Currently works with the Option dummy field, but Entry and Suggestion dummy fields don't make it into xcontent responses right now

andyb-elastic added 5 commits July 25, 2018 14:39

checkstyle fixes

f062443

fill in toXContent support for custom fields - change Entry and Option

5b8fa25

to XContentFragments

remove custom suggester IT from server

4e5ff17

Merge branch 'master' into feature-plugin-suggester-named-writeable-a…

80ce511

…ttempt-3

use an empty test suggester impl in SearchModuleTests

69163ba

jimczi approved these changes Jul 26, 2018

View reviewed changes

andyb-elastic added 3 commits July 27, 2018 14:28

backwards compatiblity corrections

bcd3704

Raise compatibility version to 7.0, remove serialization for the 2.x version of completion suggestions

make release note more clear

87bddd1

Merge branch 'master' into feature-plugin-suggester-named-writeable-a…

a7c3f01

…ttempt-3

andyb-elastic added 2 commits July 31, 2018 14:32

serialization shim for TermSuggestion

0059b61

do this serialization as an obvious hack instead of leaving behind deprecated innerReadFrom/WriteTo methods which could have an ambiguous intent

andyb-elastic added 4 commits August 1, 2018 19:10

wip round triup works but only because it's not equalsing

a2b7bef

serialization tests pass

df532f1

More complicated equals behavior for options makes things start acting weird because the entry reduce code relies on them being equal if they have the same text. Arguably better to fix the entry reduce code but I'm not sure where else may rely on this behavior

remove commented out methods

ee7efb4

Merge branch 'master' into feature-plugin-suggester-named-writeable-a…

b77ed84

…ttempt-3

Merge branch 'master' into feature-plugin-suggester-named-writeable-a…

4603a0d

…ttempt-3

jimczi approved these changes Aug 6, 2018

View reviewed changes

andyb-elastic merged commit 8bfb0f3 into elastic:master Aug 7, 2018

andyb-elastic removed the v6.4.0 label Aug 7, 2018

andyb-elastic mentioned this pull request Aug 7, 2018

Bug: Suggest request for a CustomSuggester fail with "ReduceSearchPhaseException" in Multi-DataNode scenarios. #26585

Closed

jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

cbuescher mentioned this pull request Oct 2, 2020

Remove unused deprecated serialization code #63166

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

serialize suggestion responses as named writeables #30284

serialize suggestion responses as named writeables #30284

andyb-elastic commented May 1, 2018

elasticmachine commented May 1, 2018

jimczi left a comment

jimczi May 2, 2018

andyb-elastic May 2, 2018

jimczi May 2, 2018

jimczi May 2, 2018

andyb-elastic May 2, 2018

jimczi May 2, 2018

andyb-elastic commented Jul 25, 2018

andyb-elastic commented Jul 26, 2018

jimczi left a comment

jimczi Jul 26, 2018

jimczi Jul 26, 2018

jimczi Jul 26, 2018

andyb-elastic commented Jul 31, 2018

andyb-elastic commented Aug 1, 2018

andyb-elastic commented Aug 1, 2018

andyb-elastic commented Aug 2, 2018

andyb-elastic commented Aug 3, 2018

andyb-elastic commented Aug 6, 2018

jimczi left a comment

jimczi Aug 6, 2018

andyb-elastic Aug 6, 2018

andyb-elastic commented Aug 7, 2018

serialize suggestion responses as named writeables #30284

serialize suggestion responses as named writeables #30284

Conversation

andyb-elastic commented May 1, 2018

elasticmachine commented May 1, 2018

jimczi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyb-elastic commented Jul 25, 2018

andyb-elastic commented Jul 26, 2018

jimczi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyb-elastic commented Jul 31, 2018

andyb-elastic commented Aug 1, 2018

andyb-elastic commented Aug 1, 2018

andyb-elastic commented Aug 2, 2018

andyb-elastic commented Aug 3, 2018

andyb-elastic commented Aug 6, 2018

jimczi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyb-elastic commented Aug 7, 2018