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.

Sign up for GitHub

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

Jump to bottom

Correctly handle unencodable messages in the producer #538

Merged
merged 2 commits into from
Sep 25, 2015
Merged

Correctly handle unencodable messages in the producer #538

merged 2 commits into from
Sep 25, 2015

Conversation

eapache
Copy link
Contributor

@eapache eapache commented Sep 24, 2015

Quick fix for #449, pending more thought and writing tests etc.

@cep21
Copy link

cep21 commented Sep 24, 2015

What if the flag setting happened inside return error or return success?

@wvanbergen
Copy link
Contributor

can we simply set the returned flag in the return functions themselves?

@eapache
Copy link
Contributor Author

eapache commented Sep 24, 2015

What if the flag setting happened inside return error or return success?

I suspect I'll end up getting rid of the flag (and the setting of the array to nil which it partially replaced) entirely, as that seems very fragile. Yes, setting the flag in the returning functions would have fixed this issue, but it also would have potentially hidden others since there are a lot of other places we could use messages without checking for the flag.

@eapache eapache force-pushed the handle-unencodable-messages branch from f15799e to 8b5d6d8 Compare September 24, 2015 20:49
@eapache
Copy link
Contributor Author

eapache commented Sep 24, 2015

Ya, setting the flag isn't right at all because it results in messages reaching the user with flags set, which breaks our API guarantee.

@eapache
Fix handling of unencodable messages
2840a37 
Move `Key.Encode()` and `Value.Encode()` calls slightly earlier (to
`groupAndFilter`) where we have access to the batch in order to be able to
remove them from consideration on error. Otherwise failed messages would not be
removed from the batch and could end up returned twice.

Add cache members to the ProducerMessage struct to store the results until we
actually need them.

Fixes #449.

This is perhaps not the most elegant solution. However it is correct and a
better solution would be a lot more invasive. This will do in order to ship the
fix in a 1.5.1 patch release.
@eapache
Add test for encoding failures
7e88a99
@eapache eapache force-pushed the handle-unencodable-messages branch from 34c6a26 to 7e88a99 Compare September 24, 2015 21:53
@eapache
Copy link
Contributor Author

eapache commented Sep 24, 2015

I've just pushed a better (i.e. actually correct) fix to this branch. It's still not ideal architecturally, but I think I'm happy enough with it to release it as v1.5.1 if you both agree.

@cep21
Copy link

cep21 commented Sep 24, 2015

This code works for me. There's something stinky about the existence of keyCache, valueCache, but I think that's what you're talking about w/ "not ideal" and it is an improvement for sure. I'll probably merge it into my SafeWaitGroup branch.

@eapache
Copy link
Contributor Author

eapache commented Sep 25, 2015

but I think that's what you're talking about w/ "not ideal"

Understatement is a bad habit of mine :P

@wvanbergen
Copy link
Contributor

👍 - this fix looks pretty straightforward to me.

Side note: another good reason to remove the Encoder interface for sarama v2. :)

@eapache
Copy link
Contributor Author

eapache commented Sep 25, 2015

Side note: another good reason to remove the Encoder interface for sarama v2. :)

Yup :)

eapache added a commit that referenced this pull request Sep 25, 2015
@eapache
Merge pull request #538 from Shopify/handle-unencodable-messages
53f2a54 
Correctly handle unencodable messages in the producer
@eapache eapache merged commit 53f2a54 into master Sep 25, 2015
@eapache eapache deleted the handle-unencodable-messages branch September 25, 2015 14:37
@eapache
Copy link
Contributor Author

eapache commented Sep 25, 2015

I'll release 1.5.1 with this shortly and then file another ticket to refactor/solve it properly.

@eapache
Copy link
Contributor Author

eapache commented Sep 25, 2015

1.6.1 released.

@eapache eapache mentioned this pull request Sep 25, 2015
Closed
@eapache
Copy link
Contributor Author

eapache commented Sep 25, 2015

The longer-term refactor has been rolled into #433 which has just suddenly gone from "nice to have eventually" to "reasonably important".

@eapache eapache mentioned this pull request Sep 27, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@eapache @cep21 @wvanbergen