-----------------------------------------------------------
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
> 
>

Reply via email to