You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Sriharsha Chintalapani <ha...@hortonworks.com> on 2014/07/15 03:18:27 UTC

Review Request 23474: Patch for KAFKA-1483

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23474/
-----------------------------------------------------------

Review request for kafka.


Bugs: KAFKA-1483
    https://issues.apache.org/jira/browse/KAFKA-1483


Repository: kafka


Description
-------

KAFKA-1483. Split Brain about Leader Partitions.


Diffs
-----

  core/src/main/scala/kafka/server/ReplicaManager.scala 6a56a772c134dbf1e70c1bfe067223009bfdbac8 

Diff: https://reviews.apache.org/r/23474/diff/


Testing
-------


Thanks,

Sriharsha Chintalapani

Re: Review Request 23474: Patch for KAFKA-1483

Posted by Sriharsha Chintalapani <ha...@hortonworks.com>.

> On July 15, 2014, 5:51 p.m., Guozhang Wang wrote:
> > core/src/main/scala/kafka/server/ReplicaManager.scala, line 484
> > <https://reviews.apache.org/r/23474/diff/1/?file=631878#file631878line484>
> >
> >     I am wondering if this function will introduce more lock contention on leaderIsrUpdateLock? Every leaderReplicaIfLocal call will need to hold on its read lock while other requests at the same time may try to modify the leader option?
> 
> Sriharsha Chintalapani wrote:
>     It might increase lock contention but writes to change the leadership occurs not so frequently so using readLock wouldn't be much of an issue I think. I also considered comparing ReplicaManager.localBrokerId with Partition.leaderReplicaIdOpt without a lock this will present an issue where the leader is changed and we might be comparing against old leader value.
>     Instead of using leaderReplicaIfLocal I can create another method "checkLeader" in Partition which will return a boolean after obtaining a readLock on leaderIsrUpdateLock. Right now there is unnecessary call going to getReplica.
> 
> Guozhang Wang wrote:
>     I think one optimization we can try at least is to grab the read lock before the iteration to avoid grab/release on each partition during the iteration.
> 
> Jun Rao wrote:
>     The intention of maintaining a separate leaderPartitions is to make LeaderCount check an O(1) operation, instead of O(n). However, UnderReplicatedPartitions is still an O(n) operation and it's already paying the readLock overhead. So, making LeaderCount an O(n) operation with the readLock overhead probably won't be too bad.

one approach to eliminate leaderIsrUpdateLock is to wrap allPartitions in another ReadWriteLock and not use the partition.leaderIsrUpdateLock for leader check. Let me know if that makes sense or if we are ok with the current approach. Thanks.


- Sriharsha


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23474/#review47785
-----------------------------------------------------------


On July 16, 2014, 6:07 p.m., Sriharsha Chintalapani wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23474/
> -----------------------------------------------------------
> 
> (Updated July 16, 2014, 6:07 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1483
>     https://issues.apache.org/jira/browse/KAFKA-1483
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1483. Split Brain about Leader Partitions.
> 
> 
> Diffs
> -----
> 
>   core/src/main/scala/kafka/server/ReplicaManager.scala 6a56a772c134dbf1e70c1bfe067223009bfdbac8 
> 
> Diff: https://reviews.apache.org/r/23474/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Sriharsha Chintalapani
> 
>

Re: Review Request 23474: Patch for KAFKA-1483

Posted by Sriharsha Chintalapani <ha...@hortonworks.com>.

> On July 15, 2014, 5:51 p.m., Guozhang Wang wrote:
> > core/src/main/scala/kafka/server/ReplicaManager.scala, line 484
> > <https://reviews.apache.org/r/23474/diff/1/?file=631878#file631878line484>
> >
> >     I am wondering if this function will introduce more lock contention on leaderIsrUpdateLock? Every leaderReplicaIfLocal call will need to hold on its read lock while other requests at the same time may try to modify the leader option?

It might increase lock contention but writes to change the leadership occurs not so frequently so using readLock wouldn't be much of an issue I think. I also considered comparing ReplicaManager.localBrokerId with Partition.leaderReplicaIdOpt without a lock this will present an issue where the leader is changed and we might be comparing against old leader value.
Instead of using leaderReplicaIfLocal I can create another method "checkLeader" in Partition which will return a boolean after obtaining a readLock on leaderIsrUpdateLock. Right now there is unnecessary call going to getReplica.


- Sriharsha


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23474/#review47785
-----------------------------------------------------------


On July 15, 2014, 1:18 a.m., Sriharsha Chintalapani wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23474/
> -----------------------------------------------------------
> 
> (Updated July 15, 2014, 1:18 a.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1483
>     https://issues.apache.org/jira/browse/KAFKA-1483
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1483. Split Brain about Leader Partitions.
> 
> 
> Diffs
> -----
> 
>   core/src/main/scala/kafka/server/ReplicaManager.scala 6a56a772c134dbf1e70c1bfe067223009bfdbac8 
> 
> Diff: https://reviews.apache.org/r/23474/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Sriharsha Chintalapani
> 
>

Re: Review Request 23474: Patch for KAFKA-1483

Posted by Guozhang Wang <gu...@linkedin.com>.

> On July 15, 2014, 5:51 p.m., Guozhang Wang wrote:
> > core/src/main/scala/kafka/server/ReplicaManager.scala, line 484
> > <https://reviews.apache.org/r/23474/diff/1/?file=631878#file631878line484>
> >
> >     I am wondering if this function will introduce more lock contention on leaderIsrUpdateLock? Every leaderReplicaIfLocal call will need to hold on its read lock while other requests at the same time may try to modify the leader option?
> 
> Sriharsha Chintalapani wrote:
>     It might increase lock contention but writes to change the leadership occurs not so frequently so using readLock wouldn't be much of an issue I think. I also considered comparing ReplicaManager.localBrokerId with Partition.leaderReplicaIdOpt without a lock this will present an issue where the leader is changed and we might be comparing against old leader value.
>     Instead of using leaderReplicaIfLocal I can create another method "checkLeader" in Partition which will return a boolean after obtaining a readLock on leaderIsrUpdateLock. Right now there is unnecessary call going to getReplica.

I think one optimization we can try at least is to grab the read lock before the iteration to avoid grab/release on each partition during the iteration.


- Guozhang


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23474/#review47785
-----------------------------------------------------------


On July 15, 2014, 1:18 a.m., Sriharsha Chintalapani wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23474/
> -----------------------------------------------------------
> 
> (Updated July 15, 2014, 1:18 a.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1483
>     https://issues.apache.org/jira/browse/KAFKA-1483
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1483. Split Brain about Leader Partitions.
> 
> 
> Diffs
> -----
> 
>   core/src/main/scala/kafka/server/ReplicaManager.scala 6a56a772c134dbf1e70c1bfe067223009bfdbac8 
> 
> Diff: https://reviews.apache.org/r/23474/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Sriharsha Chintalapani
> 
>

Re: Review Request 23474: Patch for KAFKA-1483

Posted by Jun Rao <ju...@gmail.com>.

> On July 15, 2014, 5:51 p.m., Guozhang Wang wrote:
> > core/src/main/scala/kafka/server/ReplicaManager.scala, line 484
> > <https://reviews.apache.org/r/23474/diff/1/?file=631878#file631878line484>
> >
> >     I am wondering if this function will introduce more lock contention on leaderIsrUpdateLock? Every leaderReplicaIfLocal call will need to hold on its read lock while other requests at the same time may try to modify the leader option?
> 
> Sriharsha Chintalapani wrote:
>     It might increase lock contention but writes to change the leadership occurs not so frequently so using readLock wouldn't be much of an issue I think. I also considered comparing ReplicaManager.localBrokerId with Partition.leaderReplicaIdOpt without a lock this will present an issue where the leader is changed and we might be comparing against old leader value.
>     Instead of using leaderReplicaIfLocal I can create another method "checkLeader" in Partition which will return a boolean after obtaining a readLock on leaderIsrUpdateLock. Right now there is unnecessary call going to getReplica.
> 
> Guozhang Wang wrote:
>     I think one optimization we can try at least is to grab the read lock before the iteration to avoid grab/release on each partition during the iteration.

The intention of maintaining a separate leaderPartitions is to make LeaderCount check an O(1) operation, instead of O(n). However, UnderReplicatedPartitions is still an O(n) operation and it's already paying the readLock overhead. So, making LeaderCount an O(n) operation with the readLock overhead probably won't be too bad.


- Jun


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23474/#review47785
-----------------------------------------------------------


On July 15, 2014, 1:18 a.m., Sriharsha Chintalapani wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23474/
> -----------------------------------------------------------
> 
> (Updated July 15, 2014, 1:18 a.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1483
>     https://issues.apache.org/jira/browse/KAFKA-1483
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1483. Split Brain about Leader Partitions.
> 
> 
> Diffs
> -----
> 
>   core/src/main/scala/kafka/server/ReplicaManager.scala 6a56a772c134dbf1e70c1bfe067223009bfdbac8 
> 
> Diff: https://reviews.apache.org/r/23474/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Sriharsha Chintalapani
> 
>

Re: Review Request 23474: Patch for KAFKA-1483

Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23474/#review47785
-----------------------------------------------------------



core/src/main/scala/kafka/server/ReplicaManager.scala
<https://reviews.apache.org/r/23474/#comment83980>

    I am wondering if this function will introduce more lock contention on leaderIsrUpdateLock? Every leaderReplicaIfLocal call will need to hold on its read lock while other requests at the same time may try to modify the leader option? 


- Guozhang Wang


On July 15, 2014, 1:18 a.m., Sriharsha Chintalapani wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23474/
> -----------------------------------------------------------
> 
> (Updated July 15, 2014, 1:18 a.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1483
>     https://issues.apache.org/jira/browse/KAFKA-1483
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1483. Split Brain about Leader Partitions.
> 
> 
> Diffs
> -----
> 
>   core/src/main/scala/kafka/server/ReplicaManager.scala 6a56a772c134dbf1e70c1bfe067223009bfdbac8 
> 
> Diff: https://reviews.apache.org/r/23474/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Sriharsha Chintalapani
> 
>

Re: Review Request 23474: Patch for KAFKA-1483

Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23474/#review47884
-----------------------------------------------------------



core/src/main/scala/kafka/server/ReplicaManager.scala
<https://reviews.apache.org/r/23474/#comment84115>

    It seems that we can just feed all partitions to do the maybeShrinkIsr() check. That function already checks the leader info. Then, we can get rid of leaderParitionsLock.


- Jun Rao


On July 15, 2014, 1:18 a.m., Sriharsha Chintalapani wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23474/
> -----------------------------------------------------------
> 
> (Updated July 15, 2014, 1:18 a.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1483
>     https://issues.apache.org/jira/browse/KAFKA-1483
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1483. Split Brain about Leader Partitions.
> 
> 
> Diffs
> -----
> 
>   core/src/main/scala/kafka/server/ReplicaManager.scala 6a56a772c134dbf1e70c1bfe067223009bfdbac8 
> 
> Diff: https://reviews.apache.org/r/23474/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Sriharsha Chintalapani
> 
>

Re: Review Request 23474: Patch for KAFKA-1483

Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23474/#review48028
-----------------------------------------------------------

Ship it!


- Guozhang Wang


On July 16, 2014, 6:07 p.m., Sriharsha Chintalapani wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23474/
> -----------------------------------------------------------
> 
> (Updated July 16, 2014, 6:07 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1483
>     https://issues.apache.org/jira/browse/KAFKA-1483
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1483. Split Brain about Leader Partitions.
> 
> 
> Diffs
> -----
> 
>   core/src/main/scala/kafka/server/ReplicaManager.scala 6a56a772c134dbf1e70c1bfe067223009bfdbac8 
> 
> Diff: https://reviews.apache.org/r/23474/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Sriharsha Chintalapani
> 
>

Re: Review Request 23474: Patch for KAFKA-1483

Posted by Neha Narkhede <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23474/#review48624
-----------------------------------------------------------

Ship it!


Ship It!

- Neha Narkhede


On July 16, 2014, 6:07 p.m., Sriharsha Chintalapani wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23474/
> -----------------------------------------------------------
> 
> (Updated July 16, 2014, 6:07 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1483
>     https://issues.apache.org/jira/browse/KAFKA-1483
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1483. Split Brain about Leader Partitions.
> 
> 
> Diffs
> -----
> 
>   core/src/main/scala/kafka/server/ReplicaManager.scala 6a56a772c134dbf1e70c1bfe067223009bfdbac8 
> 
> Diff: https://reviews.apache.org/r/23474/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Sriharsha Chintalapani
> 
>

Re: Review Request 23474: Patch for KAFKA-1483

Posted by Sriharsha Chintalapani <ha...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23474/
-----------------------------------------------------------

(Updated July 16, 2014, 6:07 p.m.)


Review request for kafka.


Bugs: KAFKA-1483
    https://issues.apache.org/jira/browse/KAFKA-1483


Repository: kafka


Description
-------

KAFKA-1483. Split Brain about Leader Partitions.


Diffs (updated)
-----

  core/src/main/scala/kafka/server/ReplicaManager.scala 6a56a772c134dbf1e70c1bfe067223009bfdbac8 

Diff: https://reviews.apache.org/r/23474/diff/


Testing
-------


Thanks,

Sriharsha Chintalapani