[ML] Fix serialising the inference update request#122278
[ML] Fix serialising the inference update request#122278davidkyle merged 2 commits intoelastic:mainfrom
Conversation
davidkyle
added
>bug
:ml
|
Pinging @elastic/ml-core (Team:ML) |
|
Hi @davidkyle, I've created a changelog YAML for you. |
| this.inferenceEntityId = in.readString(); | ||
| this.content = in.readBytesReference(); | ||
| this.taskType = TaskType.fromStream(in); | ||
| this.content = in.readBytesReference(); |
There was a problem hiding this comment.
Surprisingly the fix doesn't require a new transport version. This change means that reading from older nodes will start working and reads from same version and future versions will work
There was a problem hiding this comment.
Making sure I understand, we don't need a new transport version because it was incorrect to start? Will this have any expected impact on serverless?
There was a problem hiding this comment.
The writeTo() code writes taskType then content and the read was reading content then taskType.
My intuition was that a fix would need a new transport version but when I wrote the code it ended up as:
if (version.before(MY_FIX_VERSION)) {
// change the order as this is what write is doing in the old version
this.taskType = TaskType.fromStream(in);
this.content = in.readBytesReference();
} else {
// change the order as this is what write is doing in the current version
this.taskType = TaskType.fromStream(in);
this.content = in.readBytesReference();
}
Both sides of the condition are the same so I took out the transport version check, that suprised me as things are rarely this simple. As for serverless, what was once broken should start working.
kderusso
left a comment
kderusso
left a comment
There was a problem hiding this comment.
Thanks for the quick fix, a couple of questions.
| this.inferenceEntityId = in.readString(); | ||
| this.content = in.readBytesReference(); | ||
| this.taskType = TaskType.fromStream(in); | ||
| this.content = in.readBytesReference(); |
There was a problem hiding this comment.
Making sure I understand, we don't need a new transport version because it was incorrect to start? Will this have any expected impact on serverless?
|
|
||
| import java.io.IOException; | ||
|
|
||
| public class UpdateInferenceModelActionRequestTests extends AbstractWireSerializingTestCase<UpdateInferenceModelAction.Request> { |
There was a problem hiding this comment.
Would we want to use AbstractBWCSerializationTestCase here?
There was a problem hiding this comment.
Because there is on BWC logic on the transport version in the serialisation code the mutateInstanceForVersion() function would have just return the input parameter. I thought it cleaner use the plain AbstractWireSerializingTestCase base class.
4 participants
The read and write were processing variables in a different order.