Compress digest#193
Conversation
guilload
force-pushed
the
guilload/compress-digest
branch
6 times, most recently
from
e7c4bf9 to
3831f08
Compare
| } | ||
|
|
||
| fn serialized_len(&self) -> usize { | ||
| if self.compress { |
There was a problem hiding this comment.
This is not ideal but we already follow the same "pattern" for the Delta. Will fix if it starts showing up on CPU profiles.
guilload
force-pushed
the
guilload/compress-digest
branch
from
3831f08 to
8645943
Compare
| let mut stream_writer = | ||
| CompressedStreamWriter::with_block_threshold(DIGEST_BLOCK_THRESHOLD); | ||
| for (chitchat_id, node_digest) in &self.node_digests { | ||
| stream_writer.append(chitchat_id); |
There was a problem hiding this comment.
The API between CompressedStreamWriter and deserialize_stream might be needlessly asymetric.
If we do no need the flexibility, we could have a typed CompressedStreamWriter<(ChitchatId, NodeDigest)> where we append chitchat_id and node_digest together.
| let self_digest = self.compute_digest(&scheduled_for_deletion); | ||
| let delta_mtu = MAX_UDP_DATAGRAM_PAYLOAD_SIZE - 1 - self_digest.serialized_len(); | ||
| let delta_mtu = MAX_UDP_DATAGRAM_PAYLOAD_SIZE | ||
| - ChitchatMessage::HEADER_LEN |
There was a problem hiding this comment.
I don't think this is correct, the MAGIC number and version you added are included in serialized_len for instance
| let delta = self.cluster_state.compute_partial_delta_respecting_mtu( | ||
| &digest, | ||
| MAX_UDP_DATAGRAM_PAYLOAD_SIZE - 1, | ||
| MAX_UDP_DATAGRAM_PAYLOAD_SIZE - ChitchatMessage::HEADER_LEN, |
There was a problem hiding this comment.
no there a no Syn in the message and HEADER_LEN include the syn magic/version.
|
ChitchatMessage has versioning. I suggest we rely on that instead of having versioning and magic number for each of its component |
guilload
force-pushed
the
guilload/compress-digest
branch
2 times, most recently
from
30b7cb7 to
b3730fd
Compare
guilload
force-pushed
the
guilload/compress-digest
branch
from
b3730fd to
3a6c200
Compare
| /// reconciliation algorithm and its own digest. | ||
| SynAck { digest: Digest, delta: Delta }, | ||
| SynAck { | ||
| protocol_version: ProtocolVersion, |
There was a problem hiding this comment.
I think it might be better to separate the version from the model.
struct VersionedChitchatMessage {
version: ProtocolVersion,
message: ChitchatMessage,
}(I said enum in the call, that was dumb).
There was a problem hiding this comment.
We might want to remove the serialize bullshit to a separate module if it is getting larger with the different version too.
fulmicoton-dd
left a comment
fulmicoton-dd
left a comment
There was a problem hiding this comment.
LGTM but I think it would be worth doing putting version outside of the model
3 participants
No description provided.