----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34641/#review94378 -----------------------------------------------------------
core/src/main/scala/kafka/admin/ReassignPartitionsCommand.scala (line 43) <https://reviews.apache.org/r/34641/#comment148956> Formatting nitpick: Since we're now using the result of the try expression (via reassignmentStatus) we should indent the try/catch block. core/src/main/scala/kafka/admin/ReassignPartitionsCommand.scala (line 51) <https://reviews.apache.org/r/34641/#comment148957> Question: It looks to me as if we're using ReassignmentCompleted in two (different) ways: In verifyAssignment() its semantics are "the reassignment operation is fully completed" (notably, it is not in progress any longer). In the main() its semantics seem to be "reassignment operation was successfully initiated", right? This might also be the reason why main() -- unlike verifyAssignment -- will not return ReassignmentInProgress in any case. Sticking to the current way main() is supposed to work (if I understand correctly), I'd intuitively expect main() to distinguish the following states (names are just examples): (1) ReassignmentInitiated with a shell status of 0, (2) ReassignmentFailed with a shell status of 2. (I thought about the alternative to re-use ReassignmentInProgress instead of ReassignmentCompleted, but ReassignmentInProgress has a shell status code of 1 whereas we'd need a status of 0 for non-failures. So this doesn't work.) I don't have a strong opinion either way, but personally I'd prefer not to conflate these two different semantics of the current ReassignmentCompleted usage. Would that work? And please correct me if my understanding is wrong! core/src/main/scala/kafka/admin/ReassignPartitionsCommand.scala (line 87) <https://reviews.apache.org/r/34641/#comment148955> Formatting nitpick: Missing spaces between if's and conditions. core/src/main/scala/kafka/admin/ReassignPartitionsCommand.scala (line 265) <https://reviews.apache.org/r/34641/#comment148961> FYI: Exit code 2 is arguably a reserved status code (see http://tldp.org/LDP/abs/html/exitcodes.html#EXITCODESREF): "Misuse of shell builtins (according to Bash documentation)" To be extra-compliant we could change: ReassignmentFailed (the true failure) -> status 1 ReassignmentInProgress (more like a WARN) -> status 3 Again, I don't have a strong opinion here as it's IMHO pretty common to ignore the advice/link above. In general the patch is going in the right direction. I only added two minor formatting issues and a clarification request. - Michael Noll On Aug. 5, 2015, 3:19 p.m., Manikumar Reddy O wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34641/ > ----------------------------------------------------------- > > (Updated Aug. 5, 2015, 3:19 p.m.) > > > Review request for kafka. > > > Bugs: KAFKA-2214 > https://issues.apache.org/jira/browse/KAFKA-2214 > > > Repository: kafka > > > Description > ------- > > Addresing ismail juma'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 > >
