search

vmware-archive / haret

A strongly consistent distributed coordination system, built using proven protocols & implemented in Rust.
461 stars 18 forks source link

Breaking up TreeOp into multiple messages #118

Open erickt opened 7 years ago

erickt commented 7 years ago

Hello! As part of cleaning up HaretClient::exec, the first thing I'm seeing is it's a bit hard to tell here which operation needs which optional message. Since things are pretty early, I was wondering what you thought about restructuring TreeOp into a series of operation-specific request and response messages, similar to etcd. The advantage here we wouldn't have the ambiguity of doing a blob_put and having the possibility the response containing namespaces.

andrewjstone commented 7 years ago

This sounds reasonable to me. I was never particularly sold 100% on how things are. It would indeed prevent all this optional parsing which seems kinda ridiculous looking at it now. This would be a big win from a client implementation POV.

Just note that the requests still need to get fed into the internal enum representation due to the strongly typed channels used in rabble. See here: https://github.com/vmware/haret/blob/master/src/vr/vr_api_messages.rs#L17

Ideally this message would be lifted out of the VR directory and renamed as well, since it is completely independent of VR. This can be done iteratively of course.

andrewjstone commented 7 years ago

@erickt if you end up doing this work, which would be much appreciated, can you also take care of something that has been bugging me for a while?

The ApiPid in the protobuf file has a different representation from a rabble Pid.

If you could make the ApiPid have the same format as the rabble Pid that would be awesome. Might as well just call it Pid as well.

andrewjstone commented 7 years ago

One more thing to keep in mind. The reason I stuff things into a oneof is because it's impossible to determine which protobuf message is received on the wire since they don't have IDs. Any message with the same fields can be decoded as one or the other, and fields can be skipped. This can be worked around in a number of ways, by having a header that contains a message type int, but then that opens the question of where to define those values since they are no longer protobuf.

Note that I don't think we actually need to combine requests and responses into an ApiMsg as these are clearly intended to be read on different sides of the connection. That appears to be an outright mistake on my part.

Another option within protobuf that doesn't use oneof is described here: https://developers.google.com/protocol-buffers/docs/techniques#union

It looks like at least for some of the messages etcd uses oneof. I can't figure out how they disambiguate other top level messages though. Maybe it has something to do with the rpc. I would particularly like to avoid generating server stubs, and don't even think the rust protobuf compiler does that anyway.

andrewjstone commented 7 years ago

Another thing, which I think is my final comment on this right now: If we decide to allow pipelining of operations, and not just a single outstanding request we will not know what message to expect in response and therefore must still somehow disambiguate. The same thing may also be necessary if we send subscription notifications on the same socket. I think the core problem may not be with the the messages.proto definition, but just with how unrusty the rust-protobuf API is. Another option may be to use this library instead and match on the oneof enums.

Anyway, I'll leave it in your capable hands, I just wanted to brain dump all the thoughts that have been collecting in my head.