----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34641/#review92403 -----------------------------------------------------------
core/src/main/scala/kafka/admin/ReassignPartitionsCommand.scala (line 41) <https://reviews.apache.org/r/34641/#comment146570> This should be a `val` (I know, it was there already, but may as well fix it while you're there). core/src/main/scala/kafka/admin/ReassignPartitionsCommand.scala (line 58) <https://reviews.apache.org/r/34641/#comment146571> This try/catch/finally is a bit complicated and it could be simpler by taking advantage of the fact that try and catch are expressions. Something like: ``` val status = try { if (...) verifyAssignment(...) else { ... ReassignmentCompleted } } catch { case e: Throwable => .... ReassignmentFailed } if (zkClient != null) zkCient.close() System.exit(status.status) ``` This obviously doesn't compile as there is code missing, but it shows what I mean. core/src/main/scala/kafka/admin/ReassignPartitionsCommand.scala (line 62) <https://reviews.apache.org/r/34641/#comment146569> Wouldn't it be better to return `ReassignmentStatus` instead of `Int`? Generally, it's better to keep the types for as long as possible. core/src/main/scala/kafka/admin/ReassignPartitionsCommand.scala (line 80) <https://reviews.apache.org/r/34641/#comment146568> Nit: braces are not needed in case statements. - Ismael Juma On July 14, 2015, 10:13 a.m., Manikumar Reddy O wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34641/ > ----------------------------------------------------------- > > (Updated July 14, 2015, 10:13 a.m.) > > > Review request for kafka. > > > Bugs: KAFKA-2214 > https://issues.apache.org/jira/browse/KAFKA-2214 > > > Repository: kafka > > > Description > ------- > > Addressing Gwen's comments > > > Diffs > ----- > > core/src/main/scala/kafka/admin/ReassignPartitionsCommand.scala > ea345895a52977c25bff57e95e12b8662331d7fe > > Diff: https://reviews.apache.org/r/34641/diff/ > > > Testing > ------- > > > Thanks, > > Manikumar Reddy O > >
