KAFKA-13916; Fenced replicas should not be allowed to join the ISR in KRaft (KIP-841, Part 1) by dajac · Pull Request #12240 · apache/kafka

dajac · 2022-06-02T13:38:47Z

This PR implements a first part of KIP-841. Specifically, it implements the following:

Adds a new metadata version.
Adds the InControlledShutdown field to the BrokerRegistrationRecord and BrokerRegistrationChangeRecord and bump their versions. The newest versions are only used if the new metadata version is enabled.
Writes a BrokerRegistrationChangeRecord with InControlledShutdown set when a broker requests a controlled shutdown.
Ensures that fenced and in controlled shutdown replicas are not picked as leaders nor included in the ISR.
Adds or extends unit tests.

The second half (the AlterPartition part) is implemented in #12181.

Committer Checklist (excluded from commit message)

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

…when broker requests a controlled shutdown

jsancio

Thanks for the changes @dajac. Review of src/main. only skimmed src/test.

metadata/src/main/java/org/apache/kafka/controller/ClusterControlManager.java

metadata/src/test/java/org/apache/kafka/controller/ReplicationControlManagerTest.java

metadata/src/main/java/org/apache/kafka/image/ClusterDelta.java

metadata/src/main/java/org/apache/kafka/metadata/BrokerRegistration.java

...ta/src/main/java/org/apache/kafka/metadata/BrokerRegistrationInControlledShutdownChange.java

jsancio · 2022-06-05T18:34:43Z

metadata/src/main/resources/common/metadata/BrokerRegistrationChangeRecord.json

  "type": "metadata",
  "name": "BrokerRegistrationChangeRecord",
-  "validVersions": "0",
+  "validVersions": "0-1",


Hmm. This version bump is not strictly required since we are protecting the tagged field with the metadata version.

For example, in PartitionChangeRecord we added the tagged field LeaderRecoveryState without changing the version.

@mumrah @cmccabe Do you have an opinion on this?

I actually followed @cmccabe's suggestion.

Okay. I am not sure where or how to document this decision but it would be good to be consistent across change type records.

I actually think we should not increase the record version unless there is an incompatible change. To my knowledge, that is what the record version has conveyed historically. However, I don't think there's any harm in increasing it, and I also don't think we need to solve it in this PR. Let's go with Colin's suggestion here and we can continue this discussion on the mailing list or offline

…ta-records

dajac · 2022-06-06T08:09:20Z

@jsancio Thanks for your review. I have addressed or replied to your comments.

jsancio · 2022-06-06T20:52:01Z

metadata/src/main/java/org/apache/kafka/image/ClusterDelta.java

-        changedBrokers.put(record.id(), Optional.of(broker.cloneWithFencing(true)));
+        changedBrokers.put(record.id(), broker.maybeCloneWith(
+            BrokerRegistrationFencingChange.UNFENCE.asBoolean(),
+            Optional.empty()
+        ));
    }

    public void replay(UnfenceBrokerRecord record) {
        BrokerRegistration broker = getBrokerOrThrow(record.id(), record.epoch(), "unfence");
-        changedBrokers.put(record.id(), Optional.of(broker.cloneWithFencing(false)));
+        changedBrokers.put(record.id(), broker.maybeCloneWith(
+            BrokerRegistrationFencingChange.FENCE.asBoolean(),
+            Optional.empty()
+        ));


We have an inflight change that changes these enum values around. We need to be careful in the order we merge this PRs: #12236 (comment)

jsancio · 2022-06-06T21:11:18Z

metadata/src/main/java/org/apache/kafka/image/MetadataImage.java

+        if (metadataVersion.equals(MetadataVersion.UNINITIALIZED)) {
+            metadataVersion = MetadataVersion.IBP_3_0_IV1;
+        }


I think we need a similar change for the ClusterControlManager iterator. If I am not mistaken it is possible for the FeatureControlManager to return UNINITIALIZED in both active controllers and inactive controllers.

Should we document this default version in MetadataVersion instead?

cc @mumrah

@mumrah is removing UNINITIALIZED in #12250. We can remove this logic afterwards.

Right, once that PR is merged, KRaft will be at metadata version IBP_3_0_IV1 implicitly until the controller finishes bootstrapping. This will be true on the controller and broker side of things

jsancio · 2022-06-06T21:46:17Z

metadata/src/main/java/org/apache/kafka/controller/ClusterControlManager.java

+     * Returns true if the broker is in fenced state; Returns false if it is
+     * not or if it does not exist.
+     */
    public boolean unfenced(int brokerId) {


Minor but I am pretty sure that we don't use this method anymore in src/main. If so, I say that we just remove it.

That's right. However, it is used in many places in the tests. I haven't found a good way to replace it in tests that is as convenient as this predicate. I would keep it.

metadata/src/main/java/org/apache/kafka/metadata/BrokerRegistration.java

dengziming

Good job, only one minor suggestion.

dengziming · 2022-06-07T03:04:10Z

metadata/src/main/java/org/apache/kafka/controller/ClusterControlManager.java

            .collect(Collectors.toSet());
    }

+    private short registerBrokerRecordVersion() {


How about moving this method to MetadataVersion? I find this logic also used in BrokerRegistration.toRecord

dengziming · 2022-06-07T03:07:36Z

metadata/src/main/java/org/apache/kafka/controller/ClusterControlManager.java

+            record,
+            record.id(),
+            record.epoch(),
+            BrokerRegistrationFencingChange.UNFENCE.asBoolean(),


Similar to Jose mentioned, this should be noticed if #12236 merged first.

…olManager.Builder

jsancio

LGTM. Test failures seem unrelated.

dajac added 8 commits June 2, 2022 08:58

add new metadata version

27c0c40

update BrokerRegistrationRecord and BrokerRegistrationChangeRecord

4a1052c

wire RegisterBrokerRecord

85a76a4

wire BrokerRegistrationChangeRecord

8ca98bd

write BrokerRegistrationChangeRecord with InControlledShutdown state …

6da0fed

…when broker requests a controlled shutdown

enforce invariants

e1f2d60

fix core tests

bbc95da

refactor + image test

3ea2126

dajac requested a review from jsancio June 2, 2022 15:47

dajac added 2 commits June 2, 2022 20:58

cleanups and fixes

d9a1af8

refactor

b65e57f

dajac mentioned this pull request Jun 3, 2022

KAFKA-13916; Fenced replicas should not be allowed to join the ISR in KRaft (KIP-841, Part 2) #12181

Merged

3 tasks

jsancio reviewed Jun 5, 2022

View reviewed changes

dajac added 4 commits June 6, 2022 08:15

address minor comments

db82247

add javadoc

13e7c0a

address comments

bb2bb7c

Merge remote-tracking branch 'upstream/trunk' into KAFKA-13916-metada…

f42c8fa

…ta-records

use MetadataVersion.IBP_3_0_IV1 by default

2761eef

dajac force-pushed the KAFKA-13916-metadata-records branch from b8caf3a to 2761eef Compare June 6, 2022 11:01

jsancio reviewed Jun 6, 2022

View reviewed changes

dengziming reviewed Jun 7, 2022

View reviewed changes

dajac added 4 commits June 7, 2022 10:14

refactor and address comments

87446cd

don't automagically instanciate FeatureControlManager in ClusterContr…

1450f43

…olManager.Builder

refactor

4bd208c

cleanup

c3f8559

jsancio approved these changes Jun 7, 2022

View reviewed changes

jsancio merged commit 151ca12 into apache:trunk Jun 7, 2022

dajac deleted the KAFKA-13916-metadata-records branch June 7, 2022 19:43

Conversation

dajac commented Jun 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

jsancio 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!

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!

dajac commented Jun 6, 2022

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!

Uh oh!

dengziming 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!

jsancio left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dajac commented Jun 2, 2022 •

edited

Loading