Re: Review Request 26373: Patch for KAFKA-1647
--- 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
--- 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
--- 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
--- 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
--- 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/#comment13 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
--- 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
--- 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
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
--- 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
--- 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
--- 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
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
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
--- 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
--- 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
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
--- 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
--- 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
--- 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
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
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
--- 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
