You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "FrankYang0529 (via GitHub)" <gi...@apache.org> on 2025/02/21 15:30:45 UTC
[PR] KAFKA-18844: Fix flaky QuorumControllerTest.testBalancePartitionLeaders() [kafka]
FrankYang0529 opened a new pull request, #18997:
URL: https://github.com/apache/kafka/pull/18997
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]
https://github.com/apache/kafka/blob/8f13e7c20737400a2b7b93449c20146089df00a6/metadata/src/main/java/org/apache/kafka/controller/QuorumController.java#L1536-L1541
[1]
https://github.com/apache/kafka/blob/8f13e7c20737400a2b7b93449c20146089df00a6/metadata/src/main/java/org/apache/kafka/controller/FeatureControlManager.java#L57
[2]
https://github.com/apache/kafka/blob/8f13e7c20737400a2b7b93449c20146089df00a6/metadata/src/test/java/org/apache/kafka/controller/QuorumControllerTest.java#L177-L178
[3]
https://github.com/apache/kafka/blob/8f13e7c20737400a2b7b93449c20146089df00a6/metadata/src/main/java/org/apache/kafka/controller/FeatureControlManager.java#L371-L379
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18844: Fix flaky QuorumControllerTest.testBalancePartitionLeaders() [kafka]
Posted by "junrao (via GitHub)" <gi...@apache.org>.
junrao commented on code in PR #18997:
URL: https://github.com/apache/kafka/pull/18997#discussion_r1968669917
##########
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();
Review Comment:
Yes, exactly. If we fix the registerBroker code, just waiting for the QC to be the leader should be enough.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18844: Stale features information in QuorumController#registerBroker [kafka]
Posted by "mumrah (via GitHub)" <gi...@apache.org>.
mumrah commented on PR #18997:
URL: https://github.com/apache/kafka/pull/18997#issuecomment-2685820634
Regarding:
<img width="547" alt="image" src="https://github.com/user-attachments/assets/096cf482-4701-4219-a8eb-c5613adfb390" />
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18844: Fix flaky QuorumControllerTest.testBalancePartitionLeaders() [kafka]
Posted by "FrankYang0529 (via GitHub)" <gi...@apache.org>.
FrankYang0529 commented on PR #18997:
URL: https://github.com/apache/kafka/pull/18997#issuecomment-2674890232
> Can you try removing the default?
I think we can't remove the default one. Each variables get default. value if it's null. Or do you mean that trying to set `bootstrapMetadata` when initialize `FeatureControlManager`?
https://github.com/apache/kafka/blob/8f13e7c20737400a2b7b93449c20146089df00a6/metadata/src/main/java/org/apache/kafka/controller/FeatureControlManager.java#L96-L111
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18844: Stale features information in QuorumController#registerBroker [kafka]
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on PR #18997:
URL: https://github.com/apache/kafka/pull/18997#issuecomment-2694548745
I cherry-picked the commit to 4.0. cc @junrao
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18844: Stale features information in QuorumController#registerBroker [kafka]
Posted by "junrao (via GitHub)" <gi...@apache.org>.
junrao commented on PR #18997:
URL: https://github.com/apache/kafka/pull/18997#issuecomment-2685839532
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18844: Stale features information in QuorumController#registerBroker [kafka]
Posted by "FrankYang0529 (via GitHub)" <gi...@apache.org>.
FrankYang0529 commented on PR #18997:
URL: https://github.com/apache/kafka/pull/18997#issuecomment-2686577937
@junrao updated PR description. Also, added reviewers to it. Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18844: Fix flaky QuorumControllerTest.testBalancePartitionLeaders() [kafka]
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on PR #18997:
URL: https://github.com/apache/kafka/pull/18997#issuecomment-2682409876
Once we merge this, we should also backport it to 4.0 so the build is stable there (and the bug is fixed).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18844: Fix flaky QuorumControllerTest.testBalancePartitionLeaders() [kafka]
Posted by "junrao (via GitHub)" <gi...@apache.org>.
junrao commented on code in PR #18997:
URL: https://github.com/apache/kafka/pull/18997#discussion_r1968324927
##########
metadata/src/test/java/org/apache/kafka/controller/QuorumControllerTestEnv.java:
##########
@@ -135,10 +135,6 @@ private QuorumControllerTestEnv(
}
QuorumController activeController() throws InterruptedException {
- return activeController(false);
- }
-
- QuorumController activeController(boolean waitForActivation) throws InterruptedException {
Review Comment:
`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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18844: Fix flaky QuorumControllerTest.testBalancePartitionLeaders() [kafka]
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on PR #18997:
URL: https://github.com/apache/kafka/pull/18997#issuecomment-2679793666
> 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](https://github.com/apache/kafka/pull/18845/commits/70667a4935afe8891292410e48b5b7b945bf0aaa#r1956516319) 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18844: Fix flaky QuorumControllerTest.testBalancePartitionLeaders() [kafka]
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on code in PR #18997:
URL: https://github.com/apache/kafka/pull/18997#discussion_r1970032554
##########
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();
Review Comment:
Nice catch @junrao!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18844: Fix flaky QuorumControllerTest.testBalancePartitionLeaders() [kafka]
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on PR #18997:
URL: https://github.com/apache/kafka/pull/18997#issuecomment-2674872246
> 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18844: Fix flaky QuorumControllerTest.testBalancePartitionLeaders() [kafka]
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on PR #18997:
URL: https://github.com/apache/kafka/pull/18997#issuecomment-2674865005
This is related to #18845, thanks for the quick fix!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18844: Fix flaky QuorumControllerTest.testBalancePartitionLeaders() [kafka]
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on PR #18997:
URL: https://github.com/apache/kafka/pull/18997#issuecomment-2678625081
@chia7712 Is the bootstrapped metadata available when `FeatureControlManager` is initialized?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18844: Fix flaky QuorumControllerTest.testBalancePartitionLeaders() [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on PR #18997:
URL: https://github.com/apache/kafka/pull/18997#issuecomment-2677756856
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18844: Fix flaky QuorumControllerTest.testBalancePartitionLeaders() [kafka]
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on code in PR #18997:
URL: https://github.com/apache/kafka/pull/18997#discussion_r1970567923
##########
metadata/src/main/java/org/apache/kafka/controller/QuorumController.java:
##########
@@ -2004,14 +2004,17 @@ 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),
+ () -> {
+ // Populate finalized features map with latest known kraft version for validation.
+ // Get the finalized features map in controller operation to avoid outdated features.
Review Comment:
Isn't the important point that both reads and writes have to happen within the controller event handling thread (versus the specific finalized features map part)? Overly specific comments tend to go stale more easily.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18844: Stale features information in QuorumController#registerBroker [kafka]
Posted by "jsancio (via GitHub)" <gi...@apache.org>.
jsancio commented on PR #18997:
URL: https://github.com/apache/kafka/pull/18997#issuecomment-2686209724
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18844: Stale features information in QuorumController#registerBroker [kafka]
Posted by "FrankYang0529 (via GitHub)" <gi...@apache.org>.
FrankYang0529 commented on PR #18997:
URL: https://github.com/apache/kafka/pull/18997#issuecomment-2690675552
3.9 PR: https://github.com/apache/kafka/pull/19058
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18844: Fix flaky QuorumControllerTest.testBalancePartitionLeaders() [kafka]
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on PR #18997:
URL: https://github.com/apache/kafka/pull/18997#issuecomment-2675079361
Yes, if it's mandatory, then we should set it in the constructor. Any issues doing that?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18844: Fix flaky QuorumControllerTest.testBalancePartitionLeaders() [kafka]
Posted by "junrao (via GitHub)" <gi...@apache.org>.
junrao commented on code in PR #18997:
URL: https://github.com/apache/kafka/pull/18997#discussion_r1968646894
##########
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();
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18844: Fix flaky QuorumControllerTest.testBalancePartitionLeaders() [kafka]
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on PR #18997:
URL: https://github.com/apache/kafka/pull/18997#issuecomment-2676396427
> 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` and rely on `FeatureControlManager.replay` to set the metadata version.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18844: Fix flaky QuorumControllerTest.testBalancePartitionLeaders() [kafka]
Posted by "FrankYang0529 (via GitHub)" <gi...@apache.org>.
FrankYang0529 commented on PR #18997:
URL: https://github.com/apache/kafka/pull/18997#issuecomment-2676491844
@ijuma, it looks like `TimelineObject` initial value cannot be `null`, so we can't remove metadata version from `FeatureControlManager.Builder`.
https://github.com/apache/kafka/blob/3fc103b48b65fc37d8ae22d4d7c36104ae1b3b35/server-common/src/main/java/org/apache/kafka/timeline/TimelineObject.java#L55-L56
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18844: Fix flaky QuorumControllerTest.testBalancePartitionLeaders() [kafka]
Posted by "FrankYang0529 (via GitHub)" <gi...@apache.org>.
FrankYang0529 commented on code in PR #18997:
URL: https://github.com/apache/kafka/pull/18997#discussion_r1966401253
##########
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(),
Review Comment:
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.
<img width="1306" alt="Screenshot 2025-02-22 at 10 37 39 AM" src="https://github.com/user-attachments/assets/f5e009e2-d788-4219-aeff-83dd72c9c78e" />
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18844: Fix flaky QuorumControllerTest.testBalancePartitionLeaders() [kafka]
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on code in PR #18997:
URL: https://github.com/apache/kafka/pull/18997#discussion_r1970567923
##########
metadata/src/main/java/org/apache/kafka/controller/QuorumController.java:
##########
@@ -2004,14 +2004,17 @@ 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),
+ () -> {
+ // Populate finalized features map with latest known kraft version for validation.
+ // Get the finalized features map in controller operation to avoid outdated features.
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18844: Stale features information in QuorumController#registerBroker [kafka]
Posted by "junrao (via GitHub)" <gi...@apache.org>.
junrao merged PR #18997:
URL: https://github.com/apache/kafka/pull/18997
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18844: Fix flaky QuorumControllerTest.testBalancePartitionLeaders() [kafka]
Posted by "FrankYang0529 (via GitHub)" <gi...@apache.org>.
FrankYang0529 commented on code in PR #18997:
URL: https://github.com/apache/kafka/pull/18997#discussion_r1968664549
##########
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();
Review Comment:
I try to change the function as following and the test can pass without active controller check.
```java
@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));
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18844: Fix flaky QuorumControllerTest.testBalancePartitionLeaders() [kafka]
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on PR #18997:
URL: https://github.com/apache/kafka/pull/18997#issuecomment-2679867936
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).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18844: Stale features information in QuorumController#registerBroker [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on PR #18997:
URL: https://github.com/apache/kafka/pull/18997#issuecomment-2686275038
> 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18844: Fix flaky QuorumControllerTest.testBalancePartitionLeaders() [kafka]
Posted by "junrao (via GitHub)" <gi...@apache.org>.
junrao commented on code in PR #18997:
URL: https://github.com/apache/kafka/pull/18997#discussion_r1970274581
##########
metadata/src/main/java/org/apache/kafka/controller/QuorumController.java:
##########
@@ -2004,14 +2004,17 @@ 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),
+ () -> {
+ // Populate finalized features map with latest known kraft version for validation.
+ // Get the finalized features map in controller operation to avoid outdated features.
Review Comment:
in controller operation => in the controller event handling thread
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18844: Fix flaky QuorumControllerTest.testBalancePartitionLeaders() [kafka]
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on PR #18997:
URL: https://github.com/apache/kafka/pull/18997#issuecomment-2677101084
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18844: Fix flaky QuorumControllerTest.testBalancePartitionLeaders() [kafka]
Posted by "junrao (via GitHub)" <gi...@apache.org>.
junrao commented on code in PR #18997:
URL: https://github.com/apache/kafka/pull/18997#discussion_r1966296852
##########
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(),
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18844: Fix flaky QuorumControllerTest.testBalancePartitionLeaders() [kafka]
Posted by "junrao (via GitHub)" <gi...@apache.org>.
junrao commented on PR #18997:
URL: https://github.com/apache/kafka/pull/18997#issuecomment-2679825519
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18844: Stale features information in QuorumController#registerBroker [kafka]
Posted by "junrao (via GitHub)" <gi...@apache.org>.
junrao commented on PR #18997:
URL: https://github.com/apache/kafka/pull/18997#issuecomment-2688642424
@jsancio : I merged the PR to trunk. Could you help cherry-pick it to 4.0 and 3.9?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18844: Stale features information in QuorumController#registerBroker [kafka]
Posted by "FrankYang0529 (via GitHub)" <gi...@apache.org>.
FrankYang0529 commented on PR #18997:
URL: https://github.com/apache/kafka/pull/18997#issuecomment-2683668617
Hi @junrao, thanks for the review. Updated title and description on Jira and the PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18844: Fix flaky QuorumControllerTest.testBalancePartitionLeaders() [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on PR #18997:
URL: https://github.com/apache/kafka/pull/18997#issuecomment-2679890248
> 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.
1. remove `metadataVersion` from `FeatureControlManager` constructor, since the `TimelineObject<MetadataVersion>` will be changed to `TimelineObject<Optional<MetadataVersion>>`. And it is initialized with `Optional.empty`
2. `FeatureControlManager#metadataVersion` should be changed to `metadataVersionOrThrow`, similar to `FeaturesImage`
3. `CompleteActivationEvent` should NOT use `FeatureControlManager#metadataVersionOrThrow`if 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18844: Fix flaky QuorumControllerTest.testBalancePartitionLeaders() [kafka]
Posted by "junrao (via GitHub)" <gi...@apache.org>.
junrao commented on PR #18997:
URL: https://github.com/apache/kafka/pull/18997#issuecomment-2679965954
> 3. 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18844: Fix flaky QuorumControllerTest.testBalancePartitionLeaders() [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on PR #18997:
URL: https://github.com/apache/kafka/pull/18997#issuecomment-2679960595
> If above list is correct, maybe we can address it in follow-up?
open https://issues.apache.org/jira/browse/KAFKA-18858
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18844: Stale features information in QuorumController#registerBroker [kafka]
Posted by "junrao (via GitHub)" <gi...@apache.org>.
junrao commented on PR #18997:
URL: https://github.com/apache/kafka/pull/18997#issuecomment-2688594729
@FrankYang0529 : Thanks for the updated description. LGTM
@jsancio : +1 on cherry-pick this to 3.9 too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org