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