You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Manikumar Reddy O <ma...@gmail.com> on 2015/07/10 18:28:48 UTC
Re: Review Request 34641: Patch for KAFKA-2214
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34641/
-----------------------------------------------------------
(Updated July 10, 2015, 4:28 p.m.)
Review request for kafka.
Bugs: KAFKA-2214
https://issues.apache.org/jira/browse/KAFKA-2214
Repository: kafka
Description (updated)
-------
rebase
Diffs (updated)
-----
core/src/main/scala/kafka/admin/ReassignPartitionsCommand.scala ea345895a52977c25bff57e95e12b8662331d7fe
Diff: https://reviews.apache.org/r/34641/diff/
Testing
-------
Thanks,
Manikumar Reddy O
Re: Review Request 34641: Patch for KAFKA-2214
Posted by Michael Noll <mi...@confluent.io>.
-----------------------------------------------------------
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
>
>
Re: Review Request 34641: Patch for KAFKA-2214
Posted by Manikumar Reddy O <ma...@gmail.com>.
-----------------------------------------------------------
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 (updated)
-------
Addresing ismail juma's comments
Diffs (updated)
-----
core/src/main/scala/kafka/admin/ReassignPartitionsCommand.scala ea345895a52977c25bff57e95e12b8662331d7fe
Diff: https://reviews.apache.org/r/34641/diff/
Testing
-------
Thanks,
Manikumar Reddy O
Re: Review Request 34641: Patch for KAFKA-2214
Posted by Manikumar Reddy O <ma...@gmail.com>.
> On July 17, 2015, 5:03 a.m., Gwen Shapira wrote:
> > Just realized that verifyAssigment runs on a Map of <Partition, Status> and originally printed status for each partition (some can succeed, fail or be in-progress).
> >
> > We need to return a single error code. In the existing code, we return 0 if all partitions were successfull, 1 if one or more failed, 2 if one of more is in progress. If some are in-progress and some are failed, it seems like the return value is either 1 or 2, at random (since it depends on order of foreach on a Map).
> >
> > I'm not sure thats expected :)
> >
> > Perhaps we can come up with something better defined?
Actually return value is not random. If some are in-progress and some are failed, we are returning failed error code. Re-asginment failed status is given more preference than in-progress error status.
- Manikumar Reddy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34641/#review92024
-----------------------------------------------------------
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
>
>
Re: Review Request 34641: Patch for KAFKA-2214
Posted by Gwen Shapira <gs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34641/#review92024
-----------------------------------------------------------
Just realized that verifyAssigment runs on a Map of <Partition, Status> and originally printed status for each partition (some can succeed, fail or be in-progress).
We need to return a single error code. In the existing code, we return 0 if all partitions were successfull, 1 if one or more failed, 2 if one of more is in progress. If some are in-progress and some are failed, it seems like the return value is either 1 or 2, at random (since it depends on order of foreach on a Map).
I'm not sure thats expected :)
Perhaps we can come up with something better defined?
- Gwen Shapira
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
>
>
Re: Review Request 34641: Patch for KAFKA-2214
Posted by Manikumar Reddy O <ma...@gmail.com>.
> On July 21, 2015, 1:50 p.m., Ismael Juma wrote:
> >
Thanks for the suggestions.
- Manikumar Reddy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34641/#review92403
-----------------------------------------------------------
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
>
>
Re: Review Request 34641: Patch for KAFKA-2214
Posted by Ismael Juma <mi...@juma.me.uk>.
-----------------------------------------------------------
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
>
>
Re: Review Request 34641: Patch for KAFKA-2214
Posted by Manikumar Reddy O <ma...@gmail.com>.
-----------------------------------------------------------
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 (updated)
-----
core/src/main/scala/kafka/admin/ReassignPartitionsCommand.scala ea345895a52977c25bff57e95e12b8662331d7fe
Diff: https://reviews.apache.org/r/34641/diff/
Testing
-------
Thanks,
Manikumar Reddy O
Re: Review Request 34641: Patch for KAFKA-2214
Posted by Manikumar Reddy O <ma...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34641/
-----------------------------------------------------------
(Updated July 14, 2015, 10:03 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 (updated)
-----
core/src/main/scala/kafka/admin/ReassignPartitionsCommand.scala ea345895a52977c25bff57e95e12b8662331d7fe
Diff: https://reviews.apache.org/r/34641/diff/
Testing
-------
Thanks,
Manikumar Reddy O
Re: Review Request 34641: Patch for KAFKA-2214
Posted by Manikumar Reddy O <ma...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34641/
-----------------------------------------------------------
(Updated July 13, 2015, 3:43 p.m.)
Review request for kafka.
Bugs: KAFKA-2214
https://issues.apache.org/jira/browse/KAFKA-2214
Repository: kafka
Description (updated)
-------
Addressing Gwen's comments
Diffs (updated)
-----
core/src/main/scala/kafka/admin/ReassignPartitionsCommand.scala ea345895a52977c25bff57e95e12b8662331d7fe
Diff: https://reviews.apache.org/r/34641/diff/
Testing
-------
Thanks,
Manikumar Reddy O
Re: Review Request 34641: Patch for KAFKA-2214
Posted by Gwen Shapira <gs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34641/#review91314
-----------------------------------------------------------
Thanks for the patch. Lets streamline error handling a bit. Also, can you post sample output to the JIRA, so we can make sure the errors are use-friendly?
core/src/main/scala/kafka/admin/ReassignPartitionsCommand.scala (lines 60 - 61)
<https://reviews.apache.org/r/34641/#comment144627>
Can we just exit in the exception clause?
core/src/main/scala/kafka/admin/ReassignPartitionsCommand.scala (line 79)
<https://reviews.apache.org/r/34641/#comment144625>
why not re-throw?
APIs that can both return error codes and throw exceptions are pretty confusing (you need to handle errors in two different ways).
- Gwen Shapira
On July 10, 2015, 4:28 p.m., Manikumar Reddy O wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34641/
> -----------------------------------------------------------
>
> (Updated July 10, 2015, 4:28 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2214
> https://issues.apache.org/jira/browse/KAFKA-2214
>
>
> Repository: kafka
>
>
> Description
> -------
>
> rebase
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/admin/ReassignPartitionsCommand.scala ea345895a52977c25bff57e95e12b8662331d7fe
>
> Diff: https://reviews.apache.org/r/34641/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Manikumar Reddy O
>
>