KAFKA-18844: Fix flaky QuorumControllerTest.testBalancePartitionLeaders()

[FrankYang0529]

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);

Committer Checklist (excluded from commit message)

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

[ijuma]

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

[ijuma]

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]

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]

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]

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.

[FrankYang0529]

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.