That is a pretty useful refactoring patch ! I have a few questions -
1. ProducerMethodsTest is empty
2. ProducerData can be a case class instead. You will get toString for free, in addition to equals() which is useful for unit testing. Al though, I'm not sure if there are any obvious downsides of using case classes.
3.1 How about throwing an InvalidConfigException when both zk.connect and broker.list are specified ? It is probably a misconfiguration that should be corrected. We have gotten bitten by this several times at LinkedIn. The user could easily miss the warning we issue saying that we will just use zk.connect.
3.2 AsyncProducerStats: Do we want to record an event as sent even before it has successfully entered the producer queue ? I'm thinking that one stat counts the number of events successfully entering the producer queue and another stat counts the number of events that got dropped due to a full queue. But maybe, the user is interested in per topic level event counts and those should be available for the sync producer as well. So maybe we can get rid of numEvents from AsyncProducerStats for now ?
In the producer send retry logic inside handle() API, what happens if a broker fails after partitionAndCollate() and right before send() ? Simply retrying send to the same broker is not useful, we should probably repartition and retry the send, which will at least attempt to select another live broker.
5.1 testBrokerListAndAsync: It is better to write as many true unit tests as possible. I don't see an advantage in actually instantiating a zookeeper consumer when EasyMock can be used to verify the producer functionality. For example, it seems that here, you want to test the async producer with the broker.list config. Simply verifying the send() API of the SyncProducer should suffice. Unit tests can assume that data doesn't get corrupted over the socket.
5.2 testJavaProducer: Same as above. Use mocks wherever possible.
testPartitionedSendToNewBrokerInExistingTopic, testPartitionedSendToNewBrokerInExistingTopic: Any reason for deleting these tests ? They test valid code paths for the ZK producer and we added these to make sure new topics and new brokers in existing topics are handled correctly with the ZK producer.
5.3 Rest of the tests: It will be a good idea to even cleanup this test suite to avoid bringing up a server and 2 SimpleConsumers. We don't know of a good way to mock out the zk part of it, so we can choose to just bring up a local zk instance, and to indicate existence of a new broker, we can just call registerBrokerInZK instead of actually bringing up a broker.
Though, if you want, this can go in a separate jira.