Add persistence encoding and decoder interfaces #3534
Conversation
| } | ||
|
|
||
| // Decoder is used to deserialize from persistence data blobs | ||
| Decoder interface { |
There was a problem hiding this comment.
Nit: not a blocker. I personally think we save one interface if combing them as Encoder. As people(at least me) usually understand encoder can do decoding. This might be useful in some coding experience.
There was a problem hiding this comment.
Updated to do this.
| } | ||
|
|
||
| // Decoder is used to deserialize from persistence data blobs | ||
| Decoder interface { |
There was a problem hiding this comment.
Encoder & Decoder interfaces should be symmetric. While there is an encoder type per format and totally separate implementations, there is only one decoder type that requires an encoding format parameter for each decode function. We should make these interfaces symmetric. I like the factory pattern that you used for the encoder.
I'd also consider keeping encoder and decoder for a format in the same type instead of defining two types per format.
There was a problem hiding this comment.
Spoke offline and addressed comment in a slightly different way.
| return fmt.Errorf("error deserializing blob to struct: %v", err) | ||
| } | ||
|
|
||
| func thriftRWEncode(t thriftRWType) (p.DataBlob, error) { |
There was a problem hiding this comment.
I'd move all thrift-specific functions to the thrift encoder/decoder type and keep this file only for common utils.
common/persistence/sql/common.go
Outdated
| logger log.Logger | ||
| db sqlplugin.DB | ||
| logger log.Logger | ||
| encoder serialization.Encoder |
There was a problem hiding this comment.
Related to my comment above: Is there any reason for keeping encoder and decoder interfaces separate? I think this separation bloats the code a little bit and leaves the door open for using incompatible encoder & decoder pairs in the future. It seems like you can just have one parser that handles both encoding and decoding.
andrewjdawson2016
force-pushed
the
SerializationInterface
branch
from
6e85445 to
6a70d24
Compare
This PR introduces the plumbing needed to serialize persistence structs as proto. It introduces a new encoder and decoder interface. Based on config the cluster can either use the proto encoder or the thrift encoder. The decoder will always be able to operate on both proto and thrift blobs. The proto encoder implementation will take as input a thrift strcut, convert it to a proto struct and then serialize it using proto.