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/10/05 00:23:01 UTC
Review Request 14497: Patch for KAFKA-1075
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14497/
-----------------------------------------------------------
Review request for kafka.
Bugs: KAFKA-1075
https://issues.apache.org/jira/browse/KAFKA-1075
Repository: kafka
Description
-------
KAFKA-1075.v1
Diffs
-----
config/tools-log4j.properties 79240490149835656e2a013a9702c5aa41c104f1
core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala 08b4b7218f62f876eb95628bd6db78c0956c7f04
Diff: https://reviews.apache.org/r/14497/diff/
Testing
-------
Thanks,
Guozhang Wang
Re: Review Request 14497: Incorporate comments from v1
Posted by Neha Narkhede <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14497/#review26740
-----------------------------------------------------------
config/tools-log4j.properties
<https://reviews.apache.org/r/14497/#comment52052>
Let's get rid of any change to this file
- Neha Narkhede
On Oct. 7, 2013, 5:25 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14497/
> -----------------------------------------------------------
>
> (Updated Oct. 7, 2013, 5:25 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1075
> https://issues.apache.org/jira/browse/KAFKA-1075
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1075.v2
>
>
> KAFKA-1075.v1
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala 08b4b7218f62f876eb95628bd6db78c0956c7f04
>
> Diff: https://reviews.apache.org/r/14497/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 14497: Rebase v2
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14497/
-----------------------------------------------------------
(Updated Oct. 7, 2013, 5:58 p.m.)
Review request for kafka.
Summary (updated)
-----------------
Rebase v2
Bugs: KAFKA-1075
https://issues.apache.org/jira/browse/KAFKA-1075
Repository: kafka
Description
-------
KAFKA-1075.v2
KAFKA-1075.v1
Diffs (updated)
-----
core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala 08b4b7218f62f876eb95628bd6db78c0956c7f04
Diff: https://reviews.apache.org/r/14497/diff/
Testing
-------
Thanks,
Guozhang Wang
Re: Review Request 14497: Incorporate comments from v1
Posted by Neha Narkhede <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14497/#review26739
-----------------------------------------------------------
Ship it!
Ship It!
- Neha Narkhede
On Oct. 7, 2013, 5:25 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14497/
> -----------------------------------------------------------
>
> (Updated Oct. 7, 2013, 5:25 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1075
> https://issues.apache.org/jira/browse/KAFKA-1075
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1075.v2
>
>
> KAFKA-1075.v1
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala 08b4b7218f62f876eb95628bd6db78c0956c7f04
>
> Diff: https://reviews.apache.org/r/14497/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 14497: Incorporate comments from v1
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14497/
-----------------------------------------------------------
(Updated Oct. 7, 2013, 5:25 p.m.)
Review request for kafka.
Summary (updated)
-----------------
Incorporate comments from v1
Bugs: KAFKA-1075
https://issues.apache.org/jira/browse/KAFKA-1075
Repository: kafka
Description (updated)
-------
KAFKA-1075.v2
KAFKA-1075.v1
Diffs (updated)
-----
core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala 08b4b7218f62f876eb95628bd6db78c0956c7f04
Diff: https://reviews.apache.org/r/14497/diff/
Testing
-------
Thanks,
Guozhang Wang
Re: Review Request 14497: Incorporate comments from v1
Posted by Neha Narkhede <ne...@gmail.com>.
> On Oct. 7, 2013, 5 p.m., Neha Narkhede wrote:
> > core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala, line 319
> > <https://reviews.apache.org/r/14497/diff/1/?file=361459#file361459line319>
> >
> > This comment is a bit misleading. The data change watch for the /brokers/topics path is only set in reinitializeConsumer() which is invoked on startup and when new topics are discovered through wildcards. Doesn't zkclient automatically re-register watches once they are fired ?
>
> Guozhang Wang wrote:
> I am not sure if the zkClient auto re-register upon watcher fired, but I read from the source code that the watcher will be re-registered upon reading the subscribed path. I made the above comment since in rebalance the getPartitionAssignmentForTopics function call will read the /brokers/topics path and hence set the watchers.
>
> Guozhang Wang wrote:
> This is what I found for IZkDataListener (the English seems not very easy to understand though :P ):
>
> "An IZkDataListener can be registered at a ZkClient for listening on zk data changes for a given path. Node: Also this listener re-subscribes it watch for the path on each zk event (zk watches are one-timers) is is not guaranteed that events on the path are missing (see http://zookeeper.wiki.sourceforge.net/ZooKeeperWatches). An implementation of this class should take that into account. "
http://pastebin.com/pZQAXnxd. Zkclient does re-register the watch *before* reading the path, which is what I wanted to check. I think we are good to go here.
"I made the above comment since in rebalance the getPartitionAssignmentForTopics function call will read the /brokers/topics path and hence set the watchers."
This is not true. It re-registers the watch when it fires the data change event, not when you read the path.
It is worth making that clear in your comment.
- Neha
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14497/#review26705
-----------------------------------------------------------
On Oct. 7, 2013, 5:25 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14497/
> -----------------------------------------------------------
>
> (Updated Oct. 7, 2013, 5:25 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1075
> https://issues.apache.org/jira/browse/KAFKA-1075
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1075.v2
>
>
> KAFKA-1075.v1
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala 08b4b7218f62f876eb95628bd6db78c0956c7f04
>
> Diff: https://reviews.apache.org/r/14497/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 14497: Patch for KAFKA-1075
Posted by Guozhang Wang <gu...@linkedin.com>.
> On Oct. 7, 2013, 5 p.m., Neha Narkhede wrote:
> > core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala, line 319
> > <https://reviews.apache.org/r/14497/diff/1/?file=361459#file361459line319>
> >
> > This comment is a bit misleading. The data change watch for the /brokers/topics path is only set in reinitializeConsumer() which is invoked on startup and when new topics are discovered through wildcards. Doesn't zkclient automatically re-register watches once they are fired ?
I am not sure if the zkClient auto re-register upon watcher fired, but I read from the source code that the watcher will be re-registered upon reading the subscribed path. I made the above comment since in rebalance the getPartitionAssignmentForTopics function call will read the /brokers/topics path and hence set the watchers.
- Guozhang
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14497/#review26705
-----------------------------------------------------------
On Oct. 4, 2013, 10:23 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14497/
> -----------------------------------------------------------
>
> (Updated Oct. 4, 2013, 10:23 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1075
> https://issues.apache.org/jira/browse/KAFKA-1075
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1075.v1
>
>
> Diffs
> -----
>
> config/tools-log4j.properties 79240490149835656e2a013a9702c5aa41c104f1
> core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala 08b4b7218f62f876eb95628bd6db78c0956c7f04
>
> Diff: https://reviews.apache.org/r/14497/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 14497: Patch for KAFKA-1075
Posted by Guozhang Wang <gu...@linkedin.com>.
> On Oct. 7, 2013, 5 p.m., Neha Narkhede wrote:
> > core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala, line 319
> > <https://reviews.apache.org/r/14497/diff/1/?file=361459#file361459line319>
> >
> > This comment is a bit misleading. The data change watch for the /brokers/topics path is only set in reinitializeConsumer() which is invoked on startup and when new topics are discovered through wildcards. Doesn't zkclient automatically re-register watches once they are fired ?
>
> Guozhang Wang wrote:
> I am not sure if the zkClient auto re-register upon watcher fired, but I read from the source code that the watcher will be re-registered upon reading the subscribed path. I made the above comment since in rebalance the getPartitionAssignmentForTopics function call will read the /brokers/topics path and hence set the watchers.
This is what I found for IZkDataListener (the English seems not very easy to understand though :P ):
"An IZkDataListener can be registered at a ZkClient for listening on zk data changes for a given path. Node: Also this listener re-subscribes it watch for the path on each zk event (zk watches are one-timers) is is not guaranteed that events on the path are missing (see http://zookeeper.wiki.sourceforge.net/ZooKeeperWatches). An implementation of this class should take that into account. "
- Guozhang
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14497/#review26705
-----------------------------------------------------------
On Oct. 4, 2013, 10:23 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14497/
> -----------------------------------------------------------
>
> (Updated Oct. 4, 2013, 10:23 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1075
> https://issues.apache.org/jira/browse/KAFKA-1075
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1075.v1
>
>
> Diffs
> -----
>
> config/tools-log4j.properties 79240490149835656e2a013a9702c5aa41c104f1
> core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala 08b4b7218f62f876eb95628bd6db78c0956c7f04
>
> Diff: https://reviews.apache.org/r/14497/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 14497: Patch for KAFKA-1075
Posted by Neha Narkhede <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14497/#review26705
-----------------------------------------------------------
core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala
<https://reviews.apache.org/r/14497/#comment52044>
This comment is a bit misleading. The data change watch for the /brokers/topics path is only set in reinitializeConsumer() which is invoked on startup and when new topics are discovered through wildcards. Doesn't zkclient automatically re-register watches once they are fired ?
core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala
<https://reviews.apache.org/r/14497/#comment52014>
this comment is a little misleading. This just creates the listener, doesn't register anything yet
core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala
<https://reviews.apache.org/r/14497/#comment52013>
this comment is a little misleading. This just creates the listener, doesn't register anything yet
- Neha Narkhede
On Oct. 4, 2013, 10:23 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14497/
> -----------------------------------------------------------
>
> (Updated Oct. 4, 2013, 10:23 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1075
> https://issues.apache.org/jira/browse/KAFKA-1075
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1075.v1
>
>
> Diffs
> -----
>
> config/tools-log4j.properties 79240490149835656e2a013a9702c5aa41c104f1
> core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala 08b4b7218f62f876eb95628bd6db78c0956c7f04
>
> Diff: https://reviews.apache.org/r/14497/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 14497: Patch for KAFKA-1075
Posted by Guozhang Wang <gu...@linkedin.com>.
> On Oct. 6, 2013, 4:29 p.m., Jun Rao wrote:
> > With the new change, will a rebalance be triggered on a consumer when #partitions is changed on a consumed topic?
Yes, and it is tested through the add-partitions tool.
- Guozhang
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14497/#review26712
-----------------------------------------------------------
On Oct. 4, 2013, 10:23 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14497/
> -----------------------------------------------------------
>
> (Updated Oct. 4, 2013, 10:23 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1075
> https://issues.apache.org/jira/browse/KAFKA-1075
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1075.v1
>
>
> Diffs
> -----
>
> config/tools-log4j.properties 79240490149835656e2a013a9702c5aa41c104f1
> core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala 08b4b7218f62f876eb95628bd6db78c0956c7f04
>
> Diff: https://reviews.apache.org/r/14497/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 14497: Patch for KAFKA-1075
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14497/#review26712
-----------------------------------------------------------
With the new change, will a rebalance be triggered on a consumer when #partitions is changed on a consumed topic?
config/tools-log4j.properties
<https://reviews.apache.org/r/14497/#comment52026>
This change is probably not intended.
core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala
<https://reviews.apache.org/r/14497/#comment52027>
This is a bit misleading. We are not registering the listener here. We only instantiated the listener. Ditto for the comment above sessionExpirationListener.
- Jun Rao
On Oct. 4, 2013, 10:23 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14497/
> -----------------------------------------------------------
>
> (Updated Oct. 4, 2013, 10:23 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1075
> https://issues.apache.org/jira/browse/KAFKA-1075
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1075.v1
>
>
> Diffs
> -----
>
> config/tools-log4j.properties 79240490149835656e2a013a9702c5aa41c104f1
> core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala 08b4b7218f62f876eb95628bd6db78c0956c7f04
>
> Diff: https://reviews.apache.org/r/14497/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>