You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Guozhang Wang <gu...@linkedin.com> on 2013/12/03 01:58:01 UTC
Review Request 15953: Patch for KAFKA-1134
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15953/
-----------------------------------------------------------
Review request for kafka.
Bugs: KAFKA-1134
https://issues.apache.org/jira/browse/KAFKA-1134
Repository: kafka
Description
-------
KAFKA-1134.v1
Diffs
-----
core/src/main/scala/kafka/controller/KafkaController.scala 4c319aba97655e7c4ec97fac2e34de4e28c9f5d3
Diff: https://reviews.apache.org/r/15953/diff/
Testing
-------
Thanks,
Guozhang Wang
Re: Review Request 15953: Patch for KAFKA-1134
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15953/#review30057
-----------------------------------------------------------
core/src/main/scala/kafka/controller/PartitionStateMachine.scala
<https://reviews.apache.org/r/15953/#comment57578>
This it not required, but just to eliminate log flushing due to fire-all-watcher-at-zk-expiration.
Neha, I think you reviewed the wrong diff, it is diff between r1 and r2.
- Guozhang Wang
On Dec. 5, 2013, 7:13 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15953/
> -----------------------------------------------------------
>
> (Updated Dec. 5, 2013, 7:13 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1134
> https://issues.apache.org/jira/browse/KAFKA-1134
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1134.v1
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/controller/ControllerChannelManager.scala beca460dfe0f4df5ccd7f6358e44cbe742d256e5
> core/src/main/scala/kafka/controller/KafkaController.scala 3beaf75f8285c8b6146aced2fefda4234cf1d307
> core/src/main/scala/kafka/controller/PartitionStateMachine.scala 829163afab99706f7a34408eda2504c8262e572e
>
> Diff: https://reviews.apache.org/r/15953/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 15953: Patch for KAFKA-1134
Posted by Neha Narkhede <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15953/#review30152
-----------------------------------------------------------
Ship it!
Ship It!
- Neha Narkhede
On Dec. 5, 2013, 7:13 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15953/
> -----------------------------------------------------------
>
> (Updated Dec. 5, 2013, 7:13 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1134
> https://issues.apache.org/jira/browse/KAFKA-1134
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1134.v1
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/controller/ControllerChannelManager.scala beca460dfe0f4df5ccd7f6358e44cbe742d256e5
> core/src/main/scala/kafka/controller/KafkaController.scala 3beaf75f8285c8b6146aced2fefda4234cf1d307
> core/src/main/scala/kafka/controller/PartitionStateMachine.scala 829163afab99706f7a34408eda2504c8262e572e
>
> Diff: https://reviews.apache.org/r/15953/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 15953: Patch for KAFKA-1134
Posted by Neha Narkhede <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15953/#review30047
-----------------------------------------------------------
core/src/main/scala/kafka/controller/KafkaController.scala
<https://reviews.apache.org/r/15953/#comment57553>
Is this change supposed to be part of this patch?
core/src/main/scala/kafka/controller/KafkaController.scala
<https://reviews.apache.org/r/15953/#comment57555>
is this intended? This is required no?
core/src/main/scala/kafka/controller/KafkaController.scala
<https://reviews.apache.org/r/15953/#comment57554>
why is this required?
- Neha Narkhede
On Dec. 5, 2013, 7:13 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15953/
> -----------------------------------------------------------
>
> (Updated Dec. 5, 2013, 7:13 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1134
> https://issues.apache.org/jira/browse/KAFKA-1134
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1134.v1
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/controller/ControllerChannelManager.scala beca460dfe0f4df5ccd7f6358e44cbe742d256e5
> core/src/main/scala/kafka/controller/KafkaController.scala 3beaf75f8285c8b6146aced2fefda4234cf1d307
> core/src/main/scala/kafka/controller/PartitionStateMachine.scala 829163afab99706f7a34408eda2504c8262e572e
>
> Diff: https://reviews.apache.org/r/15953/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 15953: Patch for KAFKA-1134
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15953/
-----------------------------------------------------------
(Updated Dec. 5, 2013, 7:13 p.m.)
Review request for kafka.
Bugs: KAFKA-1134
https://issues.apache.org/jira/browse/KAFKA-1134
Repository: kafka
Description
-------
KAFKA-1134.v1
Diffs (updated)
-----
core/src/main/scala/kafka/controller/ControllerChannelManager.scala beca460dfe0f4df5ccd7f6358e44cbe742d256e5
core/src/main/scala/kafka/controller/KafkaController.scala 3beaf75f8285c8b6146aced2fefda4234cf1d307
core/src/main/scala/kafka/controller/PartitionStateMachine.scala 829163afab99706f7a34408eda2504c8262e572e
Diff: https://reviews.apache.org/r/15953/diff/
Testing
-------
Thanks,
Guozhang Wang
Re: Review Request 15953: Patch for KAFKA-1134
Posted by Guozhang Wang <gu...@linkedin.com>.
> On Dec. 3, 2013, 1:30 a.m., Neha Narkhede wrote:
> > core/src/main/scala/kafka/controller/KafkaController.scala, line 235
> > <https://reviews.apache.org/r/15953/diff/1/?file=392920#file392920line235>
> >
> > it seems that onControllerFailover is already protected by the controllerLock. The elect() API of ZookeeperLeaderElector is invoked in 3 places and each of those acquires the controllerLock
You are right. The real issue is not that onControllerFailover is not synchronized, but is that the sendRequest is asynchronized. Hence in onControllerFailover, it just put the request on the queue, and while the send thread wakes up to send the message, it may have already been closed by the handleNewSession procedure.
I think the correct fix should be, in ControllerChannelManager.removeExistingBroker, we should also clear the request queue.
- Guozhang
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15953/#review29636
-----------------------------------------------------------
On Dec. 3, 2013, 12:58 a.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15953/
> -----------------------------------------------------------
>
> (Updated Dec. 3, 2013, 12:58 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1134
> https://issues.apache.org/jira/browse/KAFKA-1134
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1134.v1
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/controller/KafkaController.scala 4c319aba97655e7c4ec97fac2e34de4e28c9f5d3
>
> Diff: https://reviews.apache.org/r/15953/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 15953: Patch for KAFKA-1134
Posted by Neha Narkhede <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15953/#review29636
-----------------------------------------------------------
core/src/main/scala/kafka/controller/KafkaController.scala
<https://reviews.apache.org/r/15953/#comment57045>
it seems that onControllerFailover is already protected by the controllerLock. The elect() API of ZookeeperLeaderElector is invoked in 3 places and each of those acquires the controllerLock
- Neha Narkhede
On Dec. 3, 2013, 12:58 a.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15953/
> -----------------------------------------------------------
>
> (Updated Dec. 3, 2013, 12:58 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1134
> https://issues.apache.org/jira/browse/KAFKA-1134
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1134.v1
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/controller/KafkaController.scala 4c319aba97655e7c4ec97fac2e34de4e28c9f5d3
>
> Diff: https://reviews.apache.org/r/15953/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>