You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Jun Rao <ju...@gmail.com> on 2015/02/21 17:16:45 UTC
Review Request 31260: Patch for kafka-1971
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31260/
-----------------------------------------------------------
Review request for kafka.
Bugs: kafka-1971
https://issues.apache.org/jira/browse/kafka-1971
Repository: kafka
Description
-------
KAFKA-1971; starting a broker with a conflicting id will delete the previous broker registration
Diffs
-----
core/src/main/scala/kafka/server/KafkaHealthcheck.scala 4acdd70fe9c1ee78d6510741006c2ece65450671
Diff: https://reviews.apache.org/r/31260/diff/
Testing
-------
Thanks,
Jun Rao
Re: Review Request 31260: Patch for kafka-1971
Posted by Neha Narkhede <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31260/#review73592
-----------------------------------------------------------
Ship it!
+1 if the comment below is fixed.
core/src/test/scala/unit/kafka/server/ConflictBrokerRegistrationTest.scala
<https://reviews.apache.org/r/31260/#comment119989>
I'd move this to existing tests for server startup/shutdown. For example, ServerStartupTest. Let's not create an entire test suite just for this purpose.
- Neha Narkhede
On Feb. 23, 2015, 5:11 a.m., Jun Rao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31260/
> -----------------------------------------------------------
>
> (Updated Feb. 23, 2015, 5:11 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: kafka-1971
> https://issues.apache.org/jira/browse/kafka-1971
>
>
> Repository: kafka
>
>
> Description
> -------
>
> address review comments
>
>
> remove unused util method
>
>
> add unit test
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/KafkaHealthcheck.scala 4acdd70fe9c1ee78d6510741006c2ece65450671
> core/src/main/scala/kafka/server/KafkaServer.scala 7e5ddcb9be8fcef3df6ebc82a13ef44ef95f73ae
> core/src/main/scala/kafka/utils/ZkUtils.scala c78a1b6ff4213e13cabccd21a7b40cfeddbfb237
> core/src/test/scala/unit/kafka/server/ConflictBrokerRegistrationTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/31260/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jun Rao
>
>
Re: Review Request 31260: Patch for kafka-1971
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31260/
-----------------------------------------------------------
(Updated Feb. 23, 2015, 5:11 a.m.)
Review request for kafka.
Bugs: kafka-1971
https://issues.apache.org/jira/browse/kafka-1971
Repository: kafka
Description (updated)
-------
address review comments
remove unused util method
add unit test
Diffs (updated)
-----
core/src/main/scala/kafka/server/KafkaHealthcheck.scala 4acdd70fe9c1ee78d6510741006c2ece65450671
core/src/main/scala/kafka/server/KafkaServer.scala 7e5ddcb9be8fcef3df6ebc82a13ef44ef95f73ae
core/src/main/scala/kafka/utils/ZkUtils.scala c78a1b6ff4213e13cabccd21a7b40cfeddbfb237
core/src/test/scala/unit/kafka/server/ConflictBrokerRegistrationTest.scala PRE-CREATION
Diff: https://reviews.apache.org/r/31260/diff/
Testing
-------
Thanks,
Jun Rao
Re: Review Request 31260: Patch for kafka-1971
Posted by Neha Narkhede <ne...@gmail.com>.
> On Feb. 21, 2015, 6:14 p.m., Neha Narkhede wrote:
> > core/src/main/scala/kafka/server/KafkaHealthcheck.scala, line 53
> > <https://reviews.apache.org/r/31260/diff/1/?file=871715#file871715line53>
> >
> > We should try to understand why we need to explicitly de-register here. It isn't immediately clear to me.
If you understand it, please can you add a comment here?
- Neha
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31260/#review73411
-----------------------------------------------------------
On Feb. 21, 2015, 4:16 p.m., Jun Rao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31260/
> -----------------------------------------------------------
>
> (Updated Feb. 21, 2015, 4:16 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: kafka-1971
> https://issues.apache.org/jira/browse/kafka-1971
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1971; starting a broker with a conflicting id will delete the previous broker registration
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/KafkaHealthcheck.scala 4acdd70fe9c1ee78d6510741006c2ece65450671
>
> Diff: https://reviews.apache.org/r/31260/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jun Rao
>
>
Re: Review Request 31260: Patch for kafka-1971
Posted by Neha Narkhede <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31260/#review73411
-----------------------------------------------------------
Can we add a unit test in ServerStartupTest?
core/src/main/scala/kafka/server/KafkaHealthcheck.scala
<https://reviews.apache.org/r/31260/#comment119748>
Even if there is a good reason to explicitly register, isn't a better strategy to check if the broker is deleting it's own registration based on the node's data?
core/src/main/scala/kafka/server/KafkaHealthcheck.scala
<https://reviews.apache.org/r/31260/#comment119747>
We should try to understand why we need to explicitly de-register here. It isn't immediately clear to me.
- Neha Narkhede
On Feb. 21, 2015, 4:16 p.m., Jun Rao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31260/
> -----------------------------------------------------------
>
> (Updated Feb. 21, 2015, 4:16 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: kafka-1971
> https://issues.apache.org/jira/browse/kafka-1971
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1971; starting a broker with a conflicting id will delete the previous broker registration
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/KafkaHealthcheck.scala 4acdd70fe9c1ee78d6510741006c2ece65450671
>
> Diff: https://reviews.apache.org/r/31260/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jun Rao
>
>