> On Sept. 15, 2014, 7:49 p.m., Guozhang Wang wrote: > > 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? > > Joel Koshy wrote: > Sounds good. For (2) you mean name arguments that are views clearly. > Likewise, if a method returns the result of a mapValues then name the method > with something that clearly indicates that it is returning a view. This is > mainly to get around the fact that although it is a view mapValues actually > returns a Map type (unlike other collections which return an explicit > <collection>View type).
Cool. I will make th changes and upload a new patch. - Mayuresh ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25136/#review53391 ----------------------------------------------------------- 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 > >
