> On Sept. 9, 2014, 1:38 a.m., Joel Koshy wrote: > > core/src/main/scala/kafka/admin/ReassignPartitionsCommand.scala, line 125 > > <https://reviews.apache.org/r/25136/diff/4/?file=675768#file675768line125> > > > > I don't think this is required, since we are subsequently doing a map > > over the view which materializes the correct final collection. So it seems > > this map creates an unnecessary intermediate collection.
Actually, we are passing the view into another function checkIfPartitionReassignmentSucceeded inside the subsequent map, and hence we cannot really controll how this function will be using this view. On Sept. 9, 2014, 1:38 a.m., Mayuresh Gharat wrote: > > I only reviewed up to DelayedFetch, because I think these changes are > > mostly unnecessary and also add overhead. Can you re-evaluate the above and > > the rest of the patch to see if there are any _correctness_ issues? > > > > I agree with the premise of this jira - i.e., we should fix places where we > > use mapValues *and* cannot have a non-strict (i.e., lazily materialized) > > view. However, most of the existing uses of mapValues are as far as I can > > see legitimate uses of views where we don't need to materialize > > intermediate collections. Joel, I see your point. I think we should discuss a little bit on the principle of using map v.s. mapView; I was originally thinking for any views that are passing to another function or used outside of the current code block, they should be converted into an intermediate collection since it is out of controll. We can discuss over this convention. - Guozhang ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25136/#review52660 ----------------------------------------------------------- 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 > >
