You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Jiangjie Qin <be...@gmail.com> on 2014/10/06 19:06:54 UTC
Review Request 26373: Patch for KAFKA-1647
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26373/
-----------------------------------------------------------
Review request for kafka.
Bugs: KAFKA-1647
https://issues.apache.org/jira/browse/KAFKA-1647
Repository: kafka
Description
-------
Fix for Kafka-1647.
Diffs
-----
core/src/main/scala/kafka/server/ReplicaManager.scala 78b7514cc109547c562e635824684fad581af653
Diff: https://reviews.apache.org/r/26373/diff/
Testing
-------
Thanks,
Jiangjie Qin
Re: Review Request 26373: Patch for KAFKA-1647
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26373/#review56434
-----------------------------------------------------------
core/src/main/scala/kafka/server/ReplicaManager.scala
<https://reviews.apache.org/r/26373/#comment96736>
if (!partition.makeFollower(...))
However, see the comment further down on this.
core/src/main/scala/kafka/server/ReplicaManager.scala
<https://reviews.apache.org/r/26373/#comment96747>
Also, if the above change is to be made maybe we should remove this info message. However, as mentioned further below we should probably skip this partition altogether.
core/src/main/scala/kafka/server/ReplicaManager.scala
<https://reviews.apache.org/r/26373/#comment96742>
Can tweak this a bit? How about:
Broker %d received LeaderAndIsrRequest with correlation id %d from controller %d epoch %d for partition [%s,%d] but cannot become follower since the new leader %d is unavailable.
core/src/main/scala/kafka/server/ReplicaManager.scala
<https://reviews.apache.org/r/26373/#comment96745>
If a replica is already following a leader, it is unnecessary to stop the fetcher, truncate to HW and restart the fetcher. So I think we should still skip such partitions. (The associated replica will already be in the checkpoint array so its checkpoint won't get lost)
- Joel Koshy
On Oct. 6, 2014, 5:06 p.m., Jiangjie Qin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26373/
> -----------------------------------------------------------
>
> (Updated Oct. 6, 2014, 5:06 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1647
> https://issues.apache.org/jira/browse/KAFKA-1647
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Fix for Kafka-1647.
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/ReplicaManager.scala 78b7514cc109547c562e635824684fad581af653
>
> Diff: https://reviews.apache.org/r/26373/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jiangjie Qin
>
>
Re: Review Request 26373: Patch for KAFKA-1647
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26373/#review56736
-----------------------------------------------------------
Ship it!
Ship It!
- Guozhang Wang
On Oct. 13, 2014, 11:38 p.m., Jiangjie Qin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26373/
> -----------------------------------------------------------
>
> (Updated Oct. 13, 2014, 11:38 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1647
> https://issues.apache.org/jira/browse/KAFKA-1647
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Addressed Joel's comments.
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/ReplicaManager.scala 78b7514cc109547c562e635824684fad581af653
>
> Diff: https://reviews.apache.org/r/26373/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jiangjie Qin
>
>
Re: Review Request 26373: Patch for KAFKA-1647
Posted by Neha Narkhede <ne...@gmail.com>.
> On Oct. 21, 2014, 12:18 a.m., Jun Rao wrote:
> > core/src/main/scala/kafka/server/ReplicaManager.scala, lines 481-506
> > <https://reviews.apache.org/r/26373/diff/3/?file=725140#file725140line481>
> >
> > This doesn't quite fix the original problem though. The original problem is that if the leader is not alive, we won't call partition.makeFollower(), in which the local replica is created. If a local replica is not created, the partition will be ignored when checkingpoint HW and we lose the last checkpointed HW.
> >
> > So, we have to call partition.makerFollower() for every follower, whether its leader is live or not. After this, we can proceed with the rest of the steps for only those partitions with a live leader. We can log a warning for those partitions w/o a live leader.
>
> Jiangjie Qin wrote:
> Hi Jun, thanks for the review.
> I think the high watermark is actually read in partition.getOrCreateReplica(). As you said, that means in order to preserve the high watermark, the local replica is supposed to be created. The original code did not create local replica when remote leader is not up so it lost the high watermark.
> partition.getOrCreateReplica() is actually called in the following 2 places:
> 1. partition.makeFollower()
> 2. ReplicaManager line 515, when we truncate the log for the partitions in partitionsToMakeFollower.
> Both of them could preserve high watermark. The difference between them is that in (1) all the replica for the partition, including the not-existing-yet remote replica, will be created. However in (2) only local replica will be created. Because I'm not 100% sure if it could cause some other issue if we create a remote replica when it is actually not up yet, so I chose to preserve the high watermark in (2) instead of in (1), which has less change compared with original code.
Did you get a chance to repeat the kind of testing that was done to find this bug? I'd be more comfortable accepting this patch if we did that sort of testing since this change is tricky.
- Neha
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26373/#review57490
-----------------------------------------------------------
On Oct. 18, 2014, 7:26 a.m., Jiangjie Qin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26373/
> -----------------------------------------------------------
>
> (Updated Oct. 18, 2014, 7:26 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1647
> https://issues.apache.org/jira/browse/KAFKA-1647
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Addressed Joel's comments.
>
>
> the version 2 code seems to be submitted by mistake... This should be the code for review that addressed Joel's comments.
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/ReplicaManager.scala 78b7514cc109547c562e635824684fad581af653
>
> Diff: https://reviews.apache.org/r/26373/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jiangjie Qin
>
>
Re: Review Request 26373: Patch for KAFKA-1647
Posted by Jiangjie Qin <be...@gmail.com>.
> On Oct. 21, 2014, 12:18 a.m., Jun Rao wrote:
> > core/src/main/scala/kafka/server/ReplicaManager.scala, lines 481-506
> > <https://reviews.apache.org/r/26373/diff/3/?file=725140#file725140line481>
> >
> > This doesn't quite fix the original problem though. The original problem is that if the leader is not alive, we won't call partition.makeFollower(), in which the local replica is created. If a local replica is not created, the partition will be ignored when checkingpoint HW and we lose the last checkpointed HW.
> >
> > So, we have to call partition.makerFollower() for every follower, whether its leader is live or not. After this, we can proceed with the rest of the steps for only those partitions with a live leader. We can log a warning for those partitions w/o a live leader.
Hi Jun, thanks for the review.
I think the high watermark is actually read in partition.getOrCreateReplica(). As you said, that means in order to preserve the high watermark, the local replica is supposed to be created. The original code did not create local replica when remote leader is not up so it lost the high watermark.
partition.getOrCreateReplica() is actually called in the following 2 places:
1. partition.makeFollower()
2. ReplicaManager line 515, when we truncate the log for the partitions in partitionsToMakeFollower.
Both of them could preserve high watermark. The difference between them is that in (1) all the replica for the partition, including the not-existing-yet remote replica, will be created. However in (2) only local replica will be created. Because I'm not 100% sure if it could cause some other issue if we create a remote replica when it is actually not up yet, so I chose to preserve the high watermark in (2) instead of in (1), which has less change compared with original code.
- Jiangjie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26373/#review57490
-----------------------------------------------------------
On Oct. 18, 2014, 7:26 a.m., Jiangjie Qin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26373/
> -----------------------------------------------------------
>
> (Updated Oct. 18, 2014, 7:26 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1647
> https://issues.apache.org/jira/browse/KAFKA-1647
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Addressed Joel's comments.
>
>
> the version 2 code seems to be submitted by mistake... This should be the code for review that addressed Joel's comments.
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/ReplicaManager.scala 78b7514cc109547c562e635824684fad581af653
>
> Diff: https://reviews.apache.org/r/26373/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jiangjie Qin
>
>
Re: Review Request 26373: Patch for KAFKA-1647
Posted by Jun Rao <ju...@gmail.com>.
> On Oct. 21, 2014, 12:18 a.m., Jun Rao wrote:
> > core/src/main/scala/kafka/server/ReplicaManager.scala, lines 481-506
> > <https://reviews.apache.org/r/26373/diff/3/?file=725140#file725140line481>
> >
> > This doesn't quite fix the original problem though. The original problem is that if the leader is not alive, we won't call partition.makeFollower(), in which the local replica is created. If a local replica is not created, the partition will be ignored when checkingpoint HW and we lose the last checkpointed HW.
> >
> > So, we have to call partition.makerFollower() for every follower, whether its leader is live or not. After this, we can proceed with the rest of the steps for only those partitions with a live leader. We can log a warning for those partitions w/o a live leader.
>
> Jiangjie Qin wrote:
> Hi Jun, thanks for the review.
> I think the high watermark is actually read in partition.getOrCreateReplica(). As you said, that means in order to preserve the high watermark, the local replica is supposed to be created. The original code did not create local replica when remote leader is not up so it lost the high watermark.
> partition.getOrCreateReplica() is actually called in the following 2 places:
> 1. partition.makeFollower()
> 2. ReplicaManager line 515, when we truncate the log for the partitions in partitionsToMakeFollower.
> Both of them could preserve high watermark. The difference between them is that in (1) all the replica for the partition, including the not-existing-yet remote replica, will be created. However in (2) only local replica will be created. Because I'm not 100% sure if it could cause some other issue if we create a remote replica when it is actually not up yet, so I chose to preserve the high watermark in (2) instead of in (1), which has less change compared with original code.
>
> Neha Narkhede wrote:
> Did you get a chance to repeat the kind of testing that was done to find this bug? I'd be more comfortable accepting this patch if we did that sort of testing since this change is tricky.
Thanks for the explanation. Got it. On the other hand, if the leader is not ready, perhaps it's better not to truncate the data in the follower since this could cause some unnecessary data loss if the follower is later selected as the new leader.
- Jun
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26373/#review57490
-----------------------------------------------------------
On Oct. 18, 2014, 7:26 a.m., Jiangjie Qin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26373/
> -----------------------------------------------------------
>
> (Updated Oct. 18, 2014, 7:26 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1647
> https://issues.apache.org/jira/browse/KAFKA-1647
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Addressed Joel's comments.
>
>
> the version 2 code seems to be submitted by mistake... This should be the code for review that addressed Joel's comments.
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/ReplicaManager.scala 78b7514cc109547c562e635824684fad581af653
>
> Diff: https://reviews.apache.org/r/26373/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jiangjie Qin
>
>
Re: Review Request 26373: Patch for KAFKA-1647
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26373/#review57490
-----------------------------------------------------------
Thanks for the patch. Some comments below.
core/src/main/scala/kafka/server/ReplicaManager.scala
<https://reviews.apache.org/r/26373/#comment98208>
This doesn't quite fix the original problem though. The original problem is that if the leader is not alive, we won't call partition.makeFollower(), in which the local replica is created. If a local replica is not created, the partition will be ignored when checkingpoint HW and we lose the last checkpointed HW.
So, we have to call partition.makerFollower() for every follower, whether its leader is live or not. After this, we can proceed with the rest of the steps for only those partitions with a live leader. We can log a warning for those partitions w/o a live leader.
- Jun Rao
On Oct. 18, 2014, 7:26 a.m., Jiangjie Qin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26373/
> -----------------------------------------------------------
>
> (Updated Oct. 18, 2014, 7:26 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1647
> https://issues.apache.org/jira/browse/KAFKA-1647
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Addressed Joel's comments.
>
>
> the version 2 code seems to be submitted by mistake... This should be the code for review that addressed Joel's comments.
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/ReplicaManager.scala 78b7514cc109547c562e635824684fad581af653
>
> Diff: https://reviews.apache.org/r/26373/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jiangjie Qin
>
>
Re: Review Request 26373: Patch for KAFKA-1647
Posted by Jiangjie Qin <be...@gmail.com>.
> On Oct. 23, 2014, 12:42 a.m., Jun Rao wrote:
> > core/src/main/scala/kafka/server/ReplicaManager.scala, lines 531-538
> > <https://reviews.apache.org/r/26373/diff/4/?file=728653#file728653line531>
> >
> > I am not sure that I understand how Option.flatten works. Would it be clearly if we first filter out partitions with no live leader and then generate the TopicAndPartition to BrokerAndInitialOffset map?
This change actually should be reverted as Joel suggested because we no longer add the partition without leader to partitionsToMakeFollower any more, instead, we only create local replica for it. This section should be exactly same as the original code.
I put flattern here to just remove the None option from a set...
- Jiangjie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26373/#review57947
-----------------------------------------------------------
On Oct. 22, 2014, 6:08 a.m., Jiangjie Qin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26373/
> -----------------------------------------------------------
>
> (Updated Oct. 22, 2014, 6:08 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1647
> https://issues.apache.org/jira/browse/KAFKA-1647
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Addressed Joel's comments.
>
>
> the version 2 code seems to be submitted by mistake... This should be the code for review that addressed Joel's comments.
>
>
> Addressed Jun's comments. Will do tests to verify if it works.
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/ReplicaManager.scala 78b7514cc109547c562e635824684fad581af653
>
> Diff: https://reviews.apache.org/r/26373/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jiangjie Qin
>
>
Re: Review Request 26373: Patch for KAFKA-1647
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26373/#review57947
-----------------------------------------------------------
core/src/main/scala/kafka/server/ReplicaManager.scala
<https://reviews.apache.org/r/26373/#comment98856>
I am not sure that I understand how Option.flatten works. Would it be clearly if we first filter out partitions with no live leader and then generate the TopicAndPartition to BrokerAndInitialOffset map?
- Jun Rao
On Oct. 22, 2014, 6:08 a.m., Jiangjie Qin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26373/
> -----------------------------------------------------------
>
> (Updated Oct. 22, 2014, 6:08 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1647
> https://issues.apache.org/jira/browse/KAFKA-1647
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Addressed Joel's comments.
>
>
> the version 2 code seems to be submitted by mistake... This should be the code for review that addressed Joel's comments.
>
>
> Addressed Jun's comments. Will do tests to verify if it works.
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/ReplicaManager.scala 78b7514cc109547c562e635824684fad581af653
>
> Diff: https://reviews.apache.org/r/26373/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jiangjie Qin
>
>
Re: Review Request 26373: Patch for KAFKA-1647
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26373/#review58748
-----------------------------------------------------------
Ship it!
Looks good to me. Can you make these final edits and upload another RB?
core/src/main/scala/kafka/server/ReplicaManager.scala
<https://reviews.apache.org/r/26373/#comment99891>
The .format needs to be on this line. Can you fix it and upload a new patch?
core/src/main/scala/kafka/server/ReplicaManager.scala
<https://reviews.apache.org/r/26373/#comment99892>
Small edits:
// Create the local replica even if the leader is unavailable. This is required to ensure that we include the partition's high watermark in the checkpoint file (see KAFKA-1647)
Also, I'm not sure if we need to explicitly reference the jira in comments since people can just git annotate.
- Joel Koshy
On Oct. 28, 2014, 12:20 a.m., Jiangjie Qin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26373/
> -----------------------------------------------------------
>
> (Updated Oct. 28, 2014, 12:20 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1647
> https://issues.apache.org/jira/browse/KAFKA-1647
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Addressed Joel's comments.
>
>
> the version 2 code seems to be submitted by mistake... This should be the code for review that addressed Joel's comments.
>
>
> Addressed Jun's comments. Will do tests to verify if it works.
>
>
> Addressed Joel's comments, we do not need to check the if leader exits for not when adding fetcher.
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/ReplicaManager.scala 78b7514cc109547c562e635824684fad581af653
>
> Diff: https://reviews.apache.org/r/26373/diff/
>
>
> Testing
> -------
>
> Followed Joel's testing step. I was able to reproduce the problem without the patch and the WARN message goes away after applied the patch.
>
>
> Thanks,
>
> Jiangjie Qin
>
>
Re: Review Request 26373: Patch for KAFKA-1647
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26373/#review58846
-----------------------------------------------------------
Ship it!
core/src/main/scala/kafka/server/ReplicaManager.scala
<https://reviews.apache.org/r/26373/#comment100003>
Sorry - this is fine. I missed the parantheses
- Joel Koshy
On Oct. 28, 2014, 12:20 a.m., Jiangjie Qin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26373/
> -----------------------------------------------------------
>
> (Updated Oct. 28, 2014, 12:20 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1647
> https://issues.apache.org/jira/browse/KAFKA-1647
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Addressed Joel's comments.
>
>
> the version 2 code seems to be submitted by mistake... This should be the code for review that addressed Joel's comments.
>
>
> Addressed Jun's comments. Will do tests to verify if it works.
>
>
> Addressed Joel's comments, we do not need to check the if leader exits for not when adding fetcher.
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/ReplicaManager.scala 78b7514cc109547c562e635824684fad581af653
>
> Diff: https://reviews.apache.org/r/26373/diff/
>
>
> Testing
> -------
>
> Followed Joel's testing step. I was able to reproduce the problem without the patch and the WARN message goes away after applied the patch.
>
>
> Thanks,
>
> Jiangjie Qin
>
>
Re: Review Request 26373: Patch for KAFKA-1647
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26373/#review59274
-----------------------------------------------------------
Ship it!
Ship It!
- Joel Koshy
On Oct. 30, 2014, 10:50 p.m., Jiangjie Qin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26373/
> -----------------------------------------------------------
>
> (Updated Oct. 30, 2014, 10:50 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1647
> https://issues.apache.org/jira/browse/KAFKA-1647
>
>
> Repository: kafka
>
>
> Description
> -------
>
> rebased for KAFKA-1647
>
>
> rebased for KAFKA-1647
>
>
> rebased for KAFKA-1647
>
>
> rebased for KAFKA-1647
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/ReplicaManager.scala 02fa3821271e97b24fd2ae25163933222797585d
>
> Diff: https://reviews.apache.org/r/26373/diff/
>
>
> Testing
> -------
>
> Followed Joel's testing step. I was able to reproduce the problem without the patch and the WARN message goes away after applied the patch.
>
>
> Thanks,
>
> Jiangjie Qin
>
>
Re: Review Request 26373: Patch for KAFKA-1647
Posted by Jiangjie Qin <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26373/
-----------------------------------------------------------
(Updated Oct. 30, 2014, 10:50 p.m.)
Review request for kafka.
Bugs: KAFKA-1647
https://issues.apache.org/jira/browse/KAFKA-1647
Repository: kafka
Description (updated)
-------
rebased for KAFKA-1647
rebased for KAFKA-1647
rebased for KAFKA-1647
rebased for KAFKA-1647
Diffs (updated)
-----
core/src/main/scala/kafka/server/ReplicaManager.scala 02fa3821271e97b24fd2ae25163933222797585d
Diff: https://reviews.apache.org/r/26373/diff/
Testing
-------
Followed Joel's testing step. I was able to reproduce the problem without the patch and the WARN message goes away after applied the patch.
Thanks,
Jiangjie Qin
Re: Review Request 26373: Patch for KAFKA-1647
Posted by Jiangjie Qin <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26373/
-----------------------------------------------------------
(Updated Oct. 30, 2014, 10:38 p.m.)
Review request for kafka.
Bugs: KAFKA-1647
https://issues.apache.org/jira/browse/KAFKA-1647
Repository: kafka
Description (updated)
-------
rebased for KAFKA-1647
rebased for KAFKA-1647
rebased for KAFKA-1647
Diffs (updated)
-----
core/src/main/scala/kafka/server/ReplicaManager.scala 02fa3821271e97b24fd2ae25163933222797585d
Diff: https://reviews.apache.org/r/26373/diff/
Testing
-------
Followed Joel's testing step. I was able to reproduce the problem without the patch and the WARN message goes away after applied the patch.
Thanks,
Jiangjie Qin
Re: Review Request 26373: Patch for KAFKA-1647
Posted by Jiangjie Qin <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26373/
-----------------------------------------------------------
(Updated Oct. 30, 2014, 10:10 p.m.)
Review request for kafka.
Bugs: KAFKA-1647
https://issues.apache.org/jira/browse/KAFKA-1647
Repository: kafka
Description (updated)
-------
rebased for KAFKA-1647
rebased for KAFKA-1647
Diffs (updated)
-----
core/src/main/scala/kafka/server/ReplicaManager.scala 02fa3821271e97b24fd2ae25163933222797585d
Diff: https://reviews.apache.org/r/26373/diff/
Testing
-------
Followed Joel's testing step. I was able to reproduce the problem without the patch and the WARN message goes away after applied the patch.
Thanks,
Jiangjie Qin
Re: Review Request 26373: Patch for KAFKA-1647
Posted by Jiangjie Qin <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26373/
-----------------------------------------------------------
(Updated Oct. 30, 2014, 10:07 p.m.)
Review request for kafka.
Bugs: KAFKA-1647
https://issues.apache.org/jira/browse/KAFKA-1647
Repository: kafka
Description (updated)
-------
rebased for KAFKA-1647
Diffs (updated)
-----
core/src/main/scala/kafka/server/ReplicaManager.scala 02fa3821271e97b24fd2ae25163933222797585d
Diff: https://reviews.apache.org/r/26373/diff/
Testing
-------
Followed Joel's testing step. I was able to reproduce the problem without the patch and the WARN message goes away after applied the patch.
Thanks,
Jiangjie Qin
Re: Review Request 26373: Patch for KAFKA-1647
Posted by Jiangjie Qin <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26373/
-----------------------------------------------------------
(Updated Oct. 28, 2014, 12:20 a.m.)
Review request for kafka.
Bugs: KAFKA-1647
https://issues.apache.org/jira/browse/KAFKA-1647
Repository: kafka
Description
-------
Addressed Joel's comments.
the version 2 code seems to be submitted by mistake... This should be the code for review that addressed Joel's comments.
Addressed Jun's comments. Will do tests to verify if it works.
Addressed Joel's comments, we do not need to check the if leader exits for not when adding fetcher.
Diffs
-----
core/src/main/scala/kafka/server/ReplicaManager.scala 78b7514cc109547c562e635824684fad581af653
Diff: https://reviews.apache.org/r/26373/diff/
Testing (updated)
-------
Followed Joel's testing step. I was able to reproduce the problem without the patch and the WARN message goes away after applied the patch.
Thanks,
Jiangjie Qin
Re: Review Request 26373: Patch for KAFKA-1647
Posted by Jiangjie Qin <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26373/
-----------------------------------------------------------
(Updated Oct. 28, 2014, 12:19 a.m.)
Review request for kafka.
Bugs: KAFKA-1647
https://issues.apache.org/jira/browse/KAFKA-1647
Repository: kafka
Description (updated)
-------
Addressed Joel's comments.
the version 2 code seems to be submitted by mistake... This should be the code for review that addressed Joel's comments.
Addressed Jun's comments. Will do tests to verify if it works.
Addressed Joel's comments, we do not need to check the if leader exits for not when adding fetcher.
Diffs (updated)
-----
core/src/main/scala/kafka/server/ReplicaManager.scala 78b7514cc109547c562e635824684fad581af653
Diff: https://reviews.apache.org/r/26373/diff/
Testing
-------
Thanks,
Jiangjie Qin
Re: Review Request 26373: Patch for KAFKA-1647
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26373/#review57836
-----------------------------------------------------------
core/src/main/scala/kafka/server/ReplicaManager.scala
<https://reviews.apache.org/r/26373/#comment98711>
This and the following change can be reverted. I will post some steps to reproduce locally on the ticket.
- Joel Koshy
On Oct. 22, 2014, 6:08 a.m., Jiangjie Qin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26373/
> -----------------------------------------------------------
>
> (Updated Oct. 22, 2014, 6:08 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1647
> https://issues.apache.org/jira/browse/KAFKA-1647
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Addressed Joel's comments.
>
>
> the version 2 code seems to be submitted by mistake... This should be the code for review that addressed Joel's comments.
>
>
> Addressed Jun's comments. Will do tests to verify if it works.
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/ReplicaManager.scala 78b7514cc109547c562e635824684fad581af653
>
> Diff: https://reviews.apache.org/r/26373/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jiangjie Qin
>
>
Re: Review Request 26373: Patch for KAFKA-1647
Posted by Jiangjie Qin <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26373/
-----------------------------------------------------------
(Updated Oct. 22, 2014, 6:08 a.m.)
Review request for kafka.
Bugs: KAFKA-1647
https://issues.apache.org/jira/browse/KAFKA-1647
Repository: kafka
Description (updated)
-------
Addressed Joel's comments.
the version 2 code seems to be submitted by mistake... This should be the code for review that addressed Joel's comments.
Addressed Jun's comments. Will do tests to verify if it works.
Diffs (updated)
-----
core/src/main/scala/kafka/server/ReplicaManager.scala 78b7514cc109547c562e635824684fad581af653
Diff: https://reviews.apache.org/r/26373/diff/
Testing
-------
Thanks,
Jiangjie Qin
Re: Review Request 26373: Patch for KAFKA-1647
Posted by Jiangjie Qin <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26373/
-----------------------------------------------------------
(Updated Oct. 18, 2014, 7:26 a.m.)
Review request for kafka.
Bugs: KAFKA-1647
https://issues.apache.org/jira/browse/KAFKA-1647
Repository: kafka
Description (updated)
-------
Addressed Joel's comments.
the version 2 code seems to be submitted by mistake... This should be the code for review that addressed Joel's comments.
Diffs (updated)
-----
core/src/main/scala/kafka/server/ReplicaManager.scala 78b7514cc109547c562e635824684fad581af653
Diff: https://reviews.apache.org/r/26373/diff/
Testing
-------
Thanks,
Jiangjie Qin
Re: Review Request 26373: Patch for KAFKA-1647
Posted by Jiangjie Qin <be...@gmail.com>.
> On Oct. 16, 2014, 8:11 p.m., Neha Narkhede wrote:
> > core/src/main/scala/kafka/server/ReplicaManager.scala, line 504
> > <https://reviews.apache.org/r/26373/diff/2/?file=719844#file719844line504>
> >
> > Now for partitions that have a leader, we are not adding a follower.
Hi Neha, the version 2 code seems to be submitted from a wrong temp branch. I submitted the correct code for review again.
- Jiangjie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26373/#review56989
-----------------------------------------------------------
On Oct. 18, 2014, 7:26 a.m., Jiangjie Qin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26373/
> -----------------------------------------------------------
>
> (Updated Oct. 18, 2014, 7:26 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1647
> https://issues.apache.org/jira/browse/KAFKA-1647
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Addressed Joel's comments.
>
>
> the version 2 code seems to be submitted by mistake... This should be the code for review that addressed Joel's comments.
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/ReplicaManager.scala 78b7514cc109547c562e635824684fad581af653
>
> Diff: https://reviews.apache.org/r/26373/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jiangjie Qin
>
>
Re: Review Request 26373: Patch for KAFKA-1647
Posted by Neha Narkhede <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26373/#review56989
-----------------------------------------------------------
This is a fairly tricky patch and I'm not 100% sure that we haven't introduced any kind of regression. I'd feel more comfortable accepting this patch, if we repeated the kind of testing that was done to find this bug.
core/src/main/scala/kafka/server/ReplicaManager.scala
<https://reviews.apache.org/r/26373/#comment97429>
if(!partition.makeFollower)
core/src/main/scala/kafka/server/ReplicaManager.scala
<https://reviews.apache.org/r/26373/#comment97430>
Now for partitions that have a leader, we are not adding a follower.
- Neha Narkhede
On Oct. 13, 2014, 11:38 p.m., Jiangjie Qin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26373/
> -----------------------------------------------------------
>
> (Updated Oct. 13, 2014, 11:38 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1647
> https://issues.apache.org/jira/browse/KAFKA-1647
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Addressed Joel's comments.
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/ReplicaManager.scala 78b7514cc109547c562e635824684fad581af653
>
> Diff: https://reviews.apache.org/r/26373/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jiangjie Qin
>
>
Re: Review Request 26373: Patch for KAFKA-1647
Posted by Jiangjie Qin <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26373/
-----------------------------------------------------------
(Updated Oct. 13, 2014, 11:38 p.m.)
Review request for kafka.
Bugs: KAFKA-1647
https://issues.apache.org/jira/browse/KAFKA-1647
Repository: kafka
Description (updated)
-------
Addressed Joel's comments.
Diffs (updated)
-----
core/src/main/scala/kafka/server/ReplicaManager.scala 78b7514cc109547c562e635824684fad581af653
Diff: https://reviews.apache.org/r/26373/diff/
Testing
-------
Thanks,
Jiangjie Qin
Re: Review Request 26373: Patch for KAFKA-1647
Posted by Jiangjie Qin <be...@gmail.com>.
> On Oct. 7, 2014, 12:15 a.m., Guozhang Wang wrote:
> > Since now the first iteration of "if" statements is only used for logging, could we just merge it into the second check?
Guozhang, thanks for the review. I actually thought about it before. I agree that the code looks simpler if we just call partition.makeFollower without checking whether leader is up or not. The reason I retained the first if statement is that it seems more reasonable to create the remote replicas only when the leader broker is online, which is done in partition.makeFollower. And I'm not 100% sure whether it matters if we just create the remote replicas objects without checking the liveliness of leader broker.
- Jiangjie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26373/#review55615
-----------------------------------------------------------
On Oct. 6, 2014, 5:06 p.m., Jiangjie Qin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26373/
> -----------------------------------------------------------
>
> (Updated Oct. 6, 2014, 5:06 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1647
> https://issues.apache.org/jira/browse/KAFKA-1647
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Fix for Kafka-1647.
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/ReplicaManager.scala 78b7514cc109547c562e635824684fad581af653
>
> Diff: https://reviews.apache.org/r/26373/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jiangjie Qin
>
>
Re: Review Request 26373: Patch for KAFKA-1647
Posted by Guozhang Wang <gu...@linkedin.com>.
> On Oct. 7, 2014, 12:15 a.m., Guozhang Wang wrote:
> > Since now the first iteration of "if" statements is only used for logging, could we just merge it into the second check?
>
> Jiangjie Qin wrote:
> Guozhang, thanks for the review. I actually thought about it before. I agree that the code looks simpler if we just call partition.makeFollower without checking whether leader is up or not. The reason I retained the first if statement is that it seems more reasonable to create the remote replicas only when the leader broker is online, which is done in partition.makeFollower. And I'm not 100% sure whether it matters if we just create the remote replicas objects without checking the liveliness of leader broker.
>
> Guozhang Wang wrote:
> The checking of the liveness of the leader is just for assertation (i.e. the leader should always be alive when the leaderAndISR request is received, otherwise something bad has happended and we need to log the errors, but still proceed by skiping this request).
Just checked the code again. The reason we delay the process of adding fetchers is just to optimize the case when the broker is already shutting down. On the other hand, makeFollower will only return false if the newly assigned leader is the same as the current leader, and I do not remember why we need to check this. In fact, now I think the make follower logic should proceed even if the newly assigned leader is the same as the current leader.
With that, and the fact that if leaders.find(_.id == newLeaderBrokerId) passed then leaders.find(_.id == partition.leaderReplicaIdOpt.get) would also pass, I think what we can do the following process:
1. partition.makeFollowers, and only log errors if it returns false (i.e. do not exclude the partition)
2. remove fetcher, truncate log, etc.
3. if broker is not shutting down AND the new leader is up, add fetchers.
- Guozhang
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26373/#review55615
-----------------------------------------------------------
On Oct. 6, 2014, 5:06 p.m., Jiangjie Qin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26373/
> -----------------------------------------------------------
>
> (Updated Oct. 6, 2014, 5:06 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1647
> https://issues.apache.org/jira/browse/KAFKA-1647
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Fix for Kafka-1647.
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/ReplicaManager.scala 78b7514cc109547c562e635824684fad581af653
>
> Diff: https://reviews.apache.org/r/26373/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jiangjie Qin
>
>
Re: Review Request 26373: Patch for KAFKA-1647
Posted by Guozhang Wang <gu...@linkedin.com>.
> On Oct. 7, 2014, 12:15 a.m., Guozhang Wang wrote:
> > Since now the first iteration of "if" statements is only used for logging, could we just merge it into the second check?
>
> Jiangjie Qin wrote:
> Guozhang, thanks for the review. I actually thought about it before. I agree that the code looks simpler if we just call partition.makeFollower without checking whether leader is up or not. The reason I retained the first if statement is that it seems more reasonable to create the remote replicas only when the leader broker is online, which is done in partition.makeFollower. And I'm not 100% sure whether it matters if we just create the remote replicas objects without checking the liveliness of leader broker.
The checking of the liveness of the leader is just for assertation (i.e. the leader should always be alive when the leaderAndISR request is received, otherwise something bad has happended and we need to log the errors, but still proceed by skiping this request).
- Guozhang
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26373/#review55615
-----------------------------------------------------------
On Oct. 6, 2014, 5:06 p.m., Jiangjie Qin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26373/
> -----------------------------------------------------------
>
> (Updated Oct. 6, 2014, 5:06 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1647
> https://issues.apache.org/jira/browse/KAFKA-1647
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Fix for Kafka-1647.
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/ReplicaManager.scala 78b7514cc109547c562e635824684fad581af653
>
> Diff: https://reviews.apache.org/r/26373/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jiangjie Qin
>
>
Re: Review Request 26373: Patch for KAFKA-1647
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26373/#review55615
-----------------------------------------------------------
Since now the first iteration of "if" statements is only used for logging, could we just merge it into the second check?
- Guozhang Wang
On Oct. 6, 2014, 5:06 p.m., Jiangjie Qin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26373/
> -----------------------------------------------------------
>
> (Updated Oct. 6, 2014, 5:06 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1647
> https://issues.apache.org/jira/browse/KAFKA-1647
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Fix for Kafka-1647.
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/ReplicaManager.scala 78b7514cc109547c562e635824684fad581af653
>
> Diff: https://reviews.apache.org/r/26373/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jiangjie Qin
>
>