You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@kafka.apache.org by Neha Narkhede <ne...@gmail.com> on 2013/10/23 06:34:24 UTC

Review Request 14865: Patch for KAFKA-1097

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

Review request for kafka.


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


Repository: kafka


Description
-------

KAFKA-1097 Race condition while reassigning low throughput partition leads to incorrect ISR information in zookeeper; The changes include 1) Adding the ISR shrink logic as part of the OfflineReplica -> NonExistentReplica state change 2) Adding a safety check on the broker where it only expands the ISR if the replica is in the assigned replica list 3) Updating the assigned replica list on the broker on every makeLeader request and also on makeFollower request for safety, though that's not strictly required. These changes will ensure that the ISR is shrunk by the controller and the leader has an updated assigned replica list. So even if a replica sends a fetch request after the ISR is shrunk by the controller, the broker will not be able to update the ISR until it receives the next LeaderAndIsrRequest (which notifies it of the latest zkVersion of the partition state path) that also contains the shrunk ISR and assigned replica list. Using that the broker will avoid expanding the ISR if 
 the replica is not present in the new assigned replica list


Diffs
-----

  core/src/main/scala/kafka/cluster/Partition.scala 60f3ed4e88b08d0dcf2ea84259ae6dbe9d7c3a2d 
  core/src/main/scala/kafka/controller/KafkaController.scala 88d130f55997b72a8590e4cfe92857a7320e70d5 
  core/src/main/scala/kafka/controller/ReplicaStateMachine.scala 212c05d65dcdc147e55f90875bacc940e30342bf 
  core/src/main/scala/kafka/server/ReplicaManager.scala 03ba60e82cdb3dce100603d615894ede47e4b077 
  kafka-patch-review.py 82ea9a890fe79aad7d0ea6d33f3e2780e036317c 

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


Testing
-------


Thanks,

Neha Narkhede

Re: Review Request 14865: Patch for KAFKA-1097

Posted by Neha Narkhede <ne...@gmail.com>.


> On Oct. 24, 2013, 12:30 a.m., Guozhang Wang wrote:
> > kafka-patch-review.py, line 100
> > <https://reviews.apache.org/r/14865/diff/1/?file=369530#file369530line100>
> >
> >     Should this be included in this RB?

Actually I wanted to improve the tool to print the branch against which the rb is created. But I need to revise the patch, this particular diff to kafka-patch-review.py can be ignored


> On Oct. 24, 2013, 12:30 a.m., Guozhang Wang wrote:
> > core/src/main/scala/kafka/cluster/Partition.scala, line 211
> > <https://reviews.apache.org/r/14865/diff/1/?file=369526#file369526line211>
> >
> >     Previously we do not update assignedReplicas map upon receiving the LeaderAndISR request, was that OK?

Yes, since it only changed once when the partition was created and never changed after that. The exception is partition reassignment, increasing replication factor and deleting topic. So we need to fix it


> On Oct. 24, 2013, 12:30 a.m., Guozhang Wang wrote:
> > core/src/main/scala/kafka/controller/ReplicaStateMachine.scala, line 137
> > <https://reviews.apache.org/r/14865/diff/1/?file=369528#file369528line137>
> >
> >     The changes in logic of expanding ISR should have already solved the problem. Do we need to do this here?

Yes, we need to let the leader know of the shrunk assigned replica list. So we need the controller to write the ISR and then send the shrunk assigned replica list with the shrunk ISR to the leader.


> On Oct. 24, 2013, 12:30 a.m., Guozhang Wang wrote:
> > core/src/main/scala/kafka/controller/ReplicaStateMachine.scala, line 181
> > <https://reviews.apache.org/r/14865/diff/1/?file=369528#file369528line181>
> >
> >     Ditto as above.

This is required for correctness. Since the controller is actively trying to let the replica to stop, it has to send the stop replica request. It also reduces the probability of the replica unnecessarily trying to fetch from the leader and re-enter ISR


- Neha


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


On Oct. 23, 2013, 4:34 a.m., Neha Narkhede wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14865/
> -----------------------------------------------------------
> 
> (Updated Oct. 23, 2013, 4:34 a.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1097
>     https://issues.apache.org/jira/browse/KAFKA-1097
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1097 Race condition while reassigning low throughput partition leads to incorrect ISR information in zookeeper; The changes include 1) Adding the ISR shrink logic as part of the OfflineReplica -> NonExistentReplica state change 2) Adding a safety check on the broker where it only expands the ISR if the replica is in the assigned replica list 3) Updating the assigned replica list on the broker on every makeLeader request and also on makeFollower request for safety, though that's not strictly required. These changes will ensure that the ISR is shrunk by the controller and the leader has an updated assigned replica list. So even if a replica sends a fetch request after the ISR is shrunk by the controller, the broker will not be able to update the ISR until it receives the next LeaderAndIsrRequest (which notifies it of the latest zkVersion of the partition state path) that also contains the shrunk ISR and assigned replica list. Using that the broker will avoid expanding the ISR i
 f the replica is not present in the new assigned replica list
> 
> 
> Diffs
> -----
> 
>   core/src/main/scala/kafka/cluster/Partition.scala 60f3ed4e88b08d0dcf2ea84259ae6dbe9d7c3a2d 
>   core/src/main/scala/kafka/controller/KafkaController.scala 88d130f55997b72a8590e4cfe92857a7320e70d5 
>   core/src/main/scala/kafka/controller/ReplicaStateMachine.scala 212c05d65dcdc147e55f90875bacc940e30342bf 
>   core/src/main/scala/kafka/server/ReplicaManager.scala 03ba60e82cdb3dce100603d615894ede47e4b077 
>   kafka-patch-review.py 82ea9a890fe79aad7d0ea6d33f3e2780e036317c 
> 
> Diff: https://reviews.apache.org/r/14865/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Neha Narkhede
> 
>

Re: Review Request 14865: Patch for KAFKA-1097

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

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



core/src/main/scala/kafka/cluster/Partition.scala
<https://reviews.apache.org/r/14865/#comment53312>

    Previously we do not update assignedReplicas map upon receiving the LeaderAndISR request, was that OK?



core/src/main/scala/kafka/controller/ReplicaStateMachine.scala
<https://reviews.apache.org/r/14865/#comment53313>

    The changes in logic of expanding ISR should have already solved the problem. Do we need to do this here?



core/src/main/scala/kafka/controller/ReplicaStateMachine.scala
<https://reviews.apache.org/r/14865/#comment53314>

    Ditto as above.



kafka-patch-review.py
<https://reviews.apache.org/r/14865/#comment53311>

    Should this be included in this RB?


- Guozhang Wang


On Oct. 23, 2013, 4:34 a.m., Neha Narkhede wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14865/
> -----------------------------------------------------------
> 
> (Updated Oct. 23, 2013, 4:34 a.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1097
>     https://issues.apache.org/jira/browse/KAFKA-1097
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1097 Race condition while reassigning low throughput partition leads to incorrect ISR information in zookeeper; The changes include 1) Adding the ISR shrink logic as part of the OfflineReplica -> NonExistentReplica state change 2) Adding a safety check on the broker where it only expands the ISR if the replica is in the assigned replica list 3) Updating the assigned replica list on the broker on every makeLeader request and also on makeFollower request for safety, though that's not strictly required. These changes will ensure that the ISR is shrunk by the controller and the leader has an updated assigned replica list. So even if a replica sends a fetch request after the ISR is shrunk by the controller, the broker will not be able to update the ISR until it receives the next LeaderAndIsrRequest (which notifies it of the latest zkVersion of the partition state path) that also contains the shrunk ISR and assigned replica list. Using that the broker will avoid expanding the ISR i
 f the replica is not present in the new assigned replica list
> 
> 
> Diffs
> -----
> 
>   core/src/main/scala/kafka/cluster/Partition.scala 60f3ed4e88b08d0dcf2ea84259ae6dbe9d7c3a2d 
>   core/src/main/scala/kafka/controller/KafkaController.scala 88d130f55997b72a8590e4cfe92857a7320e70d5 
>   core/src/main/scala/kafka/controller/ReplicaStateMachine.scala 212c05d65dcdc147e55f90875bacc940e30342bf 
>   core/src/main/scala/kafka/server/ReplicaManager.scala 03ba60e82cdb3dce100603d615894ede47e4b077 
>   kafka-patch-review.py 82ea9a890fe79aad7d0ea6d33f3e2780e036317c 
> 
> Diff: https://reviews.apache.org/r/14865/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Neha Narkhede
> 
>

Re: Review Request 14865: Patch for KAFKA-1097

Posted by Neha Narkhede <ne...@gmail.com>.


> On Oct. 24, 2013, 4:30 p.m., Jun Rao wrote:
> > Thanks for the patch. There a couple of issues.
> > 
> > 1. The main one is during the phase of partition reassignment when we bootstrap new replicas. At this point, the assigned replica list doesn't include the new replicas. If we only allow replicas in assigned replica set to be added to ISR, those new replicas won't be added to ISR, which will prevent partition reassignment from completing. We could include those new replicas in the all replica set in the LeaderAndIsr request. We probably have to think a bit more to see if there is any other impact.
> > 
> > 2. Once the assigned replica set in the broker is updated. We need to prevent an old replica from being added back to this set again. Currently, in Partition.updateLeaderHWAndMaybeExpandIsr() (triggered by a fetch request), it will call getOrCreateReplica(), which can cause a replica to be added back to the assigned replica set. What we can do is to only call getOrCreateReplica during makeLeader() and makeFollower(). In the former, we force all replicas to be created. In the latter, we just need to make sure the local replica is created. In Partition.updateLeaderHWAndMaybeExpandIsr(), we can then use getReplica(), instead of getOrCreateReplica().

Those are great points 

1. We can't really include the new replicas in the assigned replicas and send it to the leader. The reason is that the request would have the same leaderEpoch and the broker will ignore the leader and isr request. Hacking around this would require the controller to write the state path with a higher leader epoch (even if there is no real change to the partition's state) and then send the LeaderAndIsrRequest with a higher leaderEpoch
2. This is a good suggestion. Partition.updateLeaderHWAndMaybeExpandIsr() should really be using getReplica(). 


- Neha


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


On Oct. 23, 2013, 4:34 a.m., Neha Narkhede wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14865/
> -----------------------------------------------------------
> 
> (Updated Oct. 23, 2013, 4:34 a.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1097
>     https://issues.apache.org/jira/browse/KAFKA-1097
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1097 Race condition while reassigning low throughput partition leads to incorrect ISR information in zookeeper; The changes include 1) Adding the ISR shrink logic as part of the OfflineReplica -> NonExistentReplica state change 2) Adding a safety check on the broker where it only expands the ISR if the replica is in the assigned replica list 3) Updating the assigned replica list on the broker on every makeLeader request and also on makeFollower request for safety, though that's not strictly required. These changes will ensure that the ISR is shrunk by the controller and the leader has an updated assigned replica list. So even if a replica sends a fetch request after the ISR is shrunk by the controller, the broker will not be able to update the ISR until it receives the next LeaderAndIsrRequest (which notifies it of the latest zkVersion of the partition state path) that also contains the shrunk ISR and assigned replica list. Using that the broker will avoid expanding the ISR i
 f the replica is not present in the new assigned replica list
> 
> 
> Diffs
> -----
> 
>   core/src/main/scala/kafka/cluster/Partition.scala 60f3ed4e88b08d0dcf2ea84259ae6dbe9d7c3a2d 
>   core/src/main/scala/kafka/controller/KafkaController.scala 88d130f55997b72a8590e4cfe92857a7320e70d5 
>   core/src/main/scala/kafka/controller/ReplicaStateMachine.scala 212c05d65dcdc147e55f90875bacc940e30342bf 
>   core/src/main/scala/kafka/server/ReplicaManager.scala 03ba60e82cdb3dce100603d615894ede47e4b077 
>   kafka-patch-review.py 82ea9a890fe79aad7d0ea6d33f3e2780e036317c 
> 
> Diff: https://reviews.apache.org/r/14865/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Neha Narkhede
> 
>

Re: Review Request 14865: Patch for KAFKA-1097

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

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

Thanks for the patch. There a couple of issues.

1. The main one is during the phase of partition reassignment when we bootstrap new replicas. At this point, the assigned replica list doesn't include the new replicas. If we only allow replicas in assigned replica set to be added to ISR, those new replicas won't be added to ISR, which will prevent partition reassignment from completing. We could include those new replicas in the all replica set in the LeaderAndIsr request. We probably have to think a bit more to see if there is any other impact.

2. Once the assigned replica set in the broker is updated. We need to prevent an old replica from being added back to this set again. Currently, in Partition.updateLeaderHWAndMaybeExpandIsr() (triggered by a fetch request), it will call getOrCreateReplica(), which can cause a replica to be added back to the assigned replica set. What we can do is to only call getOrCreateReplica during makeLeader() and makeFollower(). In the former, we force all replicas to be created. In the latter, we just need to make sure the local replica is created. In Partition.updateLeaderHWAndMaybeExpandIsr(), we can then use getReplica(), instead of getOrCreateReplica().

- Jun Rao

On Oct. 23, 2013, 4:34 a.m., Neha Narkhede wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14865/
> -----------------------------------------------------------
> 
> (Updated Oct. 23, 2013, 4:34 a.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1097
>     https://issues.apache.org/jira/browse/KAFKA-1097
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1097 Race condition while reassigning low throughput partition leads to incorrect ISR information in zookeeper; The changes include 1) Adding the ISR shrink logic as part of the OfflineReplica -> NonExistentReplica state change 2) Adding a safety check on the broker where it only expands the ISR if the replica is in the assigned replica list 3) Updating the assigned replica list on the broker on every makeLeader request and also on makeFollower request for safety, though that's not strictly required. These changes will ensure that the ISR is shrunk by the controller and the leader has an updated assigned replica list. So even if a replica sends a fetch request after the ISR is shrunk by the controller, the broker will not be able to update the ISR until it receives the next LeaderAndIsrRequest (which notifies it of the latest zkVersion of the partition state path) that also contains the shrunk ISR and assigned replica list. Using that the broker will avoid expanding the ISR i
 f the replica is not present in the new assigned replica list
> 
> 
> Diffs
> -----
> 
>   core/src/main/scala/kafka/cluster/Partition.scala 60f3ed4e88b08d0dcf2ea84259ae6dbe9d7c3a2d 
>   core/src/main/scala/kafka/controller/KafkaController.scala 88d130f55997b72a8590e4cfe92857a7320e70d5 
>   core/src/main/scala/kafka/controller/ReplicaStateMachine.scala 212c05d65dcdc147e55f90875bacc940e30342bf 
>   core/src/main/scala/kafka/server/ReplicaManager.scala 03ba60e82cdb3dce100603d615894ede47e4b077 
>   kafka-patch-review.py 82ea9a890fe79aad7d0ea6d33f3e2780e036317c 
> 
> Diff: https://reviews.apache.org/r/14865/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Neha Narkhede
> 
>

Re: Review Request 14865: Patch for KAFKA-1097

Posted by Neha Narkhede <ne...@gmail.com>.


> On Nov. 1, 2013, 4:33 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/controller/KafkaController.scala, lines 383-387
> > <https://reviews.apache.org/r/14865/diff/4/?file=375158#file375158line383>
> >
> >     In moveReassignedPartitionLeaderIfRequired(), we call partitionStateMachine.handleStateChanges(), which will use the old assigned replicas. It seems that we need to call updateAssignedReplicasForPartition() before this step so that we can pick up the new assigned replicas.

>> In moveReassignedPartitionLeaderIfRequired(), we call partitionStateMachine.handleStateChanges(), which will use the old assigned replicas.

It doesn't. See this

    // change the assigned replica list to just the reassigned replicas in the cache so it gets sent out on the LeaderAndIsr
    // request to the current or new leader. This will prevent it from adding the old replicas to the ISR
    val oldAndNewReplicas = controllerContext.partitionReplicaAssignment(topicAndPartition)
    controllerContext.partitionReplicaAssignment.put(topicAndPartition, reassignedReplicas)

We do this before the partitionStateMachine.handleStateChanges() call and it correctly sends the shrunk assigned replicas.


> On Nov. 1, 2013, 4:33 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/controller/KafkaController.scala, lines 387-388
> > <https://reviews.apache.org/r/14865/diff/4/?file=375158#file375158line387>
> >
> >     We can use reassignedReplicas for reassignedPartitionContext.newReplicas.

Though both are same, changed it


> On Nov. 1, 2013, 4:33 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/controller/KafkaController.scala, lines 631-632
> > <https://reviews.apache.org/r/14865/diff/4/?file=375158#file375158line631>
> >
> >     It probably doesn't hurt to send the leaderAndIsr request to both the old and the new replicas. However, it seems that we really just need to send to the new replicas since in the caller onPartitionReassignment(), we will be stopping the old replicas immediately afterwards.

Agree, but I'm afraid I may be missing some corner cases in which case sending more data will be useful compared to sending less data. 


- Neha


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


On Oct. 31, 2013, 5:37 p.m., Neha Narkhede wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14865/
> -----------------------------------------------------------
> 
> (Updated Oct. 31, 2013, 5:37 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1097
>     https://issues.apache.org/jira/browse/KAFKA-1097
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> Addressed Guozhang's comment to reduce the data logged to state change log
> 
> 
> Addressed Jun and Guozhang's review comments
> 
> 
> Rebased after KAFKA-1001 was checked in
> 
> 
> Make a reusable updateLeaderEpoch API
> 
> 
> 1. Reverted changes to test log4j.properties and AdminTest 2. Fixed a bug in moving leader logic during partition reassignment:
> 
> 
> 1. Changed partition reassignment behavior to first expand the replication factor and then shrink it. 2. Fixed the reassign partition tool to check for error conditions and improve logging 3. Did a minor improvement to the patch review tool 4. Changed the version to 0.8.1 since it should be that on trunk
> 
> 
> Revert "kafka-1097, unit tests broken due to kafka-1098"
> 
> This reverts commit 0da98f7e3b5512bdc876438704c6a895e997daee.
> 
> kafka-1097, unit tests broken due to kafka-1098
> 
> 
> Diffs
> -----
> 
>   core/src/main/scala/kafka/admin/ReassignPartitionsCommand.scala 2f706c94d340f1cd715210b3d8b00597d65b5dd0 
>   core/src/main/scala/kafka/cluster/Partition.scala d8078bd4e73a8a80be436396765ead4c62d334bd 
>   core/src/main/scala/kafka/common/NotAssignedReplicaException.scala PRE-CREATION 
>   core/src/main/scala/kafka/controller/KafkaController.scala 88d130f55997b72a8590e4cfe92857a7320e70d5 
>   core/src/main/scala/kafka/controller/ReplicaStateMachine.scala 212c05d65dcdc147e55f90875bacc940e30342bf 
>   core/src/main/scala/kafka/server/ReplicaManager.scala 7b8f89edecec446664fb4abe64f5dfafb9aa3120 
>   kafka-patch-review.py daf2c3597a81d0d6db574a488f3cf372701fd113 
>   project/Build.scala bcd1ca546b3f081d669892677276a0f19fb1236e 
> 
> Diff: https://reviews.apache.org/r/14865/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Neha Narkhede
> 
>

Re: Review Request 14865: Patch for KAFKA-1097

Posted by Neha Narkhede <ne...@gmail.com>.


> On Nov. 1, 2013, 4:33 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/controller/KafkaController.scala, lines 397-398
> > <https://reviews.apache.org/r/14865/diff/4/?file=375158#file375158line397>
> >
> >     newReplicas really means the extra replicas in the reassigned replicas. So, we need to named it properly.

Changed to newReplicasNotInOldReplicaList


- Neha


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


On Oct. 31, 2013, 5:37 p.m., Neha Narkhede wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14865/
> -----------------------------------------------------------
> 
> (Updated Oct. 31, 2013, 5:37 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1097
>     https://issues.apache.org/jira/browse/KAFKA-1097
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> Addressed Guozhang's comment to reduce the data logged to state change log
> 
> 
> Addressed Jun and Guozhang's review comments
> 
> 
> Rebased after KAFKA-1001 was checked in
> 
> 
> Make a reusable updateLeaderEpoch API
> 
> 
> 1. Reverted changes to test log4j.properties and AdminTest 2. Fixed a bug in moving leader logic during partition reassignment:
> 
> 
> 1. Changed partition reassignment behavior to first expand the replication factor and then shrink it. 2. Fixed the reassign partition tool to check for error conditions and improve logging 3. Did a minor improvement to the patch review tool 4. Changed the version to 0.8.1 since it should be that on trunk
> 
> 
> Revert "kafka-1097, unit tests broken due to kafka-1098"
> 
> This reverts commit 0da98f7e3b5512bdc876438704c6a895e997daee.
> 
> kafka-1097, unit tests broken due to kafka-1098
> 
> 
> Diffs
> -----
> 
>   core/src/main/scala/kafka/admin/ReassignPartitionsCommand.scala 2f706c94d340f1cd715210b3d8b00597d65b5dd0 
>   core/src/main/scala/kafka/cluster/Partition.scala d8078bd4e73a8a80be436396765ead4c62d334bd 
>   core/src/main/scala/kafka/common/NotAssignedReplicaException.scala PRE-CREATION 
>   core/src/main/scala/kafka/controller/KafkaController.scala 88d130f55997b72a8590e4cfe92857a7320e70d5 
>   core/src/main/scala/kafka/controller/ReplicaStateMachine.scala 212c05d65dcdc147e55f90875bacc940e30342bf 
>   core/src/main/scala/kafka/server/ReplicaManager.scala 7b8f89edecec446664fb4abe64f5dfafb9aa3120 
>   kafka-patch-review.py daf2c3597a81d0d6db574a488f3cf372701fd113 
>   project/Build.scala bcd1ca546b3f081d669892677276a0f19fb1236e 
> 
> Diff: https://reviews.apache.org/r/14865/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Neha Narkhede
> 
>

Re: Review Request 14865: Patch for KAFKA-1097

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

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



core/src/main/scala/kafka/controller/KafkaController.scala
<https://reviews.apache.org/r/14865/#comment54472>

    In moveReassignedPartitionLeaderIfRequired(), we call partitionStateMachine.handleStateChanges(), which will use the old assigned replicas. It seems that we need to call updateAssignedReplicasForPartition() before this step so that we can pick up the new assigned replicas.



core/src/main/scala/kafka/controller/KafkaController.scala
<https://reviews.apache.org/r/14865/#comment54468>

    We can use reassignedReplicas for reassignedPartitionContext.newReplicas.



core/src/main/scala/kafka/controller/KafkaController.scala
<https://reviews.apache.org/r/14865/#comment54471>

    



core/src/main/scala/kafka/controller/KafkaController.scala
<https://reviews.apache.org/r/14865/#comment54469>

    newReplicas really means the extra replicas in the reassigned replicas. So, we need to named it properly.



core/src/main/scala/kafka/controller/KafkaController.scala
<https://reviews.apache.org/r/14865/#comment54473>

    It probably doesn't hurt to send the leaderAndIsr request to both the old and the new replicas. However, it seems that we really just need to send to the new replicas since in the caller onPartitionReassignment(), we will be stopping the old replicas immediately afterwards.


- Jun Rao


On Oct. 31, 2013, 5:37 p.m., Neha Narkhede wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14865/
> -----------------------------------------------------------
> 
> (Updated Oct. 31, 2013, 5:37 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1097
>     https://issues.apache.org/jira/browse/KAFKA-1097
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> Addressed Guozhang's comment to reduce the data logged to state change log
> 
> 
> Addressed Jun and Guozhang's review comments
> 
> 
> Rebased after KAFKA-1001 was checked in
> 
> 
> Make a reusable updateLeaderEpoch API
> 
> 
> 1. Reverted changes to test log4j.properties and AdminTest 2. Fixed a bug in moving leader logic during partition reassignment:
> 
> 
> 1. Changed partition reassignment behavior to first expand the replication factor and then shrink it. 2. Fixed the reassign partition tool to check for error conditions and improve logging 3. Did a minor improvement to the patch review tool 4. Changed the version to 0.8.1 since it should be that on trunk
> 
> 
> Revert "kafka-1097, unit tests broken due to kafka-1098"
> 
> This reverts commit 0da98f7e3b5512bdc876438704c6a895e997daee.
> 
> kafka-1097, unit tests broken due to kafka-1098
> 
> 
> Diffs
> -----
> 
>   core/src/main/scala/kafka/admin/ReassignPartitionsCommand.scala 2f706c94d340f1cd715210b3d8b00597d65b5dd0 
>   core/src/main/scala/kafka/cluster/Partition.scala d8078bd4e73a8a80be436396765ead4c62d334bd 
>   core/src/main/scala/kafka/common/NotAssignedReplicaException.scala PRE-CREATION 
>   core/src/main/scala/kafka/controller/KafkaController.scala 88d130f55997b72a8590e4cfe92857a7320e70d5 
>   core/src/main/scala/kafka/controller/ReplicaStateMachine.scala 212c05d65dcdc147e55f90875bacc940e30342bf 
>   core/src/main/scala/kafka/server/ReplicaManager.scala 7b8f89edecec446664fb4abe64f5dfafb9aa3120 
>   kafka-patch-review.py daf2c3597a81d0d6db574a488f3cf372701fd113 
>   project/Build.scala bcd1ca546b3f081d669892677276a0f19fb1236e 
> 
> Diff: https://reviews.apache.org/r/14865/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Neha Narkhede
> 
>

Re: Review Request 14865: Patch for KAFKA-1097

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

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

Ship it!


Ship It!

- Guozhang Wang


On Nov. 1, 2013, 4:55 p.m., Neha Narkhede wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14865/
> -----------------------------------------------------------
> 
> (Updated Nov. 1, 2013, 4:55 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1097
>     https://issues.apache.org/jira/browse/KAFKA-1097
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> Addressed Jun's follow up review comments
> 
> 
> Addressed Guozhang's comment to reduce the data logged to state change log
> 
> 
> Addressed Jun and Guozhang's review comments
> 
> 
> Rebased after KAFKA-1001 was checked in
> 
> 
> Make a reusable updateLeaderEpoch API
> 
> 
> 1. Reverted changes to test log4j.properties and AdminTest 2. Fixed a bug in moving leader logic during partition reassignment:
> 
> 
> 1. Changed partition reassignment behavior to first expand the replication factor and then shrink it. 2. Fixed the reassign partition tool to check for error conditions and improve logging 3. Did a minor improvement to the patch review tool 4. Changed the version to 0.8.1 since it should be that on trunk
> 
> 
> Revert "kafka-1097, unit tests broken due to kafka-1098"
> 
> This reverts commit 0da98f7e3b5512bdc876438704c6a895e997daee.
> 
> kafka-1097, unit tests broken due to kafka-1098
> 
> 
> Diffs
> -----
> 
>   core/src/main/scala/kafka/admin/ReassignPartitionsCommand.scala 2f706c94d340f1cd715210b3d8b00597d65b5dd0 
>   core/src/main/scala/kafka/cluster/Partition.scala d8078bd4e73a8a80be436396765ead4c62d334bd 
>   core/src/main/scala/kafka/common/NotAssignedReplicaException.scala PRE-CREATION 
>   core/src/main/scala/kafka/controller/KafkaController.scala 88d130f55997b72a8590e4cfe92857a7320e70d5 
>   core/src/main/scala/kafka/controller/ReplicaStateMachine.scala 212c05d65dcdc147e55f90875bacc940e30342bf 
>   core/src/main/scala/kafka/server/ReplicaManager.scala 7b8f89edecec446664fb4abe64f5dfafb9aa3120 
>   kafka-patch-review.py daf2c3597a81d0d6db574a488f3cf372701fd113 
>   project/Build.scala bcd1ca546b3f081d669892677276a0f19fb1236e 
> 
> Diff: https://reviews.apache.org/r/14865/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Neha Narkhede
> 
>

Re: Review Request 14865: Patch for KAFKA-1097

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

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

Ship it!


- Jun Rao


On Nov. 1, 2013, 4:55 p.m., Neha Narkhede wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14865/
> -----------------------------------------------------------
> 
> (Updated Nov. 1, 2013, 4:55 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1097
>     https://issues.apache.org/jira/browse/KAFKA-1097
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> Addressed Jun's follow up review comments
> 
> 
> Addressed Guozhang's comment to reduce the data logged to state change log
> 
> 
> Addressed Jun and Guozhang's review comments
> 
> 
> Rebased after KAFKA-1001 was checked in
> 
> 
> Make a reusable updateLeaderEpoch API
> 
> 
> 1. Reverted changes to test log4j.properties and AdminTest 2. Fixed a bug in moving leader logic during partition reassignment:
> 
> 
> 1. Changed partition reassignment behavior to first expand the replication factor and then shrink it. 2. Fixed the reassign partition tool to check for error conditions and improve logging 3. Did a minor improvement to the patch review tool 4. Changed the version to 0.8.1 since it should be that on trunk
> 
> 
> Revert "kafka-1097, unit tests broken due to kafka-1098"
> 
> This reverts commit 0da98f7e3b5512bdc876438704c6a895e997daee.
> 
> kafka-1097, unit tests broken due to kafka-1098
> 
> 
> Diffs
> -----
> 
>   core/src/main/scala/kafka/admin/ReassignPartitionsCommand.scala 2f706c94d340f1cd715210b3d8b00597d65b5dd0 
>   core/src/main/scala/kafka/cluster/Partition.scala d8078bd4e73a8a80be436396765ead4c62d334bd 
>   core/src/main/scala/kafka/common/NotAssignedReplicaException.scala PRE-CREATION 
>   core/src/main/scala/kafka/controller/KafkaController.scala 88d130f55997b72a8590e4cfe92857a7320e70d5 
>   core/src/main/scala/kafka/controller/ReplicaStateMachine.scala 212c05d65dcdc147e55f90875bacc940e30342bf 
>   core/src/main/scala/kafka/server/ReplicaManager.scala 7b8f89edecec446664fb4abe64f5dfafb9aa3120 
>   kafka-patch-review.py daf2c3597a81d0d6db574a488f3cf372701fd113 
>   project/Build.scala bcd1ca546b3f081d669892677276a0f19fb1236e 
> 
> Diff: https://reviews.apache.org/r/14865/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Neha Narkhede
> 
>

Re: Review Request 14865: Patch for KAFKA-1097

Posted by Neha Narkhede <ne...@gmail.com>.

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

(Updated Nov. 1, 2013, 4:55 p.m.)

Review request for kafka.

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

Repository: kafka

Description (updated)
-------

Addressed Jun's follow up review comments

Addressed Guozhang's comment to reduce the data logged to state change log

Addressed Jun and Guozhang's review comments

Rebased after KAFKA-1001 was checked in

Make a reusable updateLeaderEpoch API

1. Reverted changes to test log4j.properties and AdminTest 2. Fixed a bug in moving leader logic during partition reassignment:

1. Changed partition reassignment behavior to first expand the replication factor and then shrink it. 2. Fixed the reassign partition tool to check for error conditions and improve logging 3. Did a minor improvement to the patch review tool 4. Changed the version to 0.8.1 since it should be that on trunk

Revert "kafka-1097, unit tests broken due to kafka-1098"

This reverts commit 0da98f7e3b5512bdc876438704c6a895e997daee.

kafka-1097, unit tests broken due to kafka-1098

Diffs (updated)
-----

core/src/main/scala/kafka/admin/ReassignPartitionsCommand.scala 2f706c94d340f1cd715210b3d8b00597d65b5dd0
core/src/main/scala/kafka/cluster/Partition.scala d8078bd4e73a8a80be436396765ead4c62d334bd
core/src/main/scala/kafka/common/NotAssignedReplicaException.scala PRE-CREATION
core/src/main/scala/kafka/controller/KafkaController.scala 88d130f55997b72a8590e4cfe92857a7320e70d5
core/src/main/scala/kafka/controller/ReplicaStateMachine.scala 212c05d65dcdc147e55f90875bacc940e30342bf
core/src/main/scala/kafka/server/ReplicaManager.scala 7b8f89edecec446664fb4abe64f5dfafb9aa3120
kafka-patch-review.py daf2c3597a81d0d6db574a488f3cf372701fd113
project/Build.scala bcd1ca546b3f081d669892677276a0f19fb1236e

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

Testing
-------

Thanks,

Neha Narkhede

Re: Review Request 14865: Patch for KAFKA-1097

Posted by Neha Narkhede <ne...@gmail.com>.

(Updated Oct. 31, 2013, 5:37 p.m.)

Review request for kafka.

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

Repository: kafka

Description (updated)
-------

Addressed Guozhang's comment to reduce the data logged to state change log

Addressed Jun and Guozhang's review comments

Rebased after KAFKA-1001 was checked in

Make a reusable updateLeaderEpoch API

1. Reverted changes to test log4j.properties and AdminTest 2. Fixed a bug in moving leader logic during partition reassignment:

Revert "kafka-1097, unit tests broken due to kafka-1098"

This reverts commit 0da98f7e3b5512bdc876438704c6a895e997daee.

kafka-1097, unit tests broken due to kafka-1098

Diffs (updated)
-----

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

Testing
-------

Thanks,

Neha Narkhede

Re: Review Request 14865: Patch for KAFKA-1097

Posted by Neha Narkhede <ne...@gmail.com>.


> On Oct. 31, 2013, 3:44 p.m., Guozhang Wang wrote:
> > core/src/main/scala/kafka/server/ReplicaManager.scala, line 310
> > <https://reviews.apache.org/r/14865/diff/3/?file=374593#file374593line310>
> >
> >     One thing worth mention here is that previously we are printing TopicAndPartition.toString and now we are printing Partition.toString, which will be more verbose. This is good for debugging but may flood state-change-log.

Actually, good catch. I used auto refactor in IntelliJ and didn't look closely at the log statements. Fixed this. 


- Neha


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


On Oct. 31, 2013, 4:46 a.m., Neha Narkhede wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14865/
> -----------------------------------------------------------
> 
> (Updated Oct. 31, 2013, 4:46 a.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1097
>     https://issues.apache.org/jira/browse/KAFKA-1097
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> Addressed Jun and Guozhang's review comments
> 
> 
> Rebased after KAFKA-1001 was checked in
> 
> 
> Make a reusable updateLeaderEpoch API
> 
> 
> 1. Reverted changes to test log4j.properties and AdminTest 2. Fixed a bug in moving leader logic during partition reassignment:
> 
> 
> 1. Changed partition reassignment behavior to first expand the replication factor and then shrink it. 2. Fixed the reassign partition tool to check for error conditions and improve logging 3. Did a minor improvement to the patch review tool 4. Changed the version to 0.8.1 since it should be that on trunk
> 
> 
> Revert "kafka-1097, unit tests broken due to kafka-1098"
> 
> This reverts commit 0da98f7e3b5512bdc876438704c6a895e997daee.
> 
> kafka-1097, unit tests broken due to kafka-1098
> 
> 
> Diffs
> -----
> 
>   core/src/main/scala/kafka/admin/ReassignPartitionsCommand.scala 2f706c94d340f1cd715210b3d8b00597d65b5dd0 
>   core/src/main/scala/kafka/cluster/Partition.scala d8078bd4e73a8a80be436396765ead4c62d334bd 
>   core/src/main/scala/kafka/common/NotAssignedReplicaException.scala PRE-CREATION 
>   core/src/main/scala/kafka/controller/KafkaController.scala 88d130f55997b72a8590e4cfe92857a7320e70d5 
>   core/src/main/scala/kafka/controller/ReplicaStateMachine.scala 212c05d65dcdc147e55f90875bacc940e30342bf 
>   core/src/main/scala/kafka/server/ReplicaManager.scala 7b8f89edecec446664fb4abe64f5dfafb9aa3120 
>   kafka-patch-review.py daf2c3597a81d0d6db574a488f3cf372701fd113 
>   project/Build.scala bcd1ca546b3f081d669892677276a0f19fb1236e 
> 
> Diff: https://reviews.apache.org/r/14865/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Neha Narkhede
> 
>

Re: Review Request 14865: Patch for KAFKA-1097

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

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

Ship it!



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

    One thing worth mention here is that previously we are printing TopicAndPartition.toString and now we are printing Partition.toString, which will be more verbose. This is good for debugging but may flood state-change-log.


- Guozhang Wang


On Oct. 31, 2013, 4:46 a.m., Neha Narkhede wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14865/
> -----------------------------------------------------------
> 
> (Updated Oct. 31, 2013, 4:46 a.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1097
>     https://issues.apache.org/jira/browse/KAFKA-1097
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> Addressed Jun and Guozhang's review comments
> 
> 
> Rebased after KAFKA-1001 was checked in
> 
> 
> Make a reusable updateLeaderEpoch API
> 
> 
> 1. Reverted changes to test log4j.properties and AdminTest 2. Fixed a bug in moving leader logic during partition reassignment:
> 
> 
> 1. Changed partition reassignment behavior to first expand the replication factor and then shrink it. 2. Fixed the reassign partition tool to check for error conditions and improve logging 3. Did a minor improvement to the patch review tool 4. Changed the version to 0.8.1 since it should be that on trunk
> 
> 
> Revert "kafka-1097, unit tests broken due to kafka-1098"
> 
> This reverts commit 0da98f7e3b5512bdc876438704c6a895e997daee.
> 
> kafka-1097, unit tests broken due to kafka-1098
> 
> 
> Diffs
> -----
> 
>   core/src/main/scala/kafka/admin/ReassignPartitionsCommand.scala 2f706c94d340f1cd715210b3d8b00597d65b5dd0 
>   core/src/main/scala/kafka/cluster/Partition.scala d8078bd4e73a8a80be436396765ead4c62d334bd 
>   core/src/main/scala/kafka/common/NotAssignedReplicaException.scala PRE-CREATION 
>   core/src/main/scala/kafka/controller/KafkaController.scala 88d130f55997b72a8590e4cfe92857a7320e70d5 
>   core/src/main/scala/kafka/controller/ReplicaStateMachine.scala 212c05d65dcdc147e55f90875bacc940e30342bf 
>   core/src/main/scala/kafka/server/ReplicaManager.scala 7b8f89edecec446664fb4abe64f5dfafb9aa3120 
>   kafka-patch-review.py daf2c3597a81d0d6db574a488f3cf372701fd113 
>   project/Build.scala bcd1ca546b3f081d669892677276a0f19fb1236e 
> 
> Diff: https://reviews.apache.org/r/14865/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Neha Narkhede
> 
>

Re: Review Request 14865: Patch for KAFKA-1097

Posted by Neha Narkhede <ne...@gmail.com>.

(Updated Oct. 31, 2013, 4:46 a.m.)

Review request for kafka.

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

Repository: kafka

Description (updated)
-------

Addressed Jun and Guozhang's review comments

Rebased after KAFKA-1001 was checked in

Make a reusable updateLeaderEpoch API

1. Reverted changes to test log4j.properties and AdminTest 2. Fixed a bug in moving leader logic during partition reassignment:

Revert "kafka-1097, unit tests broken due to kafka-1098"

This reverts commit 0da98f7e3b5512bdc876438704c6a895e997daee.

kafka-1097, unit tests broken due to kafka-1098

Diffs (updated)
-----

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

Testing
-------

Thanks,

Neha Narkhede

Re: Review Request 14865: Patch for KAFKA-1097

Posted by Neha Narkhede <ne...@gmail.com>.


> On Oct. 29, 2013, 9:12 p.m., Guozhang Wang wrote:
> > core/src/main/scala/kafka/cluster/Partition.scala, line 163
> > <https://reviews.apache.org/r/14865/diff/2/?file=373421#file373421line163>
> >
> >     Also add "// add replicas that are new"

Done


> On Oct. 29, 2013, 9:12 p.m., Guozhang Wang wrote:
> > core/src/main/scala/kafka/cluster/Partition.scala, line 166
> > <https://reviews.apache.org/r/14865/diff/2/?file=373421#file373421line166>
> >
> >     Can we do this removal in one line?

Done


> On Oct. 29, 2013, 9:12 p.m., Guozhang Wang wrote:
> > core/src/main/scala/kafka/controller/KafkaController.scala, line 366
> > <https://reviews.apache.org/r/14865/diff/2/?file=373422#file373422line366>
> >
> >     I think if controller failover at step 3, the new controller will start again from 1, and the AR it reads from ZK has already be AR + RAR, and hence step 1/2 will be skipped, is that right?

If the controller fails over at step 3, the new controller will start by checking if the replicas are in ISR. If yes, it will skip 1 & 2. If not, then it will repeat 1 & 2 (which are idempotent). Either way, it will still complete the reassignment successfully.


> On Oct. 29, 2013, 9:12 p.m., Guozhang Wang wrote:
> > core/src/main/scala/kafka/controller/KafkaController.scala, line 379
> > <https://reviews.apache.org/r/14865/diff/2/?file=373422#file373422line379>
> >
> >     Will the LeaderAndISR request be rejected by the current leader due to its leader epoch?

Yes, if there has been no leader or isr change, it will be rightly ignored. The places where we want to force a state change is where we need to communicate the expanded and shrunk assigned replica list to the leader. That is when we do a dummy leader epoch increment write to the partition's state path.


- Neha


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


On Oct. 29, 2013, 5:49 p.m., Neha Narkhede wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14865/
> -----------------------------------------------------------
> 
> (Updated Oct. 29, 2013, 5:49 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1097
>     https://issues.apache.org/jira/browse/KAFKA-1097
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> Make a reusable updateLeaderEpoch API
> 
> 
> 1. Reverted changes to test log4j.properties and AdminTest 2. Fixed a bug in moving leader logic during partition reassignment:
> 
> 
> 1. Changed partition reassignment behavior to first expand the replication factor and then shrink it. 2. Fixed the reassign partition tool to check for error conditions and improve logging 3. Did a minor improvement to the patch review tool 4. Changed the version to 0.8.1 since it should be that on trunk
> 
> 
> Revert "kafka-1097, unit tests broken due to kafka-1098"
> 
> This reverts commit 0da98f7e3b5512bdc876438704c6a895e997daee.
> 
> kafka-1097, unit tests broken due to kafka-1098
> 
> 
> Diffs
> -----
> 
>   core/src/main/scala/kafka/admin/ReassignPartitionsCommand.scala 2f706c94d340f1cd715210b3d8b00597d65b5dd0 
>   core/src/main/scala/kafka/cluster/Partition.scala 5ccecd179d33abfc14dcefc35dd68de7474c6978 
>   core/src/main/scala/kafka/controller/KafkaController.scala 88d130f55997b72a8590e4cfe92857a7320e70d5 
>   core/src/main/scala/kafka/controller/ReplicaStateMachine.scala 212c05d65dcdc147e55f90875bacc940e30342bf 
>   core/src/main/scala/kafka/server/ReplicaManager.scala ee1cc0cf451b691eb91d9158ca765aeb60fc3dc8 
>   kafka-patch-review.py daf2c3597a81d0d6db574a488f3cf372701fd113 
>   project/Build.scala bcd1ca546b3f081d669892677276a0f19fb1236e 
> 
> Diff: https://reviews.apache.org/r/14865/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Neha Narkhede
> 
>

Re: Review Request 14865: Patch for KAFKA-1097

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

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



core/src/main/scala/kafka/cluster/Partition.scala
<https://reviews.apache.org/r/14865/#comment53841>

    Also add "// add replicas that are new"



core/src/main/scala/kafka/cluster/Partition.scala
<https://reviews.apache.org/r/14865/#comment53840>

    Can we do this removal in one line?



core/src/main/scala/kafka/cluster/Partition.scala
<https://reviews.apache.org/r/14865/#comment53843>

    Ditto as above



core/src/main/scala/kafka/controller/KafkaController.scala
<https://reviews.apache.org/r/14865/#comment53853>

    I think if controller failover at step 3, the new controller will start again from 1, and the AR it reads from ZK has already be AR + RAR, and hence step 1/2 will be skipped, is that right?



core/src/main/scala/kafka/controller/KafkaController.scala
<https://reviews.apache.org/r/14865/#comment53850>

    Will the LeaderAndISR request be rejected by the current leader due to its leader epoch?


- Guozhang Wang


On Oct. 29, 2013, 5:49 p.m., Neha Narkhede wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14865/
> -----------------------------------------------------------
> 
> (Updated Oct. 29, 2013, 5:49 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1097
>     https://issues.apache.org/jira/browse/KAFKA-1097
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> Make a reusable updateLeaderEpoch API
> 
> 
> 1. Reverted changes to test log4j.properties and AdminTest 2. Fixed a bug in moving leader logic during partition reassignment:
> 
> 
> 1. Changed partition reassignment behavior to first expand the replication factor and then shrink it. 2. Fixed the reassign partition tool to check for error conditions and improve logging 3. Did a minor improvement to the patch review tool 4. Changed the version to 0.8.1 since it should be that on trunk
> 
> 
> Revert "kafka-1097, unit tests broken due to kafka-1098"
> 
> This reverts commit 0da98f7e3b5512bdc876438704c6a895e997daee.
> 
> kafka-1097, unit tests broken due to kafka-1098
> 
> 
> Diffs
> -----
> 
>   core/src/main/scala/kafka/admin/ReassignPartitionsCommand.scala 2f706c94d340f1cd715210b3d8b00597d65b5dd0 
>   core/src/main/scala/kafka/cluster/Partition.scala 5ccecd179d33abfc14dcefc35dd68de7474c6978 
>   core/src/main/scala/kafka/controller/KafkaController.scala 88d130f55997b72a8590e4cfe92857a7320e70d5 
>   core/src/main/scala/kafka/controller/ReplicaStateMachine.scala 212c05d65dcdc147e55f90875bacc940e30342bf 
>   core/src/main/scala/kafka/server/ReplicaManager.scala ee1cc0cf451b691eb91d9158ca765aeb60fc3dc8 
>   kafka-patch-review.py daf2c3597a81d0d6db574a488f3cf372701fd113 
>   project/Build.scala bcd1ca546b3f081d669892677276a0f19fb1236e 
> 
> Diff: https://reviews.apache.org/r/14865/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Neha Narkhede
> 
>

Re: Review Request 14865: Patch for KAFKA-1097

Posted by Neha Narkhede <ne...@gmail.com>.

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



core/src/main/scala/kafka/controller/KafkaController.scala
<https://reviews.apache.org/r/14865/#comment53790>

    We have quite a few functions that try to do the versioned write to zookeeper. It might be possible to refactor all of those to reuse parts of the loop logic, but it is tricky. So ideally I would not like to attempt a big cleanup as part of this patch.


- Neha Narkhede


On Oct. 29, 2013, 5:49 p.m., Neha Narkhede wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14865/
> -----------------------------------------------------------
> 
> (Updated Oct. 29, 2013, 5:49 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1097
>     https://issues.apache.org/jira/browse/KAFKA-1097
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> Make a reusable updateLeaderEpoch API
> 
> 
> 1. Reverted changes to test log4j.properties and AdminTest 2. Fixed a bug in moving leader logic during partition reassignment:
> 
> 
> 1. Changed partition reassignment behavior to first expand the replication factor and then shrink it. 2. Fixed the reassign partition tool to check for error conditions and improve logging 3. Did a minor improvement to the patch review tool 4. Changed the version to 0.8.1 since it should be that on trunk
> 
> 
> Revert "kafka-1097, unit tests broken due to kafka-1098"
> 
> This reverts commit 0da98f7e3b5512bdc876438704c6a895e997daee.
> 
> kafka-1097, unit tests broken due to kafka-1098
> 
> 
> Diffs
> -----
> 
>   core/src/main/scala/kafka/admin/ReassignPartitionsCommand.scala 2f706c94d340f1cd715210b3d8b00597d65b5dd0 
>   core/src/main/scala/kafka/cluster/Partition.scala 5ccecd179d33abfc14dcefc35dd68de7474c6978 
>   core/src/main/scala/kafka/controller/KafkaController.scala 88d130f55997b72a8590e4cfe92857a7320e70d5 
>   core/src/main/scala/kafka/controller/ReplicaStateMachine.scala 212c05d65dcdc147e55f90875bacc940e30342bf 
>   core/src/main/scala/kafka/server/ReplicaManager.scala ee1cc0cf451b691eb91d9158ca765aeb60fc3dc8 
>   kafka-patch-review.py daf2c3597a81d0d6db574a488f3cf372701fd113 
>   project/Build.scala bcd1ca546b3f081d669892677276a0f19fb1236e 
> 
> Diff: https://reviews.apache.org/r/14865/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Neha Narkhede
> 
>

Re: Review Request 14865: Patch for KAFKA-1097

Posted by Neha Narkhede <ne...@gmail.com>.


> On Oct. 30, 2013, 5:12 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/controller/KafkaController.scala, lines 397-398
> > <https://reviews.apache.org/r/14865/diff/2/?file=373422#file373422line397>
> >
> >     Should we name this addedReplicas? To me, new and old replicas mean the set of replicas after and before the reassignment.

But added vs old doesn't make much sense. We need a clear way to reference replicas-before-assignment and replicas-after-assignment. So, here, new and old replicas does mean the set of replicas after and before the reassignment. 


> On Oct. 30, 2013, 5:12 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/controller/KafkaController.scala, lines 401-405
> > <https://reviews.apache.org/r/14865/diff/2/?file=373422#file373422line401>
> >
> >     Since we already send the LeaderAndIsr request to both the old and the new replicas in updateLeaderEpochAndSendRequest(). It seems that sending it again to new replicas in startNewReplicasForReassignedPartition() is redundant. I guess we need that call in order to complete the replica state transition?
> >

Yes, we need to do that for performing the state transition. And anyways, it will ignore it if the leader epoch is not > the previous leader epoch


> On Oct. 30, 2013, 5:12 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/controller/KafkaController.scala, lines 874-875
> > <https://reviews.apache.org/r/14865/diff/2/?file=373422#file373422line874>
> >
> >     Should we throw an exception here?

Makes sense. Included that change.


- Neha


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


On Oct. 31, 2013, 4:46 a.m., Neha Narkhede wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14865/
> -----------------------------------------------------------
> 
> (Updated Oct. 31, 2013, 4:46 a.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1097
>     https://issues.apache.org/jira/browse/KAFKA-1097
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> Addressed Jun and Guozhang's review comments
> 
> 
> Rebased after KAFKA-1001 was checked in
> 
> 
> Make a reusable updateLeaderEpoch API
> 
> 
> 1. Reverted changes to test log4j.properties and AdminTest 2. Fixed a bug in moving leader logic during partition reassignment:
> 
> 
> 1. Changed partition reassignment behavior to first expand the replication factor and then shrink it. 2. Fixed the reassign partition tool to check for error conditions and improve logging 3. Did a minor improvement to the patch review tool 4. Changed the version to 0.8.1 since it should be that on trunk
> 
> 
> Revert "kafka-1097, unit tests broken due to kafka-1098"
> 
> This reverts commit 0da98f7e3b5512bdc876438704c6a895e997daee.
> 
> kafka-1097, unit tests broken due to kafka-1098
> 
> 
> Diffs
> -----
> 
>   core/src/main/scala/kafka/admin/ReassignPartitionsCommand.scala 2f706c94d340f1cd715210b3d8b00597d65b5dd0 
>   core/src/main/scala/kafka/cluster/Partition.scala d8078bd4e73a8a80be436396765ead4c62d334bd 
>   core/src/main/scala/kafka/common/NotAssignedReplicaException.scala PRE-CREATION 
>   core/src/main/scala/kafka/controller/KafkaController.scala 88d130f55997b72a8590e4cfe92857a7320e70d5 
>   core/src/main/scala/kafka/controller/ReplicaStateMachine.scala 212c05d65dcdc147e55f90875bacc940e30342bf 
>   core/src/main/scala/kafka/server/ReplicaManager.scala 7b8f89edecec446664fb4abe64f5dfafb9aa3120 
>   kafka-patch-review.py daf2c3597a81d0d6db574a488f3cf372701fd113 
>   project/Build.scala bcd1ca546b3f081d669892677276a0f19fb1236e 
> 
> Diff: https://reviews.apache.org/r/14865/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Neha Narkhede
> 
>

Re: Review Request 14865: Patch for KAFKA-1097

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

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


Thanks for the patch. Could you rebase?


core/src/main/scala/kafka/cluster/Partition.scala
<https://reviews.apache.org/r/14865/#comment54028>

    Should we create a new NotAssignedReplicaException so that we can propagate the error to the client more accurately?



core/src/main/scala/kafka/controller/KafkaController.scala
<https://reviews.apache.org/r/14865/#comment54018>

    Should we name this addedReplicas? To me, new and old replicas mean the set of replicas after and before the reassignment.



core/src/main/scala/kafka/controller/KafkaController.scala
<https://reviews.apache.org/r/14865/#comment54011>

    Since we already send the LeaderAndIsr request to both the old and the new replicas in updateLeaderEpochAndSendRequest(). It seems that sending it again to new replicas in startNewReplicasForReassignedPartition() is redundant. I guess we need that call in order to complete the replica state transition?
    



core/src/main/scala/kafka/controller/KafkaController.scala
<https://reviews.apache.org/r/14865/#comment54014>

    Should we throw an exception here?


- Jun Rao


On Oct. 29, 2013, 5:49 p.m., Neha Narkhede wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14865/
> -----------------------------------------------------------
> 
> (Updated Oct. 29, 2013, 5:49 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1097
>     https://issues.apache.org/jira/browse/KAFKA-1097
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> Make a reusable updateLeaderEpoch API
> 
> 
> 1. Reverted changes to test log4j.properties and AdminTest 2. Fixed a bug in moving leader logic during partition reassignment:
> 
> 
> 1. Changed partition reassignment behavior to first expand the replication factor and then shrink it. 2. Fixed the reassign partition tool to check for error conditions and improve logging 3. Did a minor improvement to the patch review tool 4. Changed the version to 0.8.1 since it should be that on trunk
> 
> 
> Revert "kafka-1097, unit tests broken due to kafka-1098"
> 
> This reverts commit 0da98f7e3b5512bdc876438704c6a895e997daee.
> 
> kafka-1097, unit tests broken due to kafka-1098
> 
> 
> Diffs
> -----
> 
>   core/src/main/scala/kafka/admin/ReassignPartitionsCommand.scala 2f706c94d340f1cd715210b3d8b00597d65b5dd0 
>   core/src/main/scala/kafka/cluster/Partition.scala 5ccecd179d33abfc14dcefc35dd68de7474c6978 
>   core/src/main/scala/kafka/controller/KafkaController.scala 88d130f55997b72a8590e4cfe92857a7320e70d5 
>   core/src/main/scala/kafka/controller/ReplicaStateMachine.scala 212c05d65dcdc147e55f90875bacc940e30342bf 
>   core/src/main/scala/kafka/server/ReplicaManager.scala ee1cc0cf451b691eb91d9158ca765aeb60fc3dc8 
>   kafka-patch-review.py daf2c3597a81d0d6db574a488f3cf372701fd113 
>   project/Build.scala bcd1ca546b3f081d669892677276a0f19fb1236e 
> 
> Diff: https://reviews.apache.org/r/14865/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Neha Narkhede
> 
>

Re: Review Request 14865: Patch for KAFKA-1097

Posted by Neha Narkhede <ne...@gmail.com>.

(Updated Oct. 29, 2013, 5:49 p.m.)

Review request for kafka.

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

Repository: kafka

Description (updated)
-------

Make a reusable updateLeaderEpoch API

1. Reverted changes to test log4j.properties and AdminTest 2. Fixed a bug in moving leader logic during partition reassignment:

Revert "kafka-1097, unit tests broken due to kafka-1098"

This reverts commit 0da98f7e3b5512bdc876438704c6a895e997daee.

kafka-1097, unit tests broken due to kafka-1098

Diffs (updated)
-----

core/src/main/scala/kafka/admin/ReassignPartitionsCommand.scala 2f706c94d340f1cd715210b3d8b00597d65b5dd0
core/src/main/scala/kafka/cluster/Partition.scala 5ccecd179d33abfc14dcefc35dd68de7474c6978
core/src/main/scala/kafka/controller/KafkaController.scala 88d130f55997b72a8590e4cfe92857a7320e70d5
core/src/main/scala/kafka/controller/ReplicaStateMachine.scala 212c05d65dcdc147e55f90875bacc940e30342bf
core/src/main/scala/kafka/server/ReplicaManager.scala ee1cc0cf451b691eb91d9158ca765aeb60fc3dc8
kafka-patch-review.py daf2c3597a81d0d6db574a488f3cf372701fd113
project/Build.scala bcd1ca546b3f081d669892677276a0f19fb1236e

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

Testing
-------

Thanks,

Neha Narkhede