Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix compression timestamps #759

Merged
merged 4 commits into from
Oct 6, 2016
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
22 changes: 20 additions & 2 deletions produce_set.go
Original file line number Diff line number Diff line change
Expand Up @@ -90,11 +90,29 @@ func (ps *produceSet) buildRequest() *ProduceRequest {
Logger.Println(err) // if this happens, it's basically our fault.
panic(err)
}
req.AddMessage(topic, partition, &Message{
compMsg := &Message{
Codec: ps.parent.conf.Producer.Compression,
Key: nil,
Value: payload,
})
}
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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// Due to a lack of better timestamp notation copy the oldest
// (earliest) timestamp to message.
for _, msgBlock := range set.setToSend.Messages {
msg := msgBlock.Msg
if msg.Version > compMsg.Version {
compMsg.Version = msg.Version
Copy link
Contributor

@eapache eapache Oct 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
if !msg.Timestamp.IsZero() &&
(compMsg.Timestamp.IsZero() ||
compMsg.Timestamp.After(msg.Timestamp)) {
compMsg.Timestamp = msg.Timestamp
}
}
}
req.AddMessage(topic, partition, compMsg)
}
}
}
Expand Down
42 changes: 42 additions & 0 deletions produce_set_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -141,3 +141,45 @@ func TestProduceSetRequestBuilding(t *testing.T) {
t.Error("Wrong number of topics in request")
}
}

func TestProduceSetCompressedRequestBuilding(t *testing.T) {
parent, ps := makeProduceSet()
parent.conf.Producer.RequiredAcks = WaitForAll
parent.conf.Producer.Timeout = 10 * time.Second
parent.conf.Producer.Compression = CompressionGZIP
parent.conf.Version = V0_10_0_0

msg := &ProducerMessage{
Topic: "t1",
Partition: 0,
Key: StringEncoder(TestMessage),
Value: StringEncoder(TestMessage),
Timestamp: time.Now(),
}
for i := 0; i < 10; i++ {
safeAddMessage(t, ps, msg)
}

req := ps.buildRequest()

if req.Version != 2 {
t.Error("Wrong request version")
}

for _, msgBlock := range req.msgSets["t1"][0].Messages {
msg := msgBlock.Msg
err := msg.decodeSet()
if err != nil {
t.Error("Failed to decode set from payload")
}
for _, compMsgBlock := range msg.Set.Messages {
compMsg := compMsgBlock.Msg
if compMsg.Version != 1 {
t.Error("Wrong compressed message version")
}
}
if msg.Version != 1 {
t.Error("Wrong compressed parent message version")
}
}
}