[jira] [Issue Comment Edited] (KAFKA-253) Refactor the async producer to have only one queue instead of one queue per broker in a Kafka cluster

["Neha Narkhede (Issue Comment Edited) (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/KAFKA-253?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13198186#comment-13198186 ] 

Neha Narkhede edited comment on KAFKA-253 at 2/1/12 9:53 PM:
-------------------------------------------------------------

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. Producer: 

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 ?

4. DefaultEventHandler
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. ProducerTest

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.

                
      was (Author: nehanarkhede):
    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. Producer: 

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 ?

4. DefaultEventHandler
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. ProducerTest

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. Al though, if you want, this can go in a separate jira.

                  
> Refactor the async producer to have only one queue instead of one queue per broker in a Kafka cluster
> -----------------------------------------------------------------------------------------------------
>
>                 Key: KAFKA-253
>                 URL: https://issues.apache.org/jira/browse/KAFKA-253
>             Project: Kafka
>          Issue Type: Improvement
>    Affects Versions: 0.8
>            Reporter: Neha Narkhede
>            Assignee: Jun Rao
>         Attachments: kafka-253.patch
>
>   Original Estimate: 168h
>  Remaining Estimate: 168h
>
> Today, the async producer is associated with a particular broker instance, just like the SyncProducer. The Producer maintains a producer pool of sync/async producers, one per broker. Since the producer pool creates one async producer per broker, we have multiple producer queues for one Producer instance. 
> With replication, a topic partition will be logical. This requires refactoring the AsyncProducer to be broker agnostic. As a side effect of this refactoring, we should also ensure that we have only one queue per Producer instance.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira