KAFKA-9839: Broker should accept control requests with newer broker epoch by apovzner · Pull Request #8509 · apache/kafka

apovzner · 2020-04-18T02:23:17Z

A broker throws IllegalStateException if the broker epoch in the LeaderAndIsr/UpdateMetadataRequest/StopReplicaRequest is larger than its current broker epoch. However, there is no guarantee that the broker would receive the latest broker epoch before the controller: When the broker registers with ZK, there are few more instructions to process before this broker "knows" about its epoch, while the controller may already get notified and send UPDATE_METADATA request (as an example) with the new epoch. This will result in clients getting stale metadata from this broker.

With this PR, a broker accepts LeaderAndIsr/UpdateMetadataRequest/StopReplicaRequest if the broker epoch is newer than the current epoch.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

…poch

dajac

Thanks for the PR. The changes look good to me. However, I wonder if we could also add unit tests in KafkaApisTest to cover this change. What do you think?

apovzner · 2020-04-20T16:17:32Z

@dajac I did initially add a unit test to KafkaApisTest for UpdateMetadataRequest. However, since KafkaApisTest mocks pretty much every other component (like replica manager, etc), the only thing it would test is the fact that passing newer broker epoch would know throw IllegalStateException. Basically, it does not add any more coverage in addition to BrokerEpochIntegrationTest which also tests more of a code path. Given this info, you still think we should add it? Or perhaps there are more reasons I missed?

hachikuji · 2020-04-20T16:30:48Z

ok to test

dajac · 2020-04-21T12:03:56Z

@apovzner Yeah, you're right. It does not increase the overall coverage.

I spent quite some time in the KafkaApis recently and I noticed that its unit test coverage is quite low. If not mistaken, we have literally zero unit tests for the LeaderAndIsr and UpdateMetadata requests over there. I believe that having a reasonable unit tests coverage will help us to catch regression earlier in the development process. When one touches the KafkaApis, the first reflex is to run its unit tests to verify that things are not broken.

I do agree that adding a test which verifies that IllegalStateException is not thrown does not bring much. What do you think about adding few unit tests which verify respectively that a stale broker epoch is rejected and a valid broker epoch is accepted for the LeaderAndIsr, the StopReplica and the UpdateMetadata requests?

apovzner · 2020-04-22T03:22:18Z

@dajac Sounds good! I added unit tests to KafkaApisTest as you suggested.

dajac

Thanks for the updated PR, @apovzner! I left few small comments.

dajac · 2020-04-22T09:06:11Z

+  @Test
+  def testStopReplicaRequestWithCurrentBrokerEpoch(): Unit = {
+    val currentBrokerEpoch = 1239875L
+    testUpdateMetadataRequest(currentBrokerEpoch, currentBrokerEpoch, Errors.NONE)


This and the two above should use testStopReplicaRequest instead of testUpdateMetadataRequest.

thanks for catching this and others below!

dajac · 2020-04-22T09:08:33Z

+  def testStopReplicaRequest(currentBrokerEpoch: Long, brokerEpochInRequest: Long, expectedError: Errors): Unit = {
+    val controllerId = 0
+    val controllerEpoch = 5
+    val brokerEpoch = 230498320L


brokerEpoch is not necessary, I suppose.

dajac · 2020-04-22T09:09:09Z

+      EasyMock.eq(request.context.correlationId),
+      EasyMock.eq(controllerId),
+      EasyMock.eq(controllerEpoch),
+      EasyMock.eq(brokerEpoch + 1),


This should rely on brokerEpochInRequest.

hachikuji

Thanks @apovzner. I just had a minor suggestion in addition to David's comments.

hachikuji · 2020-04-22T15:37:30Z

-      else if (brokerEpochInRequest == curBrokerEpoch) false
-      else throw new IllegalStateException(s"Epoch $brokerEpochInRequest larger than current broker epoch $curBrokerEpoch")
-    }
+    else brokerEpochInRequest < controller.brokerEpoch


Short comment here may be helpful about the case where the controller sees the epoch bump first.

apovzner · 2020-04-22T21:15:33Z

@dajac and @hachikuji thanks for your comments, I addressed them.

hachikuji · 2020-04-22T22:19:56Z

ok to test

hachikuji · 2020-04-22T22:20:05Z

retest this please

hachikuji · 2020-04-22T22:20:15Z

retest this please

hachikuji · 2020-04-23T02:55:55Z

retest this please

dajac

@apovzner Thanks for the updated PR. LGTM. I just left a small cosmetic comment.

dajac · 2020-04-23T06:24:16Z

+      // brokerEpochInRequest > controller.brokerEpoch is possible in rare scenarios where the controller gets notified
+      // about the new broker epoch and sends a control request with this epoch before the broker learns about it


nit: Alignment of the comment looks a bit weird. I would align it with the if/else.

hachikuji · 2020-04-23T16:23:54Z

retest this please

hachikuji

LGTM

hachikuji · 2020-04-24T16:02:42Z

retest this please

hachikuji · 2020-04-24T16:02:57Z

ok to test

…poch (#8509) A broker throws IllegalStateException if the broker epoch in the LeaderAndIsr/UpdateMetadataRequest/StopReplicaRequest is larger than its current broker epoch. However, there is no guarantee that the broker would receive the latest broker epoch before the controller: when the broker registers with ZK, there are few more instructions to process before this broker "knows" about its epoch, while the controller may already get notified and send UPDATE_METADATA request (as an example) with the new epoch. This will result in clients getting stale metadata from this broker. With this PR, a broker accepts LeaderAndIsr/UpdateMetadataRequest/StopReplicaRequest if the broker epoch is newer than the current epoch. Reviewers: David Jacot <djacot@confluent.io>, Jason Gustafson <jason@confluent.io>

…t-for-generated-requests * apache-github/trunk: (366 commits) MINOR: Improve producer test BufferPoolTest#testCloseNotifyWaiters. (apache#7982) MINOR: document how to escape json parameters to ducktape tests (apache#8546) KAFKA-9885; Evict last members of a group when the maximum allowed is reached (apache#8525) KAFKA-9866: Avoid election for topics where preferred leader is not in ISR (apache#8524) KAFKA-9839; Broker should accept control requests with newer broker epoch (apache#8509) KAKFA-9612: Add an option to kafka-configs.sh to add configs from a prop file (KIP-574) MINOR: Partition is under reassignment when adding and removing (apache#8364) MINOR: reduce allocations in log start and recovery checkpoints (apache#8467) MINOR: Remove unused foreign-key join class (apache#8547) HOTFIX: Fix broker bounce system tests (apache#8532) KAFKA-9704: Fix the issue z/OS won't let us resize file when mmap. (apache#8224) KAFKA-8639: Replace AddPartitionsToTxn with Automated Protocol (apache#8326) MINOR: equals() should compare all fields for generated classes (apache#8539) KAFKA-9844; Fix race condition which allows more than maximum number of members(apache#8454) KAFKA-9823: Remember the sent generation for the coordinator request (apache#8445) KAFKA-9883: Add better error message when REST API forwards a request and leader is not known (apache#8536) KAFKA-9907: Switch default build to Scala 2.13 (apache#8537) MINOR: Some html fixes in Streams DSL documentation (apache#8503) MINOR: Enable fatal warnings with scala 2.13 (apache#8429) KAFKA-9852: Change the max duration that calls to the buffer pool can block from 2000ms to 10ms to reduce overall test runtime (apache#8464) ...

…poch (apache#8509) A broker throws IllegalStateException if the broker epoch in the LeaderAndIsr/UpdateMetadataRequest/StopReplicaRequest is larger than its current broker epoch. However, there is no guarantee that the broker would receive the latest broker epoch before the controller: when the broker registers with ZK, there are few more instructions to process before this broker "knows" about its epoch, while the controller may already get notified and send UPDATE_METADATA request (as an example) with the new epoch. This will result in clients getting stale metadata from this broker. With this PR, a broker accepts LeaderAndIsr/UpdateMetadataRequest/StopReplicaRequest if the broker epoch is newer than the current epoch. Reviewers: David Jacot <djacot@confluent.io>, Jason Gustafson <jason@confluent.io>

KAFKA-9839: Broker should accept control requests with newer broker e…

0c9d015

…poch

dajac reviewed Apr 18, 2020

View reviewed changes

added KafkaApisTest unit tests

d8604ed

dajac reviewed Apr 22, 2020

View reviewed changes

hachikuji reviewed Apr 22, 2020

View reviewed changes

fixed testStopReplicaRequestXXX unit tests and added a comment

062c787

dajac approved these changes Apr 23, 2020

View reviewed changes

hachikuji approved these changes Apr 23, 2020

View reviewed changes

adjusted comment alignment

d952854

hachikuji merged commit bd17085 into apache:trunk Apr 27, 2020

viktorsomogyi mentioned this pull request Aug 10, 2020

KAFKA-9839; Broker should accept control requests with newer broker epoch #9150

Closed

3 tasks

		// brokerEpochInRequest > controller.brokerEpoch is possible in rare scenarios where the controller gets notified
		// about the new broker epoch and sends a control request with this epoch before the broker learns about it

Conversation

apovzner commented Apr 18, 2020

Committer Checklist (excluded from commit message)

Uh oh!

dajac left a comment

Choose a reason for hiding this comment

Uh oh!

apovzner commented Apr 20, 2020

Uh oh!

hachikuji commented Apr 20, 2020

Uh oh!

dajac commented Apr 21, 2020

Uh oh!

apovzner commented Apr 22, 2020

Uh oh!

dajac left a comment

Choose a reason for hiding this comment

Uh oh!

dajac Apr 22, 2020

Choose a reason for hiding this comment

Uh oh!

apovzner Apr 22, 2020

Choose a reason for hiding this comment

Uh oh!

dajac Apr 22, 2020

Choose a reason for hiding this comment

Uh oh!

dajac Apr 22, 2020

Choose a reason for hiding this comment

Uh oh!

hachikuji left a comment

Choose a reason for hiding this comment

Uh oh!

hachikuji Apr 22, 2020

Choose a reason for hiding this comment

Uh oh!

apovzner commented Apr 22, 2020

Uh oh!

hachikuji commented Apr 22, 2020

Uh oh!

hachikuji commented Apr 22, 2020

Uh oh!

hachikuji commented Apr 22, 2020

Uh oh!

hachikuji commented Apr 23, 2020

Uh oh!

dajac left a comment

Choose a reason for hiding this comment

Uh oh!

dajac Apr 23, 2020

Choose a reason for hiding this comment

Uh oh!

hachikuji commented Apr 23, 2020

Uh oh!

hachikuji left a comment

Choose a reason for hiding this comment

Uh oh!

hachikuji commented Apr 24, 2020

Uh oh!

hachikuji commented Apr 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants