search

MeltwaterArchive / dropwizard-extra

A set of miscellaneous and common Dropwizard utilities
109 stars 45 forks source link

Get KafkaProducer working #22

Closed mjwillson closed 10 years ago

mjwillson commented 10 years ago

This is a rewrite of parts of datasift#19.

I tried to stick to the existing conventions used by KafkaConsumer when it comes to config property names, getters/setters and annotations. (Although we did note that, with newer versions of dropwizard and jackson at least, Jackson serialization can be done with a lot less boilerplate than is used here.)

I also included a commit which fixes some typos (in setter method names) and inconsistencies in KafkaConsumer -- can separate this out if you want but wouldn't hurt to merge too.

Cheers!

nicktelford commented 10 years ago

Thanks for working on this. I've committed a similar set of changes that I've been working on myself in c20e11e8d2c86f461231fe776df41a914ce67eb1.

However, I hadn't spotted the mistakes in KafkaConsumerFactory. Since these commits have been submitted together, what I've done is apply those changes manually, but mark the commit author as you: 4fe714dcea6b52bd193cae81086a2ea3ac93fb67

mjwillson commented 10 years ago

Hiya -- thanks, this change looks great, and does a better job with richer typing of some of the config settings.

One thing from my patch which yours doesn't do: could KafkaProducer extend (and InstrumentedProducer implement) io.dropwizard.lifecycle.Managed so that close is called on stop? This would allow the producer to be managed via the dropwizard environment lifecycle. (The patch does advertise it as being managed, so this would be nice to have :)

mjwillson commented 10 years ago

Also, was there a reason to capitalize Producer in the package name

package com.datasift.dropwizard.kafka.Producer;

?

mjwillson commented 10 years ago

I put together a quick pull request #24 against this new code anyway to address these two things.

Another thing is it'd be nice to have some tests for this new producer code, which I did have in my original patch if only of the config making it across intact. Will add these to the PR if I get time but can't guarantee it.