search

vmware-archive / haret

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

Convert actor `Msg` to use protobuf instead of msgpack #90

Closed andrewjstone closed 7 years ago

andrewjstone commented 7 years ago

This allows for backwards and forwards compatibility since it no longer performs auto-serialization with rustc_serialize.

https://github.com/andrewjstone/rabble/issues/16 https://github.com/andrewjstone/rabble/pull/17

andrewjstone commented 7 years ago

I've done the bulk of this conversion in the convert-msg-to-protobuf branch. It ended up being a TON of error-prone code for manual conversion and it has me questioning whether this is the right way to do things. It doesn't even compile yet.

Currently I'm leaning toward not merging this code and instead using https://serde.rs/ which went 1.0 yesterday. This will obviate the need for all this manual code, and I think still provides the features required for upgrade/downgrade unlike rustc_serialize. Furthermore, using serde (with msgpack) will allow messages larger than 1mb (whether it's a good idea or not) and will likely be faster than protobuf due to less allocations.

The reasons I think serde is good enough for backwards and forwards compatibility are as follows:

  1. serde will ignore unknown fields in structs by default (backwards compatibility)
  2. serde allows default values for missing fields (forward compatibility).
  3. serde also allows manual serialization when needed.

Finally, the big impetus for using protobuf besides the struct field forward/backward compatibility was adding new sub-messages as enum variants. However, deserializing these should actually fail if they are received by an old node, as the new variant isn't understood. This is actually what my manual protobuf code does now.

Many of the changes merged in https://github.com/andrewjstone/rabble/pull/17 are still good, and it is probably fine to keep them as is. They allow the users of rabble to serialize however they want rather than forcing them to use rustc_serialize. While the same backwards/forwards compatibility arguments likely hold for rabble as they do for haret, it should be fine keeping things as is for now. We can always change to using serde later if it becomes a good idea. This issue is only about the changes to haret. Any changes to haret using serde or protobuf should still build on the changes to rabble in https://github.com/andrewjstone/rabble/pull/17.

I'm going to put this issue on the backburner for now, as it isn't urgent. I have higher priorities, and this still needs mulling over.

andrewjstone commented 7 years ago

Closing this, as the decision right now is to use Serde. Rabble changes using protobuf were reverted and serde is going to be implemented for https://github.com/andrewjstone/rabble/issues/19