Migrate Search requests to use Writeable reading strategies #26428
Conversation
talevy
added
:Core/Infra/Core
| @@ -749,6 +755,19 @@ private static Streamable tryCreateNewInstance(Streamable streamable) throws NoS | |||
| } | |||
| } | |||
|
|
|||
| private static Streamable tryCreateFromStream(Streamable streamable, StreamInput in) throws NoSuchMethodException, | |||
There was a problem hiding this comment.
forgot to add javadocs here. will do that since it is a transitional solution
| assertThat(constructor, Matchers.notNullValue()); | ||
| Streamable newInstance = constructor.newInstance(in); | ||
| return newInstance; | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
I think you should catch the NoSuchMethodException explicitly, otherwise this could mask an exception thrown from the ctor.
There was a problem hiding this comment.
good call. forgot about the good ol IOException
| @@ -688,7 +689,12 @@ public static void assertVersionSerializable(Version version, Streamable streama | |||
| input = new NamedWriteableAwareStreamInput(input, namedWriteableRegistry); | |||
| } | |||
| input.setVersion(version); | |||
| newInstance.readFrom(input); | |||
| // This is here since some Streamables are being converted into Writeables | |||
| // and the readFrom method throws an exception if called | |||
There was a problem hiding this comment.
So instead you first try and deserialize from our normal ctor. Yeah, this makes sense and the way you have it rigged looks about right for a temporary measure.
| @@ -749,6 +755,19 @@ private static Streamable tryCreateNewInstance(Streamable streamable) throws NoS | |||
| } | |||
| } | |||
|
|
|||
| private static Streamable tryCreateFromStream(Streamable streamable, StreamInput in) throws NoSuchMethodException, | |||
| InstantiationException, IllegalAccessException, InvocationTargetException { | |||
There was a problem hiding this comment.
Can you indent this one more time so it doesn't look like method body?
There was a problem hiding this comment.
Actually, I don't think think you throw these exceptions any more since you catch and return null.
There was a problem hiding this comment.
I think you might want to let InvocationTargetException bubble up instead of return null. Those are probably serialization errors.
There was a problem hiding this comment.
Or, better, maybe only return null if there isn't a StreamInput ctor, otherwise you should use it.
I think when we remove the Streamable interface we'd want to remove the null return entirely and require such a constructor here.
There was a problem hiding this comment.
I've updated as Ryan suggested. so now all those are thrown again. And yes, once all are migrated. the original tryCreateNewInstance will be removed
| id = in.readLong(); | ||
| dfs = readAggregatedDfs(in); | ||
| originalIndices = OriginalIndices.readOriginalIndices(in); | ||
| throw new UnsupportedOperationException("usage of Streamable is to be replaced by Writeable"); |
There was a problem hiding this comment.
Can you move writeTo to be under the reading ctor when you do this?
There was a problem hiding this comment.
in general. for all of them? So we have writeTo under the last constructor for all these objects?
There was a problem hiding this comment.
For all of them, yeah. I like having the write and read on close physically so I can look at them at the same time.
There was a problem hiding this comment.
cool. finished moving all of them
|
updated to reflect your comments, thanks! |
| try { | ||
| Class<? extends Streamable> clazz = streamable.getClass(); | ||
| Constructor<? extends Streamable> constructor = clazz.getConstructor(StreamInput.class); | ||
| assertThat(constructor, Matchers.notNullValue()); |
There was a problem hiding this comment.
I don't think you need this assertion. I think it just throws instead, right?
|
thanks @nik9000. I missed some reindex test failures. might come up with a few more file changes to make it green |
* master: Allow abort of bulk items before processing (elastic#26434) [Tests] Improve testing of FieldSortBuilder (elastic#26437) Upgrade to lucene-7.0.0-snapshot-d94a5f0. (elastic#26441) Implement adaptive replica selection (elastic#26128) Build: Quiet bwc build output (elastic#26430) Migrate Search requests to use Writeable reading strategies (elastic#26428) Changed version from 7.0.0-alpha1 to 6.1.0 in the nested sorting serialization check. Remove dead path conf BWC code in build
This is a continuation of the Streamable -> Writeable migration. This PR focuses on
a few of the SearchRequest-related actions.