[jira] [Commented] (KAFKA-369) remove ZK dependency on producer

["Jun Rao (JIRA)" <ji...@apache.org>]

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

Jun Rao commented on KAFKA-369:
-------------------------------

Thanks for patch v1. Some comments:

1. ProducerConfig:
1.1 change broker.list's format to host:port; Also, change the comment and explain this is just for bootstrapping and it just needs to be a subset of all brokers in the cluster.
1.2 We should make broker.list a required property by not providing a default value. If we do the following, an exception will be thrown if the property is not specified. There is no need to test for null any more.
     Utils.getString(props, "broker.list")
1.3 There is no need for ProducerConfig to extend ZKConfig.

2. Producer: There is no need to check the existence of brokerList since it will be handled in ProducerConfig.

3. BrokerPartitionInfo:
3.1 It's better to rename it to something like MetaDataCache
3.2 This class shouldn't depend on ProducerPool, which is supposed to be used for caching SyncProducer objects for sending produce request. Instead, this class only needs to know broker.list. The reason is that broker.list is just for bootstrapping and we can pass in a VIP instead of a real host. Each time we need to send a getMetaData request, we can just pick a host from broker.list, instantiate a SyncProducer, send the request, get the response, and close the SyncProducer. Since getMetaData is going to be used rarely, there is no need to cache the connection. It also avoids the timeout problem with VIP. For new brokers that we get in updateInfo(), we can instantiate a new SyncProducer and add it to ProducerPool.

4. ProducerPool
4.1 If we do the above in BrokerPartitionInfo, we can get rid of the following methods.
  def addProducers(config: ProducerConfig) 
  def getAnyProducer: SyncProducer 

5. KafkaConfig: The default value of hostName should be InetAddress.getLocalHost.getHostAddress. We can then get rid of the hostName null check in KafkaZookeeper.registerBrokerInZk().

6. KafkaKig4jAppender: get rid of the commented out lines related to ZkConnect; get rid of host and port

7. KafkaKig4jAppenderTest: testZkConnectLog4jAppends should be renamed since there is no ZK connect any more.

8. ProducerTest: 
8.1 all method of testZK* should be renamed properly.
8.2  Quite a few places have the following checks. Can't we just combine them using a check for leaderIsLive?
    assertTrue("Topic new-topic not created after timeout", TestUtils.waitUntilTrue(() =>
      AdminUtils.getTopicMetaDataFromZK(List("new-topic"),
        zkClient).head.errorCode != ErrorMapping.UnknownTopicCode, zookeeper.tickTime))
    TestUtils.waitUntilLeaderIsElectedOrChanged(zkClient, "new-topic", 0, 500)

9. Utils.getAllBrokersFromBrokerList(): There is no need to get the broker id from broker.list. We can just assign some sequential ids.

                
> remove ZK dependency on producer
> --------------------------------
>
>                 Key: KAFKA-369
>                 URL: https://issues.apache.org/jira/browse/KAFKA-369
>             Project: Kafka
>          Issue Type: Sub-task
>          Components: core
>    Affects Versions: 0.8
>            Reporter: Jun Rao
>            Assignee: Yang Ye
>         Attachments: kafka_369_v1.diff
>
>   Original Estimate: 252h
>  Remaining Estimate: 252h
>
> Currently, the only place that ZK is actually used is in BrokerPartitionInfo. We use ZK to get a list of brokers for making TopicMetadataRequest requests. Instead, we can provide a list of brokers in the producer config directly. That way, the producer client is no longer dependant on ZK.

--
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