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/01/24 04:27:36 UTC
[PR] KAFKA-18602: Incorrect FinalizedVersionLevel reported for dynamic KRaft quorum (wip) [kafka]
FrankYang0529 opened a new pull request, #18685:
URL: https://github.com/apache/kafka/pull/18685
### 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-18602: Incorrect FinalizedVersionLevel reported for dynamic KRaft quorum [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #18685:
URL: https://github.com/apache/kafka/pull/18685#discussion_r1985095404
##########
core/src/main/scala/kafka/server/metadata/KRaftMetadataCache.scala:
##########
@@ -522,10 +522,14 @@ class KRaftMetadataCache(
if (kraftVersionLevel > 0) {
finalizedFeatures.put(KRaftVersion.FEATURE_NAME, kraftVersionLevel)
}
+ var metadataVersion = MetadataVersion.MINIMUM_VERSION
+ if (!image.features().metadataVersion().isEmpty) {
+ metadataVersion = image.features().metadataVersionOrThrow()
+ }
new FinalizedFeatures(
- image.features().metadataVersionOrThrow(),
+ metadataVersion,
finalizedFeatures,
- image.highestOffsetAndEpoch().offset)
+ image.highestOffsetAndEpoch().epoch())
Review Comment:
> Within the same KRaft leader's life time (i.e. the same epoch), multiple rounds of feature updates could have happened. It would be incorrect to have the same FinalizedFeaturesEpoch after each round of update.
@junrao Excuse me, the `offset` keeps increasing due to `NoOpRecord`, and this offset is returned to Admin via `FeatureMetadata#finalizedFeaturesEpoch`, which is used by the feature command. This means users see an increased `finalizedFeaturesEpoch` even though no feature has changed. Is this expected?
--
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-18602: Incorrect FinalizedVersionLevel reported for dynamic KRaft quorum [kafka]
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on code in PR #18685:
URL: https://github.com/apache/kafka/pull/18685#discussion_r1985518686
##########
core/src/main/scala/kafka/server/metadata/KRaftMetadataCache.scala:
##########
@@ -522,10 +522,14 @@ class KRaftMetadataCache(
if (kraftVersionLevel > 0) {
finalizedFeatures.put(KRaftVersion.FEATURE_NAME, kraftVersionLevel)
}
+ var metadataVersion = MetadataVersion.MINIMUM_VERSION
Review Comment:
`stillNeedToCatchUp` can return false and that would cause `initializeNewPublishers` not to initialize all publishers.
--
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-18602: Incorrect FinalizedVersionLevel reported for dynamic KRaft quorum [kafka]
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on code in PR #18685:
URL: https://github.com/apache/kafka/pull/18685#discussion_r1983816858
##########
core/src/main/scala/kafka/server/metadata/KRaftMetadataCache.scala:
##########
@@ -522,10 +522,14 @@ class KRaftMetadataCache(
if (kraftVersionLevel > 0) {
finalizedFeatures.put(KRaftVersion.FEATURE_NAME, kraftVersionLevel)
}
+ var metadataVersion = MetadataVersion.MINIMUM_VERSION
Review Comment:
This change means that we can potentially return the wrong metadata version though.
--
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-18602: Incorrect FinalizedVersionLevel reported for dynamic KRaft quorum [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #18685:
URL: https://github.com/apache/kafka/pull/18685#discussion_r1985530280
##########
core/src/main/scala/kafka/server/metadata/KRaftMetadataCache.scala:
##########
@@ -522,10 +522,14 @@ class KRaftMetadataCache(
if (kraftVersionLevel > 0) {
finalizedFeatures.put(KRaftVersion.FEATURE_NAME, kraftVersionLevel)
}
+ var metadataVersion = MetadataVersion.MINIMUM_VERSION
+ if (!image.features().metadataVersion().isEmpty) {
+ metadataVersion = image.features().metadataVersionOrThrow()
+ }
new FinalizedFeatures(
- image.features().metadataVersionOrThrow(),
+ metadataVersion,
finalizedFeatures,
- image.highestOffsetAndEpoch().offset)
+ image.highestOffsetAndEpoch().epoch())
Review Comment:
@junrao Thank you for your explanation. The following question may not be a real issue in Kafka, and I'm simply trying to learn more about the design.
> In the client, the way that FinalizedFeaturesEpoch is used is to decide which finalized features returned in ApiResponse is the latest.
The `FinalizedFeaturesEpoch` keeps increasing over time (due to `NoOpRecord`), so how do we use it to determine the "latest" ApiResponse, since the value is always greater than the previous ApiResponse?
For example: Clients may periodically update the `ApiResponse` and compare the latest `FinalizedFeaturesEpoch` with their local cached `FinalizedFeaturesEpoch`. If the latest `FinalizedFeaturesEpoch` is greater than the local cached `FinalizedFeaturesEpoch`, clients initiate local feature changes. However, since the latest `FinalizedFeaturesEpoch` is always greater than the local cached `FinalizedFeaturesEpoch`, checking it becomes unnecessary.
--
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-18602: Incorrect FinalizedVersionLevel reported for dynamic KRaft quorum [kafka]
Posted by "FrankYang0529 (via GitHub)" <gi...@apache.org>.
FrankYang0529 commented on PR #18685:
URL: https://github.com/apache/kafka/pull/18685#issuecomment-2794262999
Close the PR since the issue is fixed by https://github.com/apache/kafka/pull/19127.
--
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-18602: Incorrect FinalizedVersionLevel reported for dynamic KRaft quorum [kafka]
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on code in PR #18685:
URL: https://github.com/apache/kafka/pull/18685#discussion_r1983488901
##########
core/src/main/scala/kafka/server/metadata/KRaftMetadataCache.scala:
##########
@@ -522,10 +522,14 @@ class KRaftMetadataCache(
if (kraftVersionLevel > 0) {
finalizedFeatures.put(KRaftVersion.FEATURE_NAME, kraftVersionLevel)
}
+ var metadataVersion = MetadataVersion.MINIMUM_VERSION
Review Comment:
Why is this needed?
--
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-18602: Incorrect FinalizedVersionLevel reported for dynamic KRaft quorum [kafka]
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on code in PR #18685:
URL: https://github.com/apache/kafka/pull/18685#discussion_r1985515760
##########
core/src/main/scala/kafka/server/metadata/KRaftMetadataCache.scala:
##########
@@ -522,10 +522,14 @@ class KRaftMetadataCache(
if (kraftVersionLevel > 0) {
finalizedFeatures.put(KRaftVersion.FEATURE_NAME, kraftVersionLevel)
}
+ var metadataVersion = MetadataVersion.MINIMUM_VERSION
Review Comment:
I think `uninitializedPublishers.isEmpty()` must return true for initialization to be complete.
--
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-18602: Incorrect FinalizedVersionLevel reported for dynamic KRaft quorum [kafka]
Posted by "junrao (via GitHub)" <gi...@apache.org>.
junrao commented on code in PR #18685:
URL: https://github.com/apache/kafka/pull/18685#discussion_r1985613298
##########
core/src/main/scala/kafka/server/metadata/KRaftMetadataCache.scala:
##########
@@ -522,10 +522,14 @@ class KRaftMetadataCache(
if (kraftVersionLevel > 0) {
finalizedFeatures.put(KRaftVersion.FEATURE_NAME, kraftVersionLevel)
}
+ var metadataVersion = MetadataVersion.MINIMUM_VERSION
+ if (!image.features().metadataVersion().isEmpty) {
+ metadataVersion = image.features().metadataVersionOrThrow()
+ }
new FinalizedFeatures(
- image.features().metadataVersionOrThrow(),
+ metadataVersion,
finalizedFeatures,
- image.highestOffsetAndEpoch().offset)
+ image.highestOffsetAndEpoch().epoch())
Review Comment:
A client could talk to multiple brokers. Since the metadata is propagated to broker asynchronously, we don't want a client to see finalized feature levels going backward. Here is an example of on FinalizedFeaturesEpoch is used. If FinalizedFeaturesEpoch increases, it will update the corresponding finalized feature level. If the level doesn't actually change, we performed an unnecessary update, but it's cheap. We could implement FinalizedFeaturesEpoch that actually reflects the feature updates. We will need to store it in the metadata log somewhere.
```
public synchronized void maybeUpdateTransactionV2Enabled(boolean onInitiatialization) {
if (latestFinalizedFeaturesEpoch >= apiVersions.getMaxFinalizedFeaturesEpoch()) {
return;
}
ApiVersions.FinalizedFeaturesInfo info = apiVersions.getFinalizedFeaturesInfo();
latestFinalizedFeaturesEpoch = info.finalizedFeaturesEpoch;
Short transactionVersion = info.finalizedFeatures.get("transaction.version");
boolean wasTransactionV2Enabled = isTransactionV2Enabled;
isTransactionV2Enabled = transactionVersion != null && transactionVersion >= 2;
log.debug("Updating isTV2 enabled to {} with FinalizedFeaturesEpoch {}", isTransactionV2Enabled, latestFinalizedFeaturesEpoch);
if (!onInitiatialization && !wasTransactionV2Enabled && isTransactionV2Enabled)
clientSideEpochBumpRequired = true;
}
```
--
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-18602: Incorrect FinalizedVersionLevel reported for dynamic KRaft quorum [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on PR #18685:
URL: https://github.com/apache/kafka/pull/18685#issuecomment-2703926607
> With this fix, I observed that feature epoch keeps increasing. In trunk branch, if a cluster is stable, its feature epoch doesn't increase.
I believe that's a bug, as we're using `offset` instead of `epoch` ....
```
new FinalizedFeatures(
image.features().metadataVersionOrThrow(),
finalizedFeatures,
image.highestOffsetAndEpoch().offset)
```
https://github.com/apache/kafka/blob/899cdb598a6422b3bd7ee270d344dea574d20c99/core/src/main/scala/kafka/server/metadata/KRaftMetadataCache.scala#L528
@FrankYang0529 could you please fix it in this PR as well?
--
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-18602: Incorrect FinalizedVersionLevel reported for dynamic KRaft quorum [kafka]
Posted by "junrao (via GitHub)" <gi...@apache.org>.
junrao commented on code in PR #18685:
URL: https://github.com/apache/kafka/pull/18685#discussion_r1985471978
##########
core/src/main/scala/kafka/server/metadata/KRaftMetadataCache.scala:
##########
@@ -522,10 +522,14 @@ class KRaftMetadataCache(
if (kraftVersionLevel > 0) {
finalizedFeatures.put(KRaftVersion.FEATURE_NAME, kraftVersionLevel)
}
+ var metadataVersion = MetadataVersion.MINIMUM_VERSION
+ if (!image.features().metadataVersion().isEmpty) {
+ metadataVersion = image.features().metadataVersionOrThrow()
+ }
new FinalizedFeatures(
- image.features().metadataVersionOrThrow(),
+ metadataVersion,
finalizedFeatures,
- image.highestOffsetAndEpoch().offset)
+ image.highestOffsetAndEpoch().epoch())
Review Comment:
@chia7712 : The description of FinalizedFeaturesEpoch is the following.
`The monotonically increasing epoch for the finalized features information`
In the client, the way that FinalizedFeaturesEpoch is used is to decide which finalized features returned in ApiResponse is the latest. So, if a feature is updated, FinalizedFeaturesEpoch needs to go up. The reverse doesn't need to be true, i.e. if FinalizedFeaturesEpoch goes up, it doesn't necessarily need to have a corresponding feature update.
In the ZK implementation, the reverse happens to be true. In the KRaft implementation, the reverse is not true, but this is ok.
--
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-18602: Incorrect FinalizedVersionLevel reported for dynamic KRaft quorum [kafka]
Posted by "junrao (via GitHub)" <gi...@apache.org>.
junrao commented on code in PR #18685:
URL: https://github.com/apache/kafka/pull/18685#discussion_r1985500986
##########
core/src/main/scala/kafka/server/metadata/KRaftMetadataCache.scala:
##########
@@ -522,10 +522,14 @@ class KRaftMetadataCache(
if (kraftVersionLevel > 0) {
finalizedFeatures.put(KRaftVersion.FEATURE_NAME, kraftVersionLevel)
}
+ var metadataVersion = MetadataVersion.MINIMUM_VERSION
Review Comment:
@FrankYang0529 : Good finding! It seems that we should move `future.complete(null); ` to after `initializeNewPublishers();` in the code below. What do you think, @cmccabe?
```
void scheduleInitializeNewPublishers(long delayNs) {
eventQueue.scheduleDeferred(INITIALIZE_NEW_PUBLISHERS,
new EventQueue.EarliestDeadlineFunction(eventQueue.time().nanoseconds() + delayNs),
() -> {
try {
initializeNewPublishers();
} catch (Throwable e) {
faultHandler.handleFault("Unhandled error initializing new publishers", e);
}
});
}
```
--
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-18602: Incorrect FinalizedVersionLevel reported for dynamic KRaft quorum [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #18685:
URL: https://github.com/apache/kafka/pull/18685#discussion_r1986118137
##########
core/src/main/scala/kafka/server/metadata/KRaftMetadataCache.scala:
##########
@@ -522,10 +522,14 @@ class KRaftMetadataCache(
if (kraftVersionLevel > 0) {
finalizedFeatures.put(KRaftVersion.FEATURE_NAME, kraftVersionLevel)
}
+ var metadataVersion = MetadataVersion.MINIMUM_VERSION
Review Comment:
That seems like a deadlock to me. If a controller can't receive RPC requests before having a valid metadata version, it can't communicate with other static voters initially. This means it's impossible to obtain a valid metadata version.
The approach in #19127 doesn't completely fix the issue, but the issue occurs at the start of static votes. During that phase, the quorum isn't ready, so exposing the 3.3 MV via the API response should be low risk.
To clean up, if the above description is accurate, `SimpleApiVersionManager` can replace `featuresPublisher` with `MetadataCache` and manually add the 3.3 MV to the API response when `MetadataCache` lacks a valid MV. That is basically equal to what #19127 does.
--
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-18602: Incorrect FinalizedVersionLevel reported for dynamic KRaft quorum [kafka]
Posted by "cmccabe (via GitHub)" <gi...@apache.org>.
cmccabe commented on code in PR #18685:
URL: https://github.com/apache/kafka/pull/18685#discussion_r1983884807
##########
core/src/main/scala/kafka/server/metadata/KRaftMetadataCache.scala:
##########
@@ -522,10 +522,14 @@ class KRaftMetadataCache(
if (kraftVersionLevel > 0) {
finalizedFeatures.put(KRaftVersion.FEATURE_NAME, kraftVersionLevel)
}
+ var metadataVersion = MetadataVersion.MINIMUM_VERSION
Review Comment:
Specifically this code blocks until the initial metadata publish is done:
```
// Install all metadata publishers.
FutureUtils.waitWithLogging(logger.underlying, logIdent,
"the controller metadata publishers to be installed",
sharedServer.loader.installPublishers(metadataPublishers), startupDeadline, time)
```
after that we should have a valid metadata version in the metadata cache. If we don't we should figure out that bug.
--
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-18602: Incorrect FinalizedVersionLevel reported for dynamic KRaft quorum [kafka]
Posted by "FrankYang0529 (via GitHub)" <gi...@apache.org>.
FrankYang0529 commented on code in PR #18685:
URL: https://github.com/apache/kafka/pull/18685#discussion_r1984276852
##########
core/src/main/scala/kafka/server/metadata/KRaftMetadataCache.scala:
##########
@@ -522,10 +522,14 @@ class KRaftMetadataCache(
if (kraftVersionLevel > 0) {
finalizedFeatures.put(KRaftVersion.FEATURE_NAME, kraftVersionLevel)
}
+ var metadataVersion = MetadataVersion.MINIMUM_VERSION
Review Comment:
@cmccabe If we don't add `MINIMUM_VERSION` here, we get failed test case like `ReconfigurableQuorumIntegrationTest#testRemoveController`. Failed cases in this https://github.com/apache/kafka/actions/runs/13694313653/job/38293530484 are related to this.
I got error message like
```
[2025-03-07 09:38:36,355] WARN [RaftManager id=3001] Received error UNKNOWN_SERVER_ERROR from node 3002 when making an ApiVersionsRequest with correlation id 916. Disconnecting. (org.apache.kafka.clients.NetworkClient:1028)
[2025-03-07 09:38:36,436] ERROR [ControllerApis nodeId=3001] Unexpected error handling request RequestHeader(apiKey=API_VERSIONS, apiVersion=4, clientId=raft-client-3000, correlationId=872, headerVersion=2) -- ApiVersionsRequestData(clientSoftwareName='apache-kafka-java', clientSoftwareVersion='4.1.0-SNAPSHOT') with context RequestContext(header=RequestHeader(apiKey=API_VERSIONS, apiVersion=4, clientId=raft-client-3000, correlationId=872, headerVersion=2), connectionId='127.0.0.1:59302-127.0.0.1:60038-1-81', clientAddress=/127.0.0.1, principal=User:ANONYMOUS, listenerName=ListenerName(CONTROLLER), securityProtocol=PLAINTEXT, clientInformation=ClientInformation(softwareName=unknown, softwareVersion=unknown), fromPrivilegedListener=false, principalSerde=Optional[org.apache.kafka.common.security.authenticator.DefaultKafkaPrincipalBuilder@5d65541f]) (kafka.server.ControllerApis:79)
java.lang.IllegalStateException: Unknown metadata version for FeaturesImage: metadataVersion(Optional.empty), zkMigrationState()
at org.apache.kafka.image.FeaturesImage.lambda$metadataVersionOrThrow$0(FeaturesImage.java:73) ~[kafka-metadata-4.1.0-SNAPSHOT.jar:?]
at java.base/java.util.Optional.orElseThrow(Optional.java:403) ~[?:?]
at org.apache.kafka.image.FeaturesImage.metadataVersionOrThrow(FeaturesImage.java:72) ~[kafka-metadata-4.1.0-SNAPSHOT.jar:?]
at kafka.server.metadata.KRaftMetadataCache.features(KRaftMetadataCache.scala:530) ~[main/:?]
at kafka.server.ControllerServer.$anonfun$startup$8(ControllerServer.scala:152) ~[main/:?]
at kafka.server.SimpleApiVersionManager.features(ApiVersionManager.scala:115) ~[main/:?]
at kafka.server.SimpleApiVersionManager.apiVersionResponse(ApiVersionManager.scala:103) ~[main/:?]
at kafka.server.ControllerApis.$anonfun$handleApiVersionsRequest$3(ControllerApis.scala:464) ~[main/:?]
at kafka.server.ControllerApis.$anonfun$handleApiVersionsRequest$3$adapted(ControllerApis.scala:464) ~[main/:?]
at kafka.server.RequestHandlerHelper.sendResponseMaybeThrottle(RequestHandlerHelper.scala:99) ~[main/:?]
at kafka.server.ControllerApis.handleApiVersionsRequest(ControllerApis.scala:464) ~[main/:?]
at kafka.server.ControllerApis.handle(ControllerApis.scala:100) ~[main/:?]
at kafka.server.KafkaRequestHandler.run(KafkaRequestHandler.scala:158) ~[main/:?]
at java.base/java.lang.Thread.run(Thread.java:1583) [?:?]
```
--
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-18602: Incorrect FinalizedVersionLevel reported for dynamic KRaft quorum [kafka]
Posted by "FrankYang0529 (via GitHub)" <gi...@apache.org>.
FrankYang0529 commented on PR #18685:
URL: https://github.com/apache/kafka/pull/18685#issuecomment-2705314828
> It looks like we both created PRs around the same time to fix this bug. Mine is here : #19127 .
If it's ok to remove `FeaturesPublisher`, I think we can keep using this PR. I have add your test case `KRaftClusterTest#testDescribeKRaftVersion` to this PR and add you as co-author.
> While I understand why you did it, moving kraft.version out of BootstrapMetadata in tests feels messy to me. I think we should leave this alone for now and think about it more in the future. Perhaps surfacing the initial raft state in BootstrapMetadata.java is possible. That would be better.
Agree, I revert the commit. I will create a Jira for followup. 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-18602: Incorrect FinalizedVersionLevel reported for dynamic KRaft quorum [kafka]
Posted by "FrankYang0529 (via GitHub)" <gi...@apache.org>.
FrankYang0529 commented on code in PR #18685:
URL: https://github.com/apache/kafka/pull/18685#discussion_r1983531986
##########
core/src/main/scala/kafka/server/metadata/KRaftMetadataCache.scala:
##########
@@ -522,10 +522,14 @@ class KRaftMetadataCache(
if (kraftVersionLevel > 0) {
finalizedFeatures.put(KRaftVersion.FEATURE_NAME, kraftVersionLevel)
}
+ var metadataVersion = MetadataVersion.MINIMUM_VERSION
Review Comment:
Without this, test cases with more than 1 controller cannot start. In `FeaturesPublisher`, it still uses `MINIMUM_VERSION`, so it can start cluster without error.
https://github.com/apache/kafka/blob/899cdb598a6422b3bd7ee270d344dea574d20c99/metadata/src/main/java/org/apache/kafka/metadata/publisher/FeaturesPublisher.java#L34
--
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-18602: Incorrect FinalizedVersionLevel reported for dynamic KRaft quorum [kafka]
Posted by "junrao (via GitHub)" <gi...@apache.org>.
junrao commented on code in PR #18685:
URL: https://github.com/apache/kafka/pull/18685#discussion_r1983832557
##########
core/src/main/scala/kafka/server/metadata/KRaftMetadataCache.scala:
##########
@@ -522,10 +522,14 @@ class KRaftMetadataCache(
if (kraftVersionLevel > 0) {
finalizedFeatures.put(KRaftVersion.FEATURE_NAME, kraftVersionLevel)
}
+ var metadataVersion = MetadataVersion.MINIMUM_VERSION
Review Comment:
Hmm, is this because the KRaft client needs to serve ApiVersionRequest during bootstrap? MV won't be ready during bootstrap, but ApiVersionResponse needs to include finalized features that depend on MV being available.
##########
core/src/main/scala/kafka/server/metadata/KRaftMetadataCache.scala:
##########
@@ -522,10 +522,14 @@ class KRaftMetadataCache(
if (kraftVersionLevel > 0) {
finalizedFeatures.put(KRaftVersion.FEATURE_NAME, kraftVersionLevel)
}
+ var metadataVersion = MetadataVersion.MINIMUM_VERSION
+ if (!image.features().metadataVersion().isEmpty) {
+ metadataVersion = image.features().metadataVersionOrThrow()
+ }
new FinalizedFeatures(
- image.features().metadataVersionOrThrow(),
+ metadataVersion,
finalizedFeatures,
- image.highestOffsetAndEpoch().offset)
+ image.highestOffsetAndEpoch().epoch())
Review Comment:
Hmm, this doesn't seem correct. Within the same KRaft leader's life time (i.e. the same epoch), multiple rounds of feature updates could have happened. It would be incorrect to have the same FinalizedFeaturesEpoch after each round of update.
--
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-18602: Incorrect FinalizedVersionLevel reported for dynamic KRaft quorum [kafka]
Posted by "cmccabe (via GitHub)" <gi...@apache.org>.
cmccabe commented on PR #18685:
URL: https://github.com/apache/kafka/pull/18685#issuecomment-2707182871
@ijuma @junrao @chia7712 @FrankYang0529
if you want a minimal change that fixes this, with a test, there is my PR: https://github.com/apache/kafka/pull/19127
Since this a blocker for 4.0, maybe we should review #19127 and do the refactoring discussed in the original PR outside the 4.0 release time frame
--
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-18602: Incorrect FinalizedVersionLevel reported for dynamic KRaft quorum [kafka]
Posted by "cmccabe (via GitHub)" <gi...@apache.org>.
cmccabe commented on PR #18685:
URL: https://github.com/apache/kafka/pull/18685#issuecomment-2704663862
It looks like we both created PRs around the same time to fix this bug. Mine is here : https://github.com/apache/kafka/pull/19127 .
Some thoughts:
- It is true that FeaturesPublisher is kind of redundant now with the metadata cache. The reason why it was this way is that originally the controller didn't create the metadata cache. I guess it's OK to remove FeaturesPublisher.
- While I understand why you did it, moving kraft.version out of BootstrapMetadata in tests feels messy to me. I think we should leave this alone for now and think about it more in the future. Perhaps surfacing the initial raft state in BootstrapMetadata.java is possible. That would be better.
--
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-18602: Incorrect FinalizedVersionLevel reported for dynamic KRaft quorum (wip) [kafka]
Posted by "FrankYang0529 (via GitHub)" <gi...@apache.org>.
FrankYang0529 commented on PR #18685:
URL: https://github.com/apache/kafka/pull/18685#issuecomment-2703917342
> 1. With this fix, I observed that feature epoch keeps increasing. In trunk branch, if a cluster is stable, its feature epoch doesn't increase.
I found that we can fix this error by change this line to `image.highestOffsetAndEpoch().epoch()`. Is it ok to update this?
https://github.com/apache/kafka/blob/899cdb598a6422b3bd7ee270d344dea574d20c99/core/src/main/scala/kafka/server/metadata/KRaftMetadataCache.scala#L528
--
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-18602: Incorrect FinalizedVersionLevel reported for dynamic KRaft quorum [kafka]
Posted by "FrankYang0529 (via GitHub)" <gi...@apache.org>.
FrankYang0529 commented on code in PR #18685:
URL: https://github.com/apache/kafka/pull/18685#discussion_r1985052075
##########
core/src/main/scala/kafka/server/metadata/KRaftMetadataCache.scala:
##########
@@ -522,10 +522,14 @@ class KRaftMetadataCache(
if (kraftVersionLevel > 0) {
finalizedFeatures.put(KRaftVersion.FEATURE_NAME, kraftVersionLevel)
}
+ var metadataVersion = MetadataVersion.MINIMUM_VERSION
Review Comment:
@cmccabe I think `MetadataLoader#installPublishers` doesn't really wait for publishers initialized, because it uses `scheduleInitializeNewPublishers` to schedule another task and complete future immediately.
https://github.com/apache/kafka/blob/269e8892ad26904c82a3bff7b7d7a81c47619e4a/metadata/src/main/java/org/apache/kafka/image/loader/MetadataLoader.java#L486-L492
--
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-18602: Incorrect FinalizedVersionLevel reported for dynamic KRaft quorum (wip) [kafka]
Posted by "jsancio (via GitHub)" <gi...@apache.org>.
jsancio commented on PR #18685:
URL: https://github.com/apache/kafka/pull/18685#issuecomment-2701811371
@FrankYang0529 is this still a WIP? I would like to get this fixed and included in the 4.0.0 release. cc @dajac @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-18602: Incorrect FinalizedVersionLevel reported for dynamic KRaft quorum [kafka]
Posted by "FrankYang0529 (via GitHub)" <gi...@apache.org>.
FrankYang0529 commented on code in PR #18685:
URL: https://github.com/apache/kafka/pull/18685#discussion_r1983535958
##########
core/src/main/scala/kafka/server/ControllerServer.scala:
##########
@@ -153,7 +153,7 @@ class ControllerServer(
val apiVersionManager = new SimpleApiVersionManager(
ListenerType.CONTROLLER,
config.unstableApiVersionsEnabled,
- () => featuresPublisher.features()
Review Comment:
Removed 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-18602: Incorrect FinalizedVersionLevel reported for dynamic KRaft quorum [kafka]
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on code in PR #18685:
URL: https://github.com/apache/kafka/pull/18685#discussion_r1985518686
##########
core/src/main/scala/kafka/server/metadata/KRaftMetadataCache.scala:
##########
@@ -522,10 +522,14 @@ class KRaftMetadataCache(
if (kraftVersionLevel > 0) {
finalizedFeatures.put(KRaftVersion.FEATURE_NAME, kraftVersionLevel)
}
+ var metadataVersion = MetadataVersion.MINIMUM_VERSION
Review Comment:
`stillNeedToCatchUp` can return false and that would cause `initializeNewPublishers` not to complete.
--
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-18602: Incorrect FinalizedVersionLevel reported for dynamic KRaft quorum [kafka]
Posted by "FrankYang0529 (via GitHub)" <gi...@apache.org>.
FrankYang0529 commented on code in PR #18685:
URL: https://github.com/apache/kafka/pull/18685#discussion_r1986040272
##########
core/src/main/scala/kafka/server/metadata/KRaftMetadataCache.scala:
##########
@@ -522,10 +522,14 @@ class KRaftMetadataCache(
if (kraftVersionLevel > 0) {
finalizedFeatures.put(KRaftVersion.FEATURE_NAME, kraftVersionLevel)
}
+ var metadataVersion = MetadataVersion.MINIMUM_VERSION
Review Comment:
@chia7712 @cmccabe @ijuma @junrao, I found that we cannot add the check of `uninitializedPublishers.isEmpty()` at the end of `ControllerServer#startup`. If users use static voter (`controller.quorum.voters`), the `NetworkClient` tries to send `ApiVersionsReqeust` to other voters [0]. After it receives error response, it disconnects the connection [1]. At the meantime, the `KafkaRaftClient` tries to send `VoteRequest` [2], but the request cannot be sent cause of disconnection. Finally, none of controller can be ready with empty `uninitializedPublishers`.
Do we want to use another Jira to track this issue? The original issue is resolved by #19127.
[0]
https://github.com/apache/kafka/blob/947c414a8c0d30223c725a91507b2d891d5851e2/clients/src/main/java/org/apache/kafka/clients/NetworkClient.java#L1087-L1089
[1]
https://github.com/apache/kafka/blob/947c414a8c0d30223c725a91507b2d891d5851e2/clients/src/main/java/org/apache/kafka/clients/NetworkClient.java#L1027-L1031
[2]
https://github.com/apache/kafka/blob/947c414a8c0d30223c725a91507b2d891d5851e2/raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java#L3161-L3164
--
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-18602: Incorrect FinalizedVersionLevel reported for dynamic KRaft quorum [kafka]
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on code in PR #18685:
URL: https://github.com/apache/kafka/pull/18685#discussion_r1983816858
##########
core/src/main/scala/kafka/server/metadata/KRaftMetadataCache.scala:
##########
@@ -522,10 +522,14 @@ class KRaftMetadataCache(
if (kraftVersionLevel > 0) {
finalizedFeatures.put(KRaftVersion.FEATURE_NAME, kraftVersionLevel)
}
+ var metadataVersion = MetadataVersion.MINIMUM_VERSION
Review Comment:
This change means that we can potentially return the wrong metadata version though. Right?
--
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-18602: Incorrect FinalizedVersionLevel reported for dynamic KRaft quorum (wip) [kafka]
Posted by "FrankYang0529 (via GitHub)" <gi...@apache.org>.
FrankYang0529 commented on PR #18685:
URL: https://github.com/apache/kafka/pull/18685#issuecomment-2703450996
@jsancio @MahsaSeifikar @ijuma Thanks for review. I put this PR in WIP cause of two reasons:
1. With this fix, I observed that feature epoch keeps increasing. In trunk branch, if a cluster is stable, its feature epoch doesn't increase.
2. Dynamic voter in test framework doesn't match real case. In `KafkaClusterTestKit` [0], it uses `BootstrapMetadata#featureLevel` [1] to check `kraft.version`. It goes through all `FeatureLevelRecord` and gets a value. In the real cluster, the `kraft.version` is stored in `KRaftVersionRecord`. I already addressed this in the second commit. I think `ReconfigurableQuorumIntegrationTest#testCreateAndDestroyReconfigurableCluster` can be used as the test case.
[0]
https://github.com/apache/kafka/blob/041d8019d6f73a31a591ffba22b0fdfc3e56a9c5/test-common/test-common-runtime/src/main/java/org/apache/kafka/common/test/KafkaClusterTestKit.java#L445-L457
[1]
https://github.com/apache/kafka/blob/041d8019d6f73a31a591ffba22b0fdfc3e56a9c5/metadata/src/main/java/org/apache/kafka/metadata/bootstrap/BootstrapMetadata.java#L128-L138
--
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-18602: Incorrect FinalizedVersionLevel reported for dynamic KRaft quorum [kafka]
Posted by "cmccabe (via GitHub)" <gi...@apache.org>.
cmccabe commented on code in PR #18685:
URL: https://github.com/apache/kafka/pull/18685#discussion_r1983883788
##########
core/src/main/scala/kafka/server/metadata/KRaftMetadataCache.scala:
##########
@@ -522,10 +522,14 @@ class KRaftMetadataCache(
if (kraftVersionLevel > 0) {
finalizedFeatures.put(KRaftVersion.FEATURE_NAME, kraftVersionLevel)
}
+ var metadataVersion = MetadataVersion.MINIMUM_VERSION
Review Comment:
We should be waiting for the initial metadata publish to succeed before enabling request processing on the socket server. What test cases fail when you refuse to open connections without knowing 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-18602: Incorrect FinalizedVersionLevel reported for dynamic KRaft quorum (wip) [kafka]
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on PR #18685:
URL: https://github.com/apache/kafka/pull/18685#issuecomment-2702191844
Let's add a test please.
--
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-18602: Incorrect FinalizedVersionLevel reported for dynamic KRaft quorum [kafka]
Posted by "FrankYang0529 (via GitHub)" <gi...@apache.org>.
FrankYang0529 closed pull request #18685: KAFKA-18602: Incorrect FinalizedVersionLevel reported for dynamic KRaft quorum
URL: https://github.com/apache/kafka/pull/18685
--
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-18602: Incorrect FinalizedVersionLevel reported for dynamic KRaft quorum [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #18685:
URL: https://github.com/apache/kafka/pull/18685#discussion_r1983423778
##########
core/src/main/scala/kafka/server/ControllerServer.scala:
##########
@@ -153,7 +153,7 @@ class ControllerServer(
val apiVersionManager = new SimpleApiVersionManager(
ListenerType.CONTROLLER,
config.unstableApiVersionsEnabled,
- () => featuresPublisher.features()
Review Comment:
If we decide to fetch features from cache rather than `featuresPublisher`, we can remove whole `featuresPublisher` directly.
--
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-18602: Incorrect FinalizedVersionLevel reported for dynamic KRaft quorum [kafka]
Posted by "FrankYang0529 (via GitHub)" <gi...@apache.org>.
FrankYang0529 commented on code in PR #18685:
URL: https://github.com/apache/kafka/pull/18685#discussion_r1984274154
##########
core/src/main/scala/kafka/server/metadata/KRaftMetadataCache.scala:
##########
@@ -522,10 +522,14 @@ class KRaftMetadataCache(
if (kraftVersionLevel > 0) {
finalizedFeatures.put(KRaftVersion.FEATURE_NAME, kraftVersionLevel)
}
+ var metadataVersion = MetadataVersion.MINIMUM_VERSION
+ if (!image.features().metadataVersion().isEmpty) {
+ metadataVersion = image.features().metadataVersionOrThrow()
+ }
new FinalizedFeatures(
- image.features().metadataVersionOrThrow(),
+ metadataVersion,
finalizedFeatures,
- image.highestOffsetAndEpoch().offset)
+ image.highestOffsetAndEpoch().epoch())
Review Comment:
Thanks for the suggestion. Reverted this commit.
--
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