Home | About | Sematext search-lucene.com search-hadoop.com
 Search Hadoop and all its subprojects:

Switch to Plain View
Kafka, mail # dev - Review Request 17248: Patch for KAFKA-1226


+
Joris Van Remoortere 2014-01-23, 18:15
+
Joris Van Remoortere 2014-01-23, 18:18
+
Joris Van Remoortere 2014-02-01, 00:20
Copy link to this message
-
Re: Review Request 17248: Patch for KAFKA-1215
Jun Rao 2014-02-24, 17:55

This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17248/#review35293
Sorry for the late review. The high level comment is that we need to make this change backward compatible, which means that we can do a rolling upgrade of an existing 0.8 cluster. To do that, we need to (1) consider wire protocol changes (see comments on Broker); (2) make sure that the old code can read the new ZK format. Once the following comments are addressed, could you exercise a rolling upgrade and see if there is any issue?
core/src/main/scala/kafka/admin/AdminUtils.scala
<https://reviews.apache.org/r/17248/#comment65764>

    not => Not

core/src/main/scala/kafka/admin/ReassignPartitionsCommand.scala
<https://reviews.apache.org/r/17248/#comment65766>

    Shouldn't we pass in maxReplicaPerRack?

core/src/main/scala/kafka/admin/TopicCommand.scala
<https://reviews.apache.org/r/17248/#comment65768>

    After you rebase, we need to make sure this option is only available during topic creation.

core/src/main/scala/kafka/client/ClientUtils.scala
<https://reviews.apache.org/r/17248/#comment65760>

    broker list is for the clients and there is no need for them to know the rack a broker is in. host/port is enough.

core/src/main/scala/kafka/cluster/Broker.scala
<https://reviews.apache.org/r/17248/#comment65770>

    Could we preserve the current constructor w/o rack and default rack to 0?

core/src/main/scala/kafka/cluster/Broker.scala
<https://reviews.apache.org/r/17248/#comment65763>

    This actually changes the wire protocol btw the controller and the broker, which makes rolling upgrade difficult. If we really want to change the protocol, we need to do this in 2 steps. First upgrade the code w/o using the new format. Second, use the new format. So, we will need a new config option. For this particular case, I am not sure that we need to change the protocol though. We can only populate the rack info when instantiating a broker from ZK. This will make sure that we have the rack info for replica assignment. Then, we don't change the wire protocol btw the controller and the broker. All brokers instantiated through ByteBuffer will have the default rack info.

core/src/main/scala/kafka/utils/ZkUtils.scala
<https://reviews.apache.org/r/17248/#comment65769>

    Could this be integrated into getCluster()?
- Jun Rao
On Feb. 1, 2014, 12:20 a.m., Joris Van Remoortere wrote: