KAFKA-7235: Detect outdated control requests and bounced brokers using broker generation by hzxa21 · Pull Request #5821 · apache/kafka

hzxa21 · 2018-10-20T07:38:39Z

This PR introduces the broker generation concept and leverage it to allow controller to detect fast bounced brokers and allow broker to reject outdated control requests.

It has the changes required to implement KIP-380:

[Common]

Refactor ZookeeperClient to expose the zookeeper multi request directly
Refactor KafkaZkClient to use MultiRequest instead of raw zookeeper transaciton
Atomically get creation transaction id (czxid) with broker znode creation and use it as broker epoch to identify broker gerneration across bounces
Introduce LeaderAndIsrRequest V2, UpdateMetadataRequest V5 and StopReplicaRequest V1 to include broker epoch in control requests and normalize their schemas to make it more memory efficient
Add STALE_BROKER_EPOCH error

[Broker]

Cache the current broker epoch after broker znode registration
Reject LeaderAndIsrRequest, UpdateMetadataRequest and StopReplicaRequest if the request's broker epoch < current broker epoch, and respond back with STALE_BROKER_EPOCH error

[Controller]

Cache/update broker epochs in controllerContext.brokerEpochsCache after reading from zk when processing BrokerChange event and onControllerFailover
Detect bounced brokers in BrokerChange event by comparing the broker epochs get from zk and cached broker epochs and trigger necessary state changes
Avoid sending out requests to dead brokers

[Test]

Add BrokerEpochIntegrationTest to test broker processing new versions of the control requests and rejecting requests with stale broker epoch
Add a test case in ControllerIntegrationTest to test controller detecting bounced brokers
Add test cases in RequestResponseTest to test seralization and de-seralization for new versions of the control requests
Add ControlRequstTest unit test to test control requests schemas normalization

Committer Checklist (excluded from commit message)

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

hzxa21 · 2018-11-02T05:34:56Z

@junrao @ijuma @lindong28 Can you help to review this PR when you have time? Thanks!

lindong28

Thanks for the patch! Left some comments below.

lindong28

Thanks for the update! Left some more comments below.

lindong28

Thanks for the update. Looks good overall. Left a few more minor comments. Can you rebase the patch onto trunk?

lindong28 · 2018-11-12T21:36:29Z

@hzxa21 LGTM. Thanks for the update. Could you rebase the patch?

Hey @junrao @ijuma, would you like to review the patch or it is OK for me to merge it?

junrao · 2018-11-12T21:46:12Z

@hzxa21 , @lindong28 : Yes, I will make another pass in the next couple of days.

junrao

@hzxa21 : Thanks for the patch. Left a few comments below.

junrao · 2018-11-13T23:11:36Z

Could we fix the javadoc above?

junrao · 2018-11-13T23:15:47Z

It would be useful to avoid _1/_2 for better readability. We could do foreach { case (broker, epoch) => ...}

junrao · 2018-11-14T00:43:32Z

It would be useful to avoid _1/_2 for better readability.

junrao · 2018-11-14T00:43:47Z

It would be useful to avoid _1 for better readability.

junrao · 2018-11-14T00:52:38Z

Instead of bouncedBrokerIds(broker.id), it might be clearer to do bouncedBrokerIds.contains(broker.id).

junrao · 2018-11-16T02:08:28Z

The code in this method is quite similar to that in testControlRequestWithCorrectBrokerEpoch(). Should we merged them somehow?

Sure. I have merged them into a single function.

junrao · 2018-11-16T02:11:44Z

Should this be volatile?

Yes. Fixed.

junrao · 2018-11-16T02:21:50Z

This is unnecessary. Great test!

Remove. Thanks!

junrao · 2018-11-16T02:25:08Z

indentation

junrao · 2018-11-16T02:26:06Z

indentation

hzxa21 · 2018-11-29T05:33:35Z

@lindong28 @junrao I have updated the PR to address all the comments. Could you take a second look?

junrao

@hzxa21 : Thanks for the update patch. Just a few more minor comments below.

junrao · 2018-11-30T00:33:25Z

Could initialBrokerEpoch be right after initialBrokerInfo?

Sure. Done.

junrao · 2018-11-30T01:07:40Z

indentation

junrao · 2018-11-30T01:21:33Z

control request => controlled shutdown request

I mean LeaderAndIsr/UpdateMetadata/StopReplica requests here, not controlled shutdown request. I have updated the comment to avoid confusion.

junrao · 2018-11-30T01:26:07Z

The controller part is not right. Non-controllers are registered through this api too.

junrao · 2018-11-30T01:28:31Z

It seems that this logging is redundant since registerBroker() logs the same info already?

Thanks for pointing out. I have removed the log here.

junrao · 2018-11-30T01:58:10Z

normalize => normalizes

junrao · 2018-11-30T02:01:06Z

Perhaps add a comment on how v1 differs from v0?

Sure. Added.

junrao · 2018-11-30T02:02:47Z

normalize => normalizes

junrao · 2018-11-30T02:08:58Z

unused import

junrao · 2018-11-30T02:13:03Z

typo Reuest

…g broker generation

hzxa21 · 2018-11-30T10:08:28Z

@junrao Thanks for the prompt reply. I have updated the PR to address your comments and rebase.

junrao

@hzxa21 : Thanks for the rebase. LGTM. Just a couple of minor comments below.

junrao · 2018-11-30T21:53:05Z

-  def registerBroker(brokerInfo: BrokerInfo): Unit = {
+  /**
+    * Registers the broker in zookeeper and return the broker epoch.
+    * @return broker epoch (znode create transaction id)


Could we put return after param ?

Sure. Done.

junrao · 2018-11-30T21:53:30Z


+  /**
+    * Gets all brokers with broker epoch in the cluster.
+    * @return sequence of brokers in the cluster.


Could we change the comment accordingly?

It seems that you fixed a different line?

Ah. My bad. I mislooked this one. Fixed.

hzxa21 · 2018-11-30T22:21:20Z

@junrao Thanks for the comments. I have addressed your comments in the updated commit.

junrao · 2018-11-30T22:27:28Z

@lindong28 : Do you want to take another look of this PR before merging?

lindong36 · 2018-12-01T06:48:08Z

LGTM. Thanks for the in-depth review @junrao! I am currently not able to access my original github account due to loss of my phone number and can not merge this PR myself.

junrao · 2018-12-01T16:23:00Z

@hzxa21 : Could you address the last comment on line 415 in KafkaZkClient.scala?

hzxa21 · 2018-12-01T16:58:56Z

@junrao My bad. Updated the PR to fix that.

junrao

@hzxa21 : Thanks for the latest patch. LGTM. Also note that broker epoch can potentially be used to improve KAFKA-6029 as well.

hzxa21 · 2018-12-03T06:24:02Z

@junrao Thanks a lot for the review. There are several follow-ups (KAFKA-6029 & KAFKA-7283) we can do after getting the broker epoch. I will address them accordingly and submit PRs. If you are aware of anything that can also benefit from broker epoch, do let me know and I am open to more discussion. Thanks again!

…g broker generation (apache#5821) * KAFKA-7235: Detect outdated control requests and bounced brokers using broker generation * Add broker_epoch in controlled shutdown request * Move broker epoch check into controller for ControlledShutdownRequest * Refactor schema definition for controler requests/responses * Address comments * Address comments * Address comments * Send back STALE_BROKER_EPOCH error in ControlledShutdown response * Fix build issue * Address comments * Address comments * Address comments * Address comments * Fix tests after rebase * Address comments * Address comments

lindong28 self-assigned this Nov 6, 2018

lindong28 reviewed Nov 7, 2018

View reviewed changes

lindong28 reviewed Nov 11, 2018

View reviewed changes

lindong28 reviewed Nov 12, 2018

View reviewed changes

Comment thread core/src/main/scala/kafka/controller/ControllerChannelManager.scala Outdated

Comment thread core/src/main/scala/kafka/server/KafkaApis.scala Outdated

Comment thread core/src/main/scala/kafka/zk/KafkaZkClient.scala Outdated

lindong28 reviewed Nov 12, 2018

View reviewed changes

Comment thread core/src/main/scala/kafka/controller/KafkaController.scala Outdated

hzxa21 force-pushed the KIP-380-PR branch from 517e24b to 88e4eb6 Compare November 13, 2018 19:35

junrao reviewed Nov 16, 2018

View reviewed changes

junrao reviewed Nov 30, 2018

View reviewed changes

hzxa21 added 14 commits November 30, 2018 01:54

KAFKA-7235: Detect outdated control requests and bounced brokers usin…

9802486

…g broker generation

Add broker_epoch in controlled shutdown request

c381b2d

Move broker epoch check into controller for ControlledShutdownRequest

2506649

Refactor schema definition for controler requests/responses

4dc29cd

Address comments

0f91e82

Address comments

02746fd

Address comments

5362018

Send back STALE_BROKER_EPOCH error in ControlledShutdown response

c0780f6

Fix build issue

43f3497

Address comments

bc91c3b

Address comments

c25fdef

Address comments

d422da0

Address comments

565a016

Fix tests after rebase

f33808b

hzxa21 force-pushed the KIP-380-PR branch from 3a2fdfb to f33808b Compare November 30, 2018 10:07

junrao approved these changes Nov 30, 2018

View reviewed changes

Address comments

82d7437

Address comments

aec83c3

junrao approved these changes Dec 3, 2018

View reviewed changes

junrao merged commit 2155c6d into apache:trunk Dec 3, 2018

Uh oh!

Conversation

hzxa21 commented Oct 20, 2018

Committer Checklist (excluded from commit message)

Uh oh!

hzxa21 commented Nov 2, 2018

Uh oh!

lindong28 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lindong28 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lindong28 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lindong28 commented Nov 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

junrao commented Nov 12, 2018

Uh oh!

junrao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

lindong28 left a comment •

edited

Loading

lindong28 commented Nov 12, 2018 •

edited

Loading