----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25136/#review51793 -----------------------------------------------------------
Thanks for the patch, some general comments: 1. In general we would like to avoid using ._1 and ._2 simply due to clarity of the code; instead we can use { case (key, value) => }. 2. After thinking about it twice, I think even if the resulted collection is passed to some function as parameters, as long as we know that function will only read that value (for example ZkUtils.updatePartitionReassignmentData), but have no intention to modify it we can probably still use mapValues, which gives you the benefit of not creating one more Java collection in JVM. What do you think? 3. For places we do need to use map instead of mapValues (for example in ReplicaManager when we created the delayed request's reponse status). Add comments explaning why we do so (for the above example since "acksPending" and "errorCode" may be modified after the collection is created). Some detailed comments below. core/src/main/scala/kafka/admin/ReassignPartitionsCommand.scala <https://reviews.apache.org/r/25136/#comment90400> Is this necessary? ReassignedPartitionContext seems not used anywhere. core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala <https://reviews.apache.org/r/25136/#comment90401> Is this intentional? core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala <https://reviews.apache.org/r/25136/#comment90402> Is this intentional? core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala <https://reviews.apache.org/r/25136/#comment90404> Could we use a new line for the nested map? core/src/test/scala/unit/kafka/server/LeaderElectionTest.scala <https://reviews.apache.org/r/25136/#comment90406> For resulted maps that are used in the constructor parameters, as long as the constructor parameter will not change we can use mapValues. - Guozhang Wang On Aug. 28, 2014, 2:28 a.m., Mayuresh Gharat wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/25136/ > ----------------------------------------------------------- > > (Updated Aug. 28, 2014, 2:28 a.m.) > > > Review request for kafka. > > > Bugs: KAFKA-1610 > https://issues.apache.org/jira/browse/KAFKA-1610 > > > Repository: kafka > > > Description > ------- > > Patch for replacing mapValues by map wherever necessary so that local > modifications to collections are not lost > > > Diffs > ----- > > core/src/main/scala/kafka/admin/ReassignPartitionsCommand.scala > 691d69a49a240f38883d2025afaec26fd61281b5 > core/src/main/scala/kafka/controller/KafkaController.scala > 8ab4a1b8072c9dd187a9a6e94138b725d1f1b153 > core/src/main/scala/kafka/log/LogManager.scala > 4d2924d04bc4bd62413edb0ee2d4aaf3c0052867 > core/src/main/scala/kafka/server/DelayedFetch.scala > e0f14e25af03e6d4344386dcabc1457ee784d345 > core/src/main/scala/kafka/server/DelayedProduce.scala > 9481508fc2d6140b36829840c337e557f3d090da > core/src/main/scala/kafka/server/KafkaApis.scala > c584b559416b3ee4bcbec5966be4891e0a03eefb > core/src/main/scala/kafka/server/KafkaServer.scala > 28711182aaa70eaa623de858bc063cb2613b2a4d > core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala > af4783646803e58714770c21f8c3352370f26854 > core/src/test/scala/unit/kafka/server/LeaderElectionTest.scala > c2ba07c5fdbaf0e65ca033b2e4d88f45a8a15b2e > > Diff: https://reviews.apache.org/r/25136/diff/ > > > Testing > ------- > > > Thanks, > > Mayuresh Gharat > >