Do not export Encode/DecodeState but Encode/DecodeSnapshot #29
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
encodeState and decodeState were meant as internal operations for how we serialize states for transmitting them with raft. It turns out, they were not used in the end and instead encodeOp and decodeOp (which are very similar) took their place. As convinience, we used encodeState and decodeState as usable serializable format for snapshots. How snapshots are formatted is something useful to export, so users can work directly with them. But that format should not be tied to how states are [de]serialized for raft (an internal choice), which is what encodeState and decodeState were for in the first place (and might be used for in the future). For example, you could use msgpack for sending states and operations, but simply use json for snapshot formatting. This change allows more freedom if we are ever to change how things are serialized internally vs how snapshots are stored. License: MIT Signed-off-by: Hector Sanjuan <hector@protocol.ai>
ghost
added
the
|
@ZenGround0 I just thought doing the exports this way would be marginally better. It's pedantic to a degree, but definitely more accurate in that we guarantee that those functions are specific for snapshot [de]serialization and not anything else. |
The way were were serializing and de-serializing states and operations was a bit hacky. Because deserialization does not work well on interface types, we passed in a concrete type (pointer to interface) and let Go do magic afterwards. In order to let the users provide their own serialization methods for snapshots, we need to do proper interface wrapping in concrete types (opWrapper, stateWrapper) and serialize/deserialize those instead. On the downside, testing shows states and ops original types should always be pointers. License: MIT Signed-off-by: Hector Sanjuan <hector@protocol.ai>
License: MIT Signed-off-by: Hector Sanjuan <hector@protocol.ai>
|
I have just made this more sophisticate. If the state given has Edit: in order for that to work well I had to fix all the pointer-to-interface hack with explicit wrappers. |
|
yai, I'll tag you a release |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
encodeState and decodeState were meant as internal operations for
how we serialize states for transmitting them with raft.
It turns out, they were not used in the end and instead encodeOp and
decodeOp (which are very similar) took their place.
As convinience, we used encodeState and decodeState as usable
serializable format for snapshots. How snapshots are formatted
is something useful to export, so users can work directly with them.
But that format should not be tied to how states are [de]serialized
for raft (an internal choice), which is what encodeState and
decodeState were for in the first place (and might be used
for in the future).
For example, you could use msgpack for sending states and operations,
but simply use json for snapshot formatting. This change allows more
freedom if we are ever to change how things are serialized internally
vs how snapshots are stored.
License: MIT
Signed-off-by: Hector Sanjuan hector@protocol.ai