You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2020/09/29 21:52:26 UTC
[GitHub] [kafka] hachikuji opened a new pull request #9353: KAFKA-10521; Skip partition watch registration when `AlterIsr` is expected
hachikuji opened a new pull request #9353:
URL: https://github.com/apache/kafka/pull/9353
Before `AlterIsr` with KIP-497, the controller would register watches in Zookeeper for each reassigning partition so that it could be notified immediately when the ISR was expanded and the reassignment could be completed. This notification is not needed with the latest IBP when `AlterIsr` is enabled because the controller will see all
There is one subtle detail. If we are in the middle of a roll in order to bump the IBP, then it is possible for the controller to be on the latest IBP while some of the brokers are still on the older one. In this case, the brokers on the older IBP will not send `AlterIsr`, but we can still rely on the delayed notification through the `isr_notifications` path to complete reassignments. This seems like a reasonable tradeoff since it should be a short window before the roll is completed.
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] junrao commented on a change in pull request #9353: KAFKA-10521; Skip partition watch registration when `AlterIsr` is expected
Posted by GitBox <gi...@apache.org>.
junrao commented on a change in pull request #9353:
URL: https://github.com/apache/kafka/pull/9353#discussion_r504239853
##########
File path: core/src/test/scala/integration/kafka/admin/ReassignPartitionsIntegrationTest.scala
##########
@@ -60,13 +57,53 @@ class ReassignPartitionsIntegrationTest extends ZooKeeperTestHarness {
brokerId -> brokerLevelThrottles.map(throttle => (throttle, -1L)).toMap
}.toMap
- /**
- * Test running a quick reassignment.
- */
+
@Test
def testReassignment(): Unit = {
cluster = new ReassignPartitionsTestCluster(zkConnect)
cluster.setup()
+ executeAndVerifyReassignment()
+ }
+
+ @Test
+ def testReassignmentWithAlterIsrDisabled(): Unit = {
+ // Test reassignment when the IBP is on an older version which does not use
+ // the `AlterIsr` API. In this case, the controller will register individual
+ // watches for each reassigning partition so that the reassignment can be
+ // completed as soon as the ISR is expanded.
+ val configOverrides = Map(KafkaConfig.InterBrokerProtocolVersionProp -> KAFKA_2_7_IV1.version)
+ cluster = new ReassignPartitionsTestCluster(zkConnect, configOverrides = configOverrides)
+ cluster.setup()
+ executeAndVerifyReassignment()
+ }
+
+ @Test
+ def testReassignmentCompletionDuringPartialUpgrade(): Unit = {
+ // Test reassignment during a partial upgrade when some brokers are relying on
+ // `AlterIsr` and some on rely on the old notification logic through Zookeeper.
Review comment:
typo "some on"
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] junrao commented on a change in pull request #9353: KAFKA-10521; Skip partition watch registration when `AlterIsr` is expected
Posted by GitBox <gi...@apache.org>.
junrao commented on a change in pull request #9353:
URL: https://github.com/apache/kafka/pull/9353#discussion_r503597917
##########
File path: core/src/main/scala/kafka/controller/KafkaController.scala
##########
@@ -1907,6 +1915,20 @@ class KafkaController(val config: KafkaConfig,
}
callback.apply(response)
+
+ // After we have returned the result of the `AlterIsr` request, we should check whether
+ // there are any reassignments which can be completed by a successful ISR expansion.
+ response.left.foreach { alterIsrResponses =>
+ alterIsrResponses.forKeyValue { (topicPartition, partitionResponse) =>
+ if (controllerContext.partitionsBeingReassigned.contains(topicPartition)) {
+ val isSuccessfulUpdate = partitionResponse.isRight
+ if (isSuccessfulUpdate) {
+ val assignment = controllerContext.partitionFullReplicaAssignment(topicPartition)
+ onPartitionReassignment(topicPartition, assignment)
Review comment:
This old logic in processPartitionReassignmentIsrChange() does an info logging before calling onPartitionReassignment(). Should we do the same thing here?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] hachikuji commented on pull request #9353: KAFKA-10521; Skip partition watch registration when `AlterIsr` is expected
Posted by GitBox <gi...@apache.org>.
hachikuji commented on pull request #9353:
URL: https://github.com/apache/kafka/pull/9353#issuecomment-707980589
@junrao Great catch. I had just assumed the check existed. I updated the code and added a partial upgrade integration test case.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] hachikuji merged pull request #9353: KAFKA-10521; Skip partition watch registration when `AlterIsr` is expected
Posted by GitBox <gi...@apache.org>.
hachikuji merged pull request #9353:
URL: https://github.com/apache/kafka/pull/9353
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac commented on pull request #9353: KAFKA-10521; Skip partition watch registration when `AlterIsr` is expected
Posted by GitBox <gi...@apache.org>.
dajac commented on pull request #9353:
URL: https://github.com/apache/kafka/pull/9353#issuecomment-701225542
> There is one subtle detail. If we are in the middle of a roll in order to bump the IBP, then it is possible for the controller to be on the latest IBP while some of the brokers are still on the older one. In this case, the brokers on the older IBP will not send AlterIsr, but we can still rely on the delayed notification through the isr_notifications path to complete reassignments. This seems like a reasonable tradeoff since it should be a short window before the roll is completed.
I do agree that this is a reasonable tradeoff.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org