Closed evan-stripe closed 6 years ago
Can one of the admins verify this patch?
@horkhe Would love to get your take on these when you have a chance
@evan-stripe sorry for the delay, I was overwhelmed with all the move to a new apartment, plus I am taking a trip to Kiev Ukraine. I will take a look in the next couple of days if nothing extraordinary happens 🤞
Cool, no problem! Just wanted to make sure it didn't get lost in the shuffle.
I just love how thorough you are, perfect contributor! Do you want to work for Mailgun? seriously?
Could you please also update CHANGELOG, just put there what is in the description of the PR.
And sorry for delay, it was just bad timing for me. I will be faster next time, I promise 🤞 😄
Haha, thanks so much 🙂- I'm just trying to send the PRs that I'd want to receive.
I pushed a changelog update. I went for slightly less verbose than what I put in the PR description, but let me know if you want it more fleshed out.
This adds new configuration flags for a handful of underlying Sarama options that we wanted the ability to tune. I put some notes about each of them in the commit messages, but here's why we wanted to tune these:
Producer.Partitioner
: Both the random and roundrobin partitioners can be useful in situations where consistent partitioning isn't required. For instance, even though the default hash partitioner randomly distribute messages with an unspecified key, itsRequiresConsistency
method returns true, indicating that it's possible for randomly-partitioned messages to get published to partitions that are known to be down.The implementation here is a little funky. Ideally I'd do something like
Version
orRequiredAcks
, where the config field is a thin wrapper around the Sarama type. However, Sarama'sProducer.Partitioner
is a (alias for a) function type, function types are not comparable (except tonil
), and Kafka-Pixy's tests expect to be able to compare config structs to each other.Producer.Timeout
: When publishing synchronously, it's useful to be able to reduce the broker timeout so that failures are returned to clients more quickly (allowing them to retry or otherwise attempt to mitigate the failure).Net.{Dial,Read,Write}Timeout
: We've found these useful for hedging against network-level issues (whereas, e.g., the producer timeout is only useful if requests actually make it to the producer)Putting these under a top-level "net" struct seemed consistent with Sarama's config, but maybe they should go somewhere else?