You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Jun Rao <ju...@gmail.com> on 2013/11/01 17:33:01 UTC
Re: Review Request 14865: Patch for KAFKA-1097
-----------------------------------------------------------
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 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
>
>