> On Aug. 28, 2014, 5:36 p.m., Guozhang Wang wrote:
> > 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.
I agree with point 1.)
Regarding point 2.) I think that even if the function is only reading what
happens when the collection gets changed and that function reads a different
value. Of course if the collection is created locally and passed to a function
then its better to use mapValues.
Regarding point 3.) I will add those comments
- Mayuresh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25136/#review51793
-----------------------------------------------------------
On Aug. 28, 2014, 6:12 p.m., Mayuresh Gharat wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25136/
> -----------------------------------------------------------
>
> (Updated Aug. 28, 2014, 6:12 p.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
> -------
>
> Ran the unit tests and everything passed and the build succeeeded
>
>
> Thanks,
>
> Mayuresh Gharat
>
>
