Mailing-List: contact dev-help@kafka.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@kafka.apache.org
Content-Type: multipart/alternative;
 boundary="===============2809894489940629856=="
MIME-Version: 1.0
Subject: Re: Review Request 17248: Patch for KAFKA-1215
From: "Guozhang Wang" <guwang@linkedin.com>
To: "Joris Van Remoortere" <joris.van.remoortere@gmail.com>,
 "kafka" <dev@kafka.apache.org>, "Guozhang Wang" <guwang@linkedin.com>
Date: Mon, 03 Mar 2014 18:38:03 -0000
Message-ID: <20140303183803.10089.60529@reviews.apache.org>
Auto-Submitted: auto-generated
Sender: "Guozhang Wang" <noreply@reviews.apache.org>
References: <20140201002011.21445.92845@reviews.apache.org>
In-Reply-To: <20140201002011.21445.92845@reviews.apache.org>
Reply-To: "Guozhang Wang" <guwang@linkedin.com>

--===============2809894489940629856==
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17248/#review35997
-----------------------------------------------------------


Some high-level comments:

1. Instead of exposing the maxRackReplica parameter throughout the calling hierarchy and also in the API function, could we just wrap it in these places:

a. assignReplicasToBrokers, the only place we need to read this parameter. This function is called by

createTopic

addPartitions

generateAssignment

All of these three have zkClient, so we can read the cluster and maxRackReplica information just there.

b. createOrUpdateTopicPartitionAssignmentPathInZK, one of the two places we need to write the value from the command line tool. Instead of specify this parameter we can treat it as part of the general topic configs (i.e. just add this value in parseTopicConfigsToBeAdded).

c. updateAssignedReplicasForPartition, the other place we need to write the value from the controller. Currently the logic never changes this value, what it does is just read the original value and write again. So we can just avoid doing so by just moving this value to the general configs, like I said in b).

2. Broker.scala is used by the producer, who do not need to know the rack information.


- Guozhang Wang


On Feb. 1, 2014, 12:20 a.m., Joris Van Remoortere wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17248/
> -----------------------------------------------------------
> 
> (Updated Feb. 1, 2014, 12:20 a.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1215
>     https://issues.apache.org/jira/browse/KAFKA-1215
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1226
> 
> 
> Diffs
> -----
> 
>   core/src/main/scala/kafka/admin/AdminUtils.scala a167756 
>   core/src/main/scala/kafka/admin/ReassignPartitionsCommand.scala 2637586 
>   core/src/main/scala/kafka/admin/TopicCommand.scala 842c110 
>   core/src/main/scala/kafka/client/ClientUtils.scala 1d2f81b 
>   core/src/main/scala/kafka/cluster/Broker.scala 9407ed2 
>   core/src/main/scala/kafka/controller/KafkaController.scala a0267ae 
>   core/src/main/scala/kafka/controller/PartitionStateMachine.scala ac4262a 
>   core/src/main/scala/kafka/server/KafkaApis.scala 29abc46 
>   core/src/main/scala/kafka/server/KafkaConfig.scala 3c3aafc 
>   core/src/main/scala/kafka/server/KafkaHealthcheck.scala 9dca55c 
>   core/src/main/scala/kafka/server/KafkaServer.scala 5e34f95 
>   core/src/main/scala/kafka/utils/ZkUtils.scala b42e52b 
>   core/src/test/scala/unit/kafka/admin/AddPartitionsTest.scala 115e203 
>   core/src/test/scala/unit/kafka/admin/AdminTest.scala 59de1b4 
>   core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala 8df0982 
>   core/src/test/scala/unit/kafka/consumer/ConsumerIteratorTest.scala 9347ea6 
>   core/src/test/scala/unit/kafka/integration/FetcherTest.scala 47130d3 
>   core/src/test/scala/unit/kafka/integration/TopicMetadataTest.scala 9998a11 
>   core/src/test/scala/unit/kafka/producer/AsyncProducerTest.scala 18e3555 
>   core/src/test/scala/unit/kafka/server/LeaderElectionTest.scala 38e3ae7 
>   core/src/test/scala/unit/kafka/utils/TestUtils.scala d88b6c3 
>   examples/README d33f6c5 
> 
> Diff: https://reviews.apache.org/r/17248/diff/
> 
> 
> Testing
> -------
> 
> 
> File Attachments
> ----------------
> 
> rack_aware_replica_assignment_v1.patch
>   https://reviews.apache.org/media/uploaded/files/2014/01/23/394cef99-f800-4d94-bc59-fdb6c68b53f5__rack_aware_replica_assignment_v1.patch
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>


--===============2809894489940629856==--