Transport: allow to de-serialize arbitrary objects given their name #12393
Conversation
This commit makes it possible to serialize arbitrary objects by having them extend Writeable. When reading them though, we need to be able to identify which object we have to create, based on its name. This is useful for queries once we move to parsing on the coordinating node, as well as with aggregations and so on. Introduced a new abstraction called NamedWriteable, which is supported by StreamOutput and StreamInput through writeNamedWriteable and readNamedWriteable methods. A new NamedWriteableRegistry is introduced also where named writeable prototypes need to be registered so that we are able to retrieve the proper instance of the writeable given its name and then de-serialize it calling readFrom against it.
|
LGTM but I'm not too familiar with the Netty stuff so might be worth getting someone to look at that bit. I don't have a strong opinion on the Throwable stuff but it might be nice if everything uses the same mechanism. |
|
maybe @jpountz can have a look? |
I like the current logic as it prevents from being tempted to make exception types pluggable? |
| /** | ||
| * Returns the name of the writeable object | ||
| */ | ||
| String getWriteableName(); |
There was a problem hiding this comment.
it is getName on the query-refactoring branch, but then classes that implement this interface might already have a getName method (e.g. aggs), which is why I went for this more specific method name.
|
I'm a bit concerned that this pull request adds coupling between StreamInput and NamedWriteableRegistry on one hand, and transport modules and NamedWriteableRegistry on the other hand. Could we only have this NamedWriteableRegistry handling in a subclass of StreamInput so that StreamInput would remain unaware of NamedWriteableRegistry? Also I think it would be cleaner if every component maintained its own registry (could be eg. one for queries, one for aggs, etc.) instead of sharing a single one for everything? |
|
Replaced by #12571 . |
This commit makes it possible to serialize arbitrary objects by having them extend Writeable. When reading them though, we need to be able to identify which object we have to create, based on its name. This is useful for queries once we move to parsing on the coordinating node, as well as with aggregations and so on. Introduced a new abstraction called NamedWriteable, which is supported by StreamOutput and StreamInput through writeNamedWriteable and readNamedWriteable methods. A new NamedWriteableRegistry is introduced also where named writeable prototypes need to be registered so that we are able to retrieve the proper instance of the writeable given its name and then de-serialize it calling readFrom against it. Closes elastic#12393
This commit makes it possible to serialize arbitrary objects by having them extend Writeable. When reading them though, we need to be able to identify which object we have to create, based on its name. This is useful for queries once we move to parsing on the coordinating node, as well as with aggregations and so on.
Introduced a new abstraction called NamedWriteable, which is supported by StreamOutput and StreamInput through writeNamedWriteable and readNamedWriteable methods. A new NamedWriteableRegistry is introduced also where named writeable prototypes need to be registered so that we are able to retrieve the proper instance of the writeable given its name and then de-serialize it calling readFrom against it.
Note that this same change was previously reviewed and pushed to the query-refactoring branch (#11553), the goal of this PR is to backport the change to master.
The main question is whether we should use this mechanism for exceptions or not. It seems like it would require a class per exception, maybe a bit too verbose compared to the switch that we have in
StreamInput#readThrowableandStreamOutput#writeThrowable. Looking for a second opinion there.