Fix compression timestamps#759
Conversation
eapache
left a comment
eapache
left a comment
There was a problem hiding this comment.
Thanks for this! I believe your logic is correct but it feels a bit complicated.
Per my inline comment, I think it might be better to always use v1 messages if the version supports it, and set the timestamp to time.Now() if the user does not provide one. This should avoid the need to loop over all the messages to determine the correct outer protocol version, plus it provides a sane default for the timestamp.
| } | ||
| if ps.parent.conf.Version.IsAtLeast(V0_10_0_0) { | ||
| // Compressed messages must use a protocol version | ||
| // that is newer than the inner messages version. |
There was a problem hiding this comment.
minor nit-pick: as I understand it (and as you've implemented here) the outer version has to be at least as new as the inner version, not strictly newer.
| for _, msgBlock := range set.setToSend.Messages { | ||
| msg := msgBlock.Msg | ||
| if msg.Version > compMsg.Version { | ||
| compMsg.Version = msg.Version |
There was a problem hiding this comment.
Hmm, I was thinking about better ways to accomplish this and I wonder now if the code in produceSet#add is even correct; is it safe to mix v0 and v1 messages in a single compressed batch which we'll do right now if some timestamps are 0? Or should we change to just always using v1 if the version supports it, and let the timestamps be 0 if necessary?
edit: actually this issue goes away if we provide a timestamp of time.Now() by default if you leave the timestamp as 0, which we should probably do for convenience anyway
There was a problem hiding this comment.
I think always using v1 would be great, I don't see a reason to switch back at all. Providing a default of time.Now() sounds like a good idea, too.
|
@eapache how about the updated version? Pushing message version 1 if possible and default timestamp of time.Now if unspecified? |
| } | ||
| if ps.parent.conf.Version.IsAtLeast(V0_10_0_0) { | ||
| compMsg.Version = 1 | ||
| compMsg.Timestamp = time.Now() |
There was a problem hiding this comment.
This should probably be set.setToSend.Messages[0].Msg.Timestamp so the outer message timestamp is approximately correct if the user provides custom timestamps for the inner messages?
|
Definitely like this version better, thanks for adjusting it. Just one minor comment. |
The first messages timestamp of a set is a proxy for the actual timestamp of the group in many cases.
|
@eapache you are right, the first timestamp would usually be a good proxy for the timestamp of the group. I've updated the PR, I guess it needs a squash but github can do that on merge anyway. |
|
Thanks! |
3 participants
The wrapper message has to use the "new" format if the compressed messages use it as well.
This will fix #757 #758