-
Notifications
You must be signed in to change notification settings - Fork 15.4k
KAFKA-13935 Fix static usages of IBP in KRaft mode #12250
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 16 commits
e2e3ce3
dcac78c
f404e21
d62d722
49f34d0
b903301
d9f398f
e9fc686
9833a65
df839a3
24006b6
eeaeb61
eaf680d
015349f
f79528f
0145d53
aea22cc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -31,7 +31,6 @@ import org.apache.kafka.common.internals.Topic | |||||||
| import org.apache.kafka.image.{MetadataDelta, MetadataImage, TopicDelta, TopicsImage} | ||||||||
| import org.apache.kafka.metadata.authorizer.ClusterMetadataAuthorizer | ||||||||
| import org.apache.kafka.server.authorizer.Authorizer | ||||||||
| import org.apache.kafka.server.common.MetadataVersion | ||||||||
|
|
||||||||
| import scala.collection.mutable | ||||||||
|
|
||||||||
|
|
@@ -132,10 +131,7 @@ class BrokerMetadataPublisher(conf: KafkaConfig, | |||||||
| // Publish the new metadata image to the metadata cache. | ||||||||
| metadataCache.setImage(newImage) | ||||||||
|
|
||||||||
| val metadataVersionLogMsg = newImage.features().metadataVersion() match { | ||||||||
| case MetadataVersion.UNINITIALIZED => "un-initialized metadata.version" | ||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does broker startup have any guarantees with respect to the metadata version in the log? For example, does the controller require the latest metadata feature record to be consumed before a broker can be unfenced? I'm mainly trying to understand if we might revert to an older version and what the consequences of that would be.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The broker remains fenced until it catches up on the metadata log (up to the high watermark, I believe). The broker components that care about metadata (which is most of them) have some "initialize" or "startup" method called when we first publish metadata on a broker. This should mean that when a broker component is fully started, it will have the current metadata version of the cluster. If a metadata version upgrade happened while a broker was initializing, I think it would either be included in the initial MetadataImage, or it would be handled separately. Either way, components which use the metadata version need to cope with the version changing at runtime.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair enough. I guess I was considering if we need any protection from an uninitialized metadata version in the code. We had the UNINITIALIZED sentinel before, but now it looks like we would use the min KRaft version or IBP. I don't know if there are any consequences to relying on an old version. Writes to the metadata log will go through the controller. Replication is protected by fencing. How about forwarding?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The IBP is just used for bootstrapping a metadata version when upgrading from an older KRaft cluster (see my other comment). Broker and controller components will assume the minimum KRaft version until we finish loading the metadata log. Forwarding should be okay since we don't start accepting external connections until after we've caught up on metadata in the broker. kafka/core/src/main/scala/kafka/server/BrokerServer.scala Lines 450 to 452 in f79528f
|
||||||||
| case mv: MetadataVersion => s"metadata.version ${mv.featureLevel()}" | ||||||||
| } | ||||||||
| val metadataVersionLogMsg = s"metadata.version ${newImage.features().metadataVersion()}" | ||||||||
|
|
||||||||
| if (_firstPublish) { | ||||||||
| info(s"Publishing initial metadata at offset $highestOffsetAndEpoch with $metadataVersionLogMsg.") | ||||||||
|
|
||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are still a few places referencing this static value (Partition, GroupCoordinator, etc). Should we change them to use MetadataVersion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes we should fix those as well. I believe all the remaining usages are for versions prior to 3.0-IV1, so in KRaft mode we can pass a static version
3.0-IV1(regardless of the actual MetadataVersion). Setting the IBP statically to the minimum KRaft compatible version (when in KRaft mode) also guards against any incorrect/inadvertent future usages of config.interBrokerProtocolVersion.Basically, the PR grew pretty large/complex when I was trying to include the remaining conversions. WDYT about completing those in a separate PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mumrah : Thanks for the explanation. Covering that in a separate PR sounds good to me.