[
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
