You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Timothy Chen <tn...@apache.org> on 2014/04/10 03:48:05 UTC
Review Request 20190: Patch for KAFKA-1356
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20190/
-----------------------------------------------------------
Review request for kafka.
Bugs: KAFKA-1356
https://issues.apache.org/jira/browse/KAFKA-1356
Repository: kafka
Description
-------
Refactor metadata cache code in kafka api and remove lock on ensureTopicExists
Diffs
-----
core/src/main/scala/kafka/server/KafkaApis.scala d96229e2d4aa7006b0dbd81055ce5a2459d8758c
Diff: https://reviews.apache.org/r/20190/diff/
Testing
-------
Thanks,
Timothy Chen
Re: Review Request 20190: Patch for KAFKA-1356
Posted by Timothy Chen <tn...@apache.org>.
> On April 10, 2014, 2:57 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/server/KafkaApis.scala, lines 62-63
> > <https://reviews.apache.org/r/20190/diff/1/?file=554248#file554248line62>
> >
> > Could we make this class private?
Unit tests seems to rely on accessing the cache to verify some behaviors, so I thought to leave it open as is.
> On April 10, 2014, 2:57 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/server/KafkaApis.scala, lines 283-284
> > <https://reviews.apache.org/r/20190/diff/1/?file=554248#file554248line283>
> >
> > I am not sure that we should just blindly set the controllerEpoch in ReplicaManager. ReplicaManager is supposed to hold the latest controller epoch so that it can disregard requests sent from an older controller. So, we only need to update controllerEpoch if the new value is higher than the current one. Perhaps we can add a util function in ReplicaManager to do that.
Talked to Jun and we agree the previous check is sufficient here.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20190/#review40012
-----------------------------------------------------------
On April 10, 2014, 1:48 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20190/
> -----------------------------------------------------------
>
> (Updated April 10, 2014, 1:48 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1356
> https://issues.apache.org/jira/browse/KAFKA-1356
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Refactor metadata cache code in kafka api and remove lock on ensureTopicExists
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/KafkaApis.scala d96229e2d4aa7006b0dbd81055ce5a2459d8758c
>
> Diff: https://reviews.apache.org/r/20190/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 20190: Patch for KAFKA-1356
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20190/#review40012
-----------------------------------------------------------
Thanks for the patch. Some comments below.
core/src/main/scala/kafka/server/KafkaApis.scala
<https://reviews.apache.org/r/20190/#comment72799>
Could we make this class private?
core/src/main/scala/kafka/server/KafkaApis.scala
<https://reviews.apache.org/r/20190/#comment72795>
Since we don't hold the lock while checking the cache, there is no memory barrier crossed and there is no guarantee that we will see the current value in the cache. So, I don't see the value of calling ensureTopicExists. Plus, ensureTopicExists is needed only as an optimization, not for correctness. I recommend that we remove the check on ensureTopicExists.
core/src/main/scala/kafka/server/KafkaApis.scala
<https://reviews.apache.org/r/20190/#comment72798>
I am not sure that we should just blindly set the controllerEpoch in ReplicaManager. ReplicaManager is supposed to hold the latest controller epoch so that it can disregard requests sent from an older controller. So, we only need to update controllerEpoch if the new value is higher than the current one. Perhaps we can add a util function in ReplicaManager to do that.
- Jun Rao
On April 10, 2014, 1:48 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20190/
> -----------------------------------------------------------
>
> (Updated April 10, 2014, 1:48 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1356
> https://issues.apache.org/jira/browse/KAFKA-1356
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Refactor metadata cache code in kafka api and remove lock on ensureTopicExists
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/KafkaApis.scala d96229e2d4aa7006b0dbd81055ce5a2459d8758c
>
> Diff: https://reviews.apache.org/r/20190/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 20190: Patch for KAFKA-1356
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20190/#review39992
-----------------------------------------------------------
Ship it!
core/src/main/scala/kafka/server/KafkaApis.scala
<https://reviews.apache.org/r/20190/#comment72780>
Could this be renamed to sth. like "TopicPartitionState"?
- Guozhang Wang
On April 10, 2014, 1:48 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20190/
> -----------------------------------------------------------
>
> (Updated April 10, 2014, 1:48 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1356
> https://issues.apache.org/jira/browse/KAFKA-1356
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Refactor metadata cache code in kafka api and remove lock on ensureTopicExists
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/KafkaApis.scala d96229e2d4aa7006b0dbd81055ce5a2459d8758c
>
> Diff: https://reviews.apache.org/r/20190/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Timothy Chen
>
>