KAFKA-18844: Stale features information in QuorumController#registerBroker #18997

FrankYang0529 · 2025-02-21T15:30:40Z

In #16848, we added kraft.version to finalized features and got finalized features outside controller event handling thread. This may make finalized features stale when processing registerBroker event. Also, some cases like QuorumControllerTest.testBalancePartitionLeaders become flaky cause of outdated MV. This PR moves finalized features back to controller event handling thread to avoid the error.

Reviewers: Ismael Juma ijuma@apache.org, Jun Rao junrao@gmail.com, Colin P. McCabe cmccabe@apache.org, Chia-Ping Tsai chia7712@gmail.com

Previous PR title: Fix flaky QuorumControllerTest.testBalancePartitionLeaders()
Previous PR description:
When we initialize FeatureControlManager in QuorumController [0], we don't set bootstrapMetadata for it, so it use Metadata.latestProduction() [1]. In the test case, we use IBP_3_7_IV0 as bootstrapMetadata [2]. When initializing QuorumController, it takes time to replay record to overwrite FeatureControlManager#metadataVersion. If we add Thread.sleep(1000L) before metadataVersion.set(mv) [3], we can reproduce the flaky test steadily.

I'm not sure whether setting bootstrapMetadata to FeatureControlManager is correct, so I fix the case by waiting metadataVersion is overwrote.

[0]

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

Lines 1536 to 1541 in 8f13e7c

    
           this.featureControl = new FeatureControlManager.Builder(). 
        
               setLogContext(logContext). 
        
               setQuorumFeatures(quorumFeatures). 
        
               setSnapshotRegistry(snapshotRegistry). 
        
               setClusterFeatureSupportDescriber(clusterSupportDescriber). 
        
               build();

[1]

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

Line 57 in 8f13e7c

private MetadataVersion metadataVersion = MetadataVersion.latestProduction();

[2]

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

Lines 177 to 178 in 8f13e7c

    
           static final BootstrapMetadata SIMPLE_BOOTSTRAP = BootstrapMetadata. 
        
                   fromVersion(MetadataVersion.IBP_3_7_IV0, "test-provided bootstrap");

[3]

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

Lines 371 to 379 in 8f13e7c

    
           public void replay(FeatureLevelRecord record) { 
        
               VersionRange range = quorumFeatures.localSupportedFeature(record.name()); 
        
               if (!range.contains(record.featureLevel())) { 
        
                   throw new RuntimeException("Tried to apply FeatureLevelRecord " + record + ", but this controller only " + 
        
                       "supports versions " + range); 
        
               } 
        
               if (record.name().equals(MetadataVersion.FEATURE_NAME)) { 
        
                   MetadataVersion mv = MetadataVersion.fromFeatureLevel(record.featureLevel()); 
        
                   metadataVersion.set(mv);

ijuma · 2025-02-21T15:32:57Z

This is related to #18845, thanks for the quick fix!

ijuma · 2025-02-21T15:36:05Z

in QuorumController [0], we don't set bootstrapMetadata for it, so it use Metadata.latestProduction()

We tried to remove these defaults as part of #18845 - looks like I missed this one. Can you try removing the default? Is that an easy change or does it cause a lot of issues?

FrankYang0529 · 2025-02-21T15:43:34Z

Can you try removing the default?

I think we can't remove the default one. Each variable get default value if it's null. Or do you mean that trying to set bootstrapMetadata when initialize FeatureControlManager?

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

Lines 96 to 111 in 8f13e7c

    
           if (logContext == null) logContext = new LogContext(); 
        
           if (snapshotRegistry == null) snapshotRegistry = new SnapshotRegistry(logContext); 
        
           if (quorumFeatures == null) { 
        
               Map<String, VersionRange> localSupportedFeatures = new HashMap<>(); 
        
               localSupportedFeatures.put(MetadataVersion.FEATURE_NAME, VersionRange.of( 
        
                       MetadataVersion.MINIMUM_VERSION.featureLevel(), 
        
                       MetadataVersion.latestProduction().featureLevel())); 
        
               quorumFeatures = new QuorumFeatures(0, localSupportedFeatures, Collections.singletonList(0)); 
        
           } 
        
           return new FeatureControlManager( 
        
               logContext, 
        
               quorumFeatures, 
        
               snapshotRegistry, 
        
               metadataVersion, 
        
               clusterSupportDescriber 
        
           );

ijuma · 2025-02-21T16:58:45Z

Yes, if it's mandatory, then we should set it in the constructor. Any issues doing that?

junrao

@FrankYang0529 : Thanks for the PR.

Regarding FeatureControlManager.metadataVersion, it's bootstrapped from the bootstrap file or the log. Any test that depends on it should wait for the bootstrap to complete. So, it does seem reasonable to initialize it to null. @cmccabe : What do you think?

junrao · 2025-02-21T22:52:45Z

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

@@ -753,6 +753,10 @@ public void testBalancePartitionLeaders() throws Throwable {
            QuorumController active = controlEnv.activeController();
            Map<Integer, Long> brokerEpochs = new HashMap<>();

+            TestUtils.waitForCondition(() ->
+                    active.featureControl().finalizedFeatures(Long.MAX_VALUE).featureMap().get(MetadataVersion.FEATURE_NAME) == SIMPLE_BOOTSTRAP.metadataVersion().featureLevel(),


Since we need to wait for QuorumController to fully bootstrap before doing the remaining test, it seems that we should just call controlEnv.activeController(true) here.

Thanks for the suggestion. I think we can remove QuorumControllerTestEnv#activeController(boolean) function, because it's better for all cases get a real active controller. Other test cases like testUncleanShutdownBrokerElrEnabled also has similar error.

ijuma · 2025-02-22T21:08:45Z

So, it does seem reasonable to initialize it to null. @cmccabe : What do you think?

It looks like this would basically always be null outside of tests and it would fail in the following line of the FeatureControlManager constructor:

this.metadataVersion = new TimelineObject<>(snapshotRegistry, metadataVersion);

It seems to me that we should actually remove the metadata version from FeatureControlManager.Builder and rely on FeatureControlManager.replay to set the metadata version.

FrankYang0529 · 2025-02-23T01:30:49Z

@ijuma, it looks like TimelineObject initial value cannot be null, so we can't remove metadata version from FeatureControlManager.Builder.

kafka/server-common/src/main/java/org/apache/kafka/timeline/TimelineObject.java

Lines 55 to 56 in 3fc103b

    
           public TimelineObject(SnapshotRegistry snapshotRegistry, T initialValue) { 
        
               Objects.requireNonNull(initialValue);

ijuma · 2025-02-23T20:36:17Z

@FrankYang0529 Yes, I know it cannot be null. My point is that you'd have to use TimelineObject<Optional<MetadataVersion>> or something long those lines.

chia7712 · 2025-02-24T08:47:49Z

It seems to me that we should actually remove the metadata version from FeatureControlManager.Builder and rely on FeatureControlManager.replay to set the metadata version.

How about initializing it to the bootstrapped metadata? If the role is quorum leader, it will eventually be initialized to the bootstrapped metadata. It is still valid as the "current" metadata version if the role is a standby controller.

ijuma · 2025-02-24T14:37:39Z

@chia7712 Is the bootstrapped metadata available when FeatureControlManager is first initialized?

junrao

@FrankYang0529 : Thanks for the updated PR. A couple of more comments.

How about initializing it to the bootstrapped metadata? If the role is quorum leader, it will eventually be initialized to the bootstrapped metadata. It is still valid as the "current" metadata version if the role is a standby controller.

Chatted with Colin offline a bit. The bootstrap MV (from the bootstrap file) in QC can be higher or lower than the MV in the log. So, it should only be used if the log is empty. We could use MINIMUM_VERSION for initialization and the correct MV will be set after replaying the log. If the log is empty, in production, we prepend the bootstrap MV in QC. In testing, any test depending on the correct MV needs to explicitly ensure that the correct MV is populated.

junrao · 2025-02-24T19:47:55Z

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

-        return activeController(false);
-    }
-
-    QuorumController activeController(boolean waitForActivation) throws InterruptedException {


controlEnv.activeController() is called multiple times in some of the tests. It's a bit wasteful to have to append/wait for a new event in every call. Perhaps we could cache the returned QuorumController and reuse it in those tests.

ijuma · 2025-02-24T22:20:33Z

We could use MINIMUM_VERSION for initialization and the correct MV will be set after replaying the log.

This is error-prone though. While working on #18845, we found an existing race condition that was hard to detect because we silently used the wrong metadata version in a number of situations. I fear this approach may lead to similar problems.

I think it's cleanest to leave the metadata version unset until we actually read it from the log.

junrao · 2025-02-24T22:38:06Z

I think it's cleanest to leave the metadata version unset until we actually read it from the log.

That's safer. The only question is what to do in FeatureControlManager.metadataVersion() when MV is not set yet. Returning an option requires every caller to check it. Perhaps we could throw an exception since the caller is not expected to use this method when MV is not set yet.

ijuma · 2025-02-24T22:56:37Z

Yes, that's the approach we took with FeaturesImage. Most callers use metadataVersionOrThrow. The rare use cases that may call it before it's set call the version that returns Optional and keep retrying until it's set (which is the desired behavior - it's not safe for them to proceed until then).

chia7712 · 2025-02-24T23:08:26Z

that's the approach we took with FeaturesImage. Most callers use metadataVersionOrThrow. The rare use cases that may call it before it's set call the version that returns Optional and keep retrying until it's set (which is the desired behavior - it's not safe for them to proceed until then).

I think it includes following changes.

remove metadataVersion from FeatureControlManager constructor, since the TimelineObject<MetadataVersion> will be changed to TimelineObject<Optional<MetadataVersion>>. And it is initialized with Optional.empty
FeatureControlManager#metadataVersion should be changed to metadataVersionOrThrow, similar to FeaturesImage
CompleteActivationEvent should NOT use FeatureControlManager#metadataVersionOrThrowif the log is empty, since the FeatureControlManager has not yet set the MV.

If above list is correct, maybe we can address it in follow-up?

chia7712 · 2025-02-24T23:48:34Z

If above list is correct, maybe we can address it in follow-up?

open https://issues.apache.org/jira/browse/KAFKA-18858

junrao · 2025-02-24T23:52:03Z

CompleteActivationEvent should NOT use FeatureControlManager#metadataVersionOrThrow if the log is empty, since the FeatureControlManager has not yet set the MV.

We could change FeatureControlManager.metadataVersion to return an option and use it in CompleteActivationEvent. With this, we could remove the isEmpty param in ActivationRecordsGenerator.generate() since it's the same as metadataVersion being empty.

We could do 1-3 in a followup jira.

junrao · 2025-02-25T00:59:27Z

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

@@ -373,7 +373,7 @@ public  void testElrEnabledByDefault() throws Throwable {
                )).
                build()
        ) {
-            controlEnv.activeController(true);
+            controlEnv.activeController();


Thinking a bit more. It seems that this test exposes a real bug introduced in https://github.com/apache/kafka/pull/16848/files#diff-77dc2adb187fd078084644613cff2b53021c8a5fbcdcfa116515734609d1332a.

In the following code, we pick up the finalized feature version (including MV) first and pass them to the registerBroker event. It's possible that the passed in MV is outdated, but when the registerBroker event is processed, the MV becomes update to date. This will fail a registerBroker request that shouldn't be failed. It seems that we should pick up the finalized feature version when the registerBroker event is processed.

public CompletableFuture<BrokerRegistrationReply> registerBroker( ControllerRequestContext context, BrokerRegistrationRequestData request ) { // populate finalized features map with latest known kraft version for validation Map<String, Short> controllerFeatures = new HashMap<>(featureControl.finalizedFeatures(Long.MAX_VALUE).featureMap()); controllerFeatures.put(KRaftVersion.FEATURE_NAME, raftClient.kraftVersion().featureLevel()); return appendWriteEvent("registerBroker", context.deadlineNs(), () -> clusterControl. registerBroker(request, offsetControl.nextWriteOffset(), new FinalizedControllerFeatures(controllerFeatures, Long.MAX_VALUE), context.requestHeader().requestApiVersion() >= 3), EnumSet.noneOf(ControllerOperationFlag.class)); }

Colin confirmed that this is an issue. In general, we should not be accessing featureControl.finalizedFeatures outside of the event queue thread since it introduces potential concurrency issues.

I try to change the function as following and the test can pass without active controller check.

@Override public CompletableFuture<BrokerRegistrationReply> registerBroker( ControllerRequestContext context, BrokerRegistrationRequestData request ) { return appendWriteEvent("registerBroker", context.deadlineNs(), () -> { // populate finalized features map with latest known kraft version for validation Map<String, Short> controllerFeatures = new HashMap<>(featureControl.finalizedFeatures(Long.MAX_VALUE).featureMap()); controllerFeatures.put(KRaftVersion.FEATURE_NAME, raftClient.kraftVersion().featureLevel()); return clusterControl. registerBroker(request, offsetControl.nextWriteOffset(), new FinalizedControllerFeatures(controllerFeatures, Long.MAX_VALUE), context.requestHeader().requestApiVersion() >= 3); }, EnumSet.noneOf(ControllerOperationFlag.class)); }

Yes, exactly. If we fix the registerBroker code, just waiting for the QC to be the leader should be enough.

Nice catch @junrao!

…rs() Signed-off-by: PoAn Yang <payang@apache.org>

ijuma · 2025-02-25T15:38:30Z

Once we merge this, we should also backport it to 4.0 so the build is stable there (and the bug is fixed).

junrao

@FrankYang0529 : Thanks for the updated PR. Just a minor comment. Also, could we change the description of the jira and the PR since this is not just a flaky test fix?

@ahuang98 and @jsancio : Should we cherry pick this to 3.9 since the issue was introduced in https://github.com/apache/kafka/pull/16848/files#diff-77dc2adb187fd078084644613cff2b53021c8a5fbcdcfa116515734609d1332a?

junrao · 2025-02-25T17:52:29Z

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

-                    context.requestHeader().requestApiVersion() >= 3),
+            () -> {
+                // Populate finalized features map with latest known kraft version for validation.
+                // Get the finalized features map in controller operation to avoid outdated features.


in controller operation => in the controller event handling thread

Isn't the important point that both reads and writes have to happen within the controller event handling thread (versus the more specific finalized features map part)? Overly specific comments tend to go stale more easily.

Signed-off-by: PoAn Yang <payang@apache.org>

junrao

@FrankYang0529 : Thanks for the updated PR. LGTM too assuming the tests pass. Also, could you change the title/description of the jira and the PR?

FrankYang0529 · 2025-02-26T01:30:51Z

Hi @junrao, thanks for the review. Updated title and description on Jira and the PR.

mumrah · 2025-02-26T18:07:43Z

Regarding:

This was added in #18985 as part of the merge queue effort. It is not yet required for this check to pass, but if you want to fix the failed check you can add the "Reviewers:" line to the end of the PR body.

junrao · 2025-02-26T18:16:59Z

@FrankYang0529 : Thanks for the updated description. Just a minor comment.

This makes finalized features may be stale when processing registerBroker event. => This may make finalized features stale when processing registerBroker event.

cmccabe

LGTM for the current change. Thanks, all. I wish we had tools to find cases where we accessed variables from the wrong thread.

jsancio · 2025-02-26T21:12:59Z

@junrao @cmccabe @ijuma @chia7712 I added 3.9.1 and 4.0.0 as the fix version so we should cherry pick it to those branches if you agree.

chia7712 · 2025-02-26T21:46:44Z

I added 3.9.1 and 4.0.0 as the fix version so we should cherry pick it to those branches if you agree.

+1

I wish we had tools to find cases where we accessed variables from the wrong thread.

open https://issues.apache.org/jira/browse/KAFKA-18877 to log/discuss it

FrankYang0529 · 2025-02-27T01:29:03Z

@junrao updated PR description. Also, added reviewers to it. Thanks.

junrao · 2025-02-27T17:10:49Z

@FrankYang0529 : Thanks for the updated description. LGTM

@jsancio : +1 on cherry-pick this to 3.9 too.

junrao · 2025-02-27T17:31:36Z

@jsancio : I merged the PR to trunk. Could you help cherry-pick it to 4.0 and 3.9?

…roker (apache#18997) In apache#16848, we added `kraft.version` to finalized features and got finalized features outside controller event handling thread. This may make finalized features stale when processing `registerBroker` event. Also, some cases like `QuorumControllerTest.testBalancePartitionLeaders` become flaky cause of outdated MV. This PR moves finalized features back to controller event handling thread to avoid the error. Reviewers: Ismael Juma <ijuma@apache.org>, Jun Rao <junrao@gmail.com>, Colin P. McCabe <cmccabe@apache.org>, Chia-Ping Tsai <chia7712@gmail.com> (cherry picked from commit 88a23da)

FrankYang0529 · 2025-02-28T13:39:52Z

3.9 PR: #19058

…roker (apache#18997) In apache#16848, we added `kraft.version` to finalized features and got finalized features outside controller event handling thread. This may make finalized features stale when processing `registerBroker` event. Also, some cases like `QuorumControllerTest.testBalancePartitionLeaders` become flaky cause of outdated MV. This PR moves finalized features back to controller event handling thread to avoid the error. Reviewers: Ismael Juma <ijuma@apache.org>, Jun Rao <junrao@gmail.com>, Colin P. McCabe <cmccabe@apache.org>, Chia-Ping Tsai <chia7712@gmail.com> (cherry picked from commit 88a23da)

…roker (#18997) In #16848, we added `kraft.version` to finalized features and got finalized features outside controller event handling thread. This may make finalized features stale when processing `registerBroker` event. Also, some cases like `QuorumControllerTest.testBalancePartitionLeaders` become flaky cause of outdated MV. This PR moves finalized features back to controller event handling thread to avoid the error. Reviewers: Ismael Juma <ijuma@apache.org>, Jun Rao <junrao@gmail.com>, Colin P. McCabe <cmccabe@apache.org>, Chia-Ping Tsai <chia7712@gmail.com>

dajac · 2025-03-03T14:19:16Z

I cherry-picked the commit to 4.0. cc @junrao

…roker (apache#18997) In apache#16848, we added `kraft.version` to finalized features and got finalized features outside controller event handling thread. This may make finalized features stale when processing `registerBroker` event. Also, some cases like `QuorumControllerTest.testBalancePartitionLeaders` become flaky cause of outdated MV. This PR moves finalized features back to controller event handling thread to avoid the error. Reviewers: Ismael Juma <ijuma@apache.org>, Jun Rao <junrao@gmail.com>, Colin P. McCabe <cmccabe@apache.org>, Chia-Ping Tsai <chia7712@gmail.com>

github-actions bot added tests Test fixes (including flaky tests) kraft small Small PRs labels Feb 21, 2025

junrao reviewed Feb 21, 2025

View reviewed changes

FrankYang0529 force-pushed the KAFKA-18844 branch from 0f8190a to 2544030 Compare February 22, 2025 02:32

junrao reviewed Feb 24, 2025

View reviewed changes

junrao reviewed Feb 25, 2025

View reviewed changes

KAFKA-18844: Fix flaky QuorumControllerTest.testBalancePartitionLeade…

0543db9

…rs() Signed-off-by: PoAn Yang <payang@apache.org>

FrankYang0529 force-pushed the KAFKA-18844 branch from 2544030 to 0543db9 Compare February 25, 2025 01:47

junrao reviewed Feb 25, 2025

View reviewed changes

update comment

99af2b4

Signed-off-by: PoAn Yang <payang@apache.org>

chia7712 approved these changes Feb 26, 2025

View reviewed changes

junrao approved these changes Feb 26, 2025

View reviewed changes

FrankYang0529 changed the title ~~KAFKA-18844: Fix flaky QuorumControllerTest.testBalancePartitionLeaders()~~ KAFKA-18844: Stale features information in QuorumController#registerBroker Feb 26, 2025

cmccabe approved these changes Feb 26, 2025

View reviewed changes

junrao merged commit 88a23da into apache:trunk Feb 27, 2025
27 checks passed

junrao mentioned this pull request Feb 27, 2025

MINOR: correct an ELR test name in ActivationRecordsGeneratorTest #19044

Merged

FrankYang0529 deleted the KAFKA-18844 branch February 28, 2025 01:01

	this.featureControl = new FeatureControlManager.Builder().
	setLogContext(logContext).
	setQuorumFeatures(quorumFeatures).
	setSnapshotRegistry(snapshotRegistry).
	setClusterFeatureSupportDescriber(clusterSupportDescriber).
	build();

	static final BootstrapMetadata SIMPLE_BOOTSTRAP = BootstrapMetadata.
	fromVersion(MetadataVersion.IBP_3_7_IV0, "test-provided bootstrap");

	public void replay(FeatureLevelRecord record) {
	VersionRange range = quorumFeatures.localSupportedFeature(record.name());
	if (!range.contains(record.featureLevel())) {
	throw new RuntimeException("Tried to apply FeatureLevelRecord " + record + ", but this controller only " +
	"supports versions " + range);
	}
	if (record.name().equals(MetadataVersion.FEATURE_NAME)) {
	MetadataVersion mv = MetadataVersion.fromFeatureLevel(record.featureLevel());
	metadataVersion.set(mv);

KAFKA-18844: Stale features information in QuorumController#registerBroker #18997

KAFKA-18844: Stale features information in QuorumController#registerBroker #18997

Uh oh!

Conversation

FrankYang0529 commented Feb 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ijuma commented Feb 21, 2025

Uh oh!

ijuma commented Feb 21, 2025

Uh oh!

FrankYang0529 commented Feb 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ijuma commented Feb 21, 2025

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!

ijuma commented Feb 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

FrankYang0529 commented Feb 23, 2025

Uh oh!

ijuma commented Feb 23, 2025

Uh oh!

chia7712 commented Feb 24, 2025

Uh oh!

ijuma commented Feb 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

junrao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ijuma commented Feb 24, 2025

Uh oh!

junrao commented Feb 24, 2025

Uh oh!

ijuma commented Feb 24, 2025

Uh oh!

chia7712 commented Feb 24, 2025

Uh oh!

chia7712 commented Feb 24, 2025

Uh oh!

junrao commented Feb 24, 2025

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!

ijuma commented Feb 25, 2025

Uh oh!

junrao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ijuma Feb 25, 2025 • 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!

junrao left a comment

Choose a reason for hiding this comment

Uh oh!

FrankYang0529 commented Feb 26, 2025

Uh oh!

mumrah commented Feb 26, 2025

Uh oh!

junrao commented Feb 26, 2025

Uh oh!

FrankYang0529 commented Feb 21, 2025 •

edited

Loading

FrankYang0529 commented Feb 21, 2025 •

edited

Loading

ijuma commented Feb 22, 2025 •

edited

Loading

ijuma commented Feb 24, 2025 •

edited

Loading

ijuma Feb 25, 2025 •

edited

Loading