----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25136/#review53391 -----------------------------------------------------------
Have talked with Mayuresh and Joel about which mapValues should be converted to map. The summary is that 1. For now the only place we do in-place modification of a map is in ProducePartitionStatus, on isAcksPending and ErrorCode; however it is not guaranteed that we will do some in-place modification in the future that are actually on a view created by mapValues. 2. On the other hand, converting all mapValues to map whose resulted collection may be used outside the current block will increase many new object creations, especially for places where a map is called for each instance in map loop (for example the fetch response). So the suggested solution is that we should keep in mind of this risk for in-place modifications in Scala (fortunately no-one does that often) by: 1. Adding comments on where "map" is already used and MUST be used since its resultd collection items will be modified (for now the only place I know of is ProducePartitionStatus, Mayuresh could you double check if there are other cases?). 2. Add comments on function parameters that are passed in as a view indicating the given map should not be modified in-place. 3. Once we are making in-place modification into a Map in Scala, check if this collection can be created as a view. Mayuresh, could you submit another patch following this suggestion and also incorporating Neha's comments? - Guozhang Wang On Sept. 3, 2014, 6:27 p.m., Mayuresh Gharat wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/25136/ > ----------------------------------------------------------- > > (Updated Sept. 3, 2014, 6:27 p.m.) > > > Review request for kafka. > > > Bugs: KAFKA-1610 > https://issues.apache.org/jira/browse/KAFKA-1610 > > > Repository: kafka > > > Description > ------- > > Added comments explaining the changes and reverted back some changes as per > comments on the reviewboard > > > Removed the unnecessary import > > > Made changes to comments as per the suggestions on the reviewboard > > > Diffs > ----- > > core/src/main/scala/kafka/admin/ReassignPartitionsCommand.scala > 691d69a49a240f38883d2025afaec26fd61281b5 > core/src/main/scala/kafka/controller/KafkaController.scala > 8ab4a1b8072c9dd187a9a6e94138b725d1f1b153 > 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 > >
