-
Notifications
You must be signed in to change notification settings - Fork 15.4k
KAFKA-13228; Ensure ApiVersionRequest is properly handled KRaft co-resident mode #11784
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 3 commits
8f795a4
0e76ad9
ac07a78
d4b9a04
c631dae
58af486
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 |
|---|---|---|
|
|
@@ -34,11 +34,14 @@ import kafka.server.KafkaRaftServer.ControllerRole | |
| import kafka.server.metadata.BrokerServerMetrics | ||
| import kafka.server.metadata.{BrokerMetadataListener, BrokerMetadataPublisher, BrokerMetadataSnapshotter, ClientQuotaMetadataManager, KRaftMetadataCache, SnapshotWriterBuilder} | ||
| import kafka.utils.{CoreUtils, KafkaScheduler} | ||
| import org.apache.kafka.clients.NodeApiVersions | ||
| import org.apache.kafka.common.feature.SupportedVersionRange | ||
| import org.apache.kafka.common.message.ApiMessageType.ListenerType | ||
| import org.apache.kafka.common.message.BrokerRegistrationRequestData.{Listener, ListenerCollection} | ||
| import org.apache.kafka.common.metrics.Metrics | ||
| import org.apache.kafka.common.network.ListenerName | ||
| import org.apache.kafka.common.protocol.ApiKeys | ||
| import org.apache.kafka.common.requests.ApiVersionsResponse | ||
| import org.apache.kafka.common.security.auth.SecurityProtocol | ||
| import org.apache.kafka.common.security.scram.internals.ScramMechanism | ||
| import org.apache.kafka.common.security.token.delegation.internals.DelegationTokenCache | ||
|
|
@@ -129,7 +132,7 @@ class BrokerServer( | |
|
|
||
| var forwardingManager: ForwardingManager = null | ||
|
|
||
| var alterIsrManager: AlterPartitionManager = null | ||
| var alterPartitionManager: AlterPartitionManager = null | ||
|
|
||
| var autoTopicCreationManager: AutoTopicCreationManager = null | ||
|
|
||
|
|
@@ -212,6 +215,11 @@ class BrokerServer( | |
|
|
||
| val controllerNodes = RaftConfig.voterConnectionsToNodes(controllerQuorumVotersFuture.get()).asScala | ||
| val controllerNodeProvider = RaftControllerNodeProvider(raftManager, config, controllerNodes) | ||
| val currentNodeControllerApiVersions = if (config.isKRaftCoResidentMode) { | ||
| Some(NodeApiVersions.create(ApiKeys.controllerApis().asScala.map(ApiVersionsResponse.toApiVersion).asJava)) | ||
| } else { | ||
| None | ||
|
Member
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. Unless I'm misreading the code, it seems like this was not None before. Are there implications for adding the None case?
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 |
||
| } | ||
|
|
||
| clientToControllerChannelManager = BrokerToControllerChannelManager( | ||
| controllerNodeProvider, | ||
|
|
@@ -220,7 +228,8 @@ class BrokerServer( | |
| config, | ||
| channelName = "forwarding", | ||
| threadNamePrefix, | ||
| retryTimeoutMs = 60000 | ||
| retryTimeoutMs = 60000, | ||
| currentNodeControllerApiVersions | ||
| ) | ||
| clientToControllerChannelManager.start() | ||
| forwardingManager = new ForwardingManagerImpl(clientToControllerChannelManager) | ||
|
|
@@ -248,17 +257,18 @@ class BrokerServer( | |
| config, | ||
| channelName = "alterIsr", | ||
| threadNamePrefix, | ||
| retryTimeoutMs = Long.MaxValue | ||
| retryTimeoutMs = Long.MaxValue, | ||
| currentNodeControllerApiVersions | ||
| ) | ||
| alterIsrManager = new DefaultAlterPartitionManager( | ||
| alterPartitionManager = new DefaultAlterPartitionManager( | ||
| controllerChannelManager = alterIsrChannelManager, | ||
| scheduler = kafkaScheduler, | ||
| time = time, | ||
| brokerId = config.nodeId, | ||
| brokerEpochSupplier = () => lifecycleManager.brokerEpoch, | ||
| metadataVersionSupplier = () => metadataCache.metadataVersion() | ||
| ) | ||
| alterIsrManager.start() | ||
| alterPartitionManager.start() | ||
|
|
||
| this._replicaManager = new ReplicaManager( | ||
| config = config, | ||
|
|
@@ -269,7 +279,7 @@ class BrokerServer( | |
| quotaManagers = quotaManagers, | ||
| metadataCache = metadataCache, | ||
| logDirFailureChannel = logDirFailureChannel, | ||
| alterPartitionManager = alterIsrManager, | ||
| alterPartitionManager = alterPartitionManager, | ||
| brokerTopicStats = brokerTopicStats, | ||
| isShuttingDown = isShuttingDown, | ||
| zkClient = None, | ||
|
|
@@ -343,10 +353,23 @@ class BrokerServer( | |
| k -> VersionRange.of(v.min, v.max) | ||
| }.asJava | ||
|
|
||
| lifecycleManager.start(() => metadataListener.highestMetadataOffset, | ||
| BrokerToControllerChannelManager(controllerNodeProvider, time, metrics, config, | ||
| "heartbeat", threadNamePrefix, config.brokerSessionTimeoutMs.toLong), | ||
| metaProps.clusterId, networkListeners, featuresRemapped) | ||
| val brokerLifecycleChannelManager = BrokerToControllerChannelManager( | ||
| controllerNodeProvider, | ||
| time, | ||
| metrics, | ||
| config, | ||
| "heartbeat", | ||
| threadNamePrefix, | ||
| config.brokerSessionTimeoutMs.toLong, | ||
| currentNodeControllerApiVersions | ||
| ) | ||
| lifecycleManager.start( | ||
| () => metadataListener.highestMetadataOffset, | ||
| brokerLifecycleChannelManager, | ||
| metaProps.clusterId, | ||
| networkListeners, | ||
| featuresRemapped | ||
| ) | ||
|
|
||
| // Register a listener with the Raft layer to receive metadata event notifications | ||
| raftManager.register(metadataListener) | ||
|
|
@@ -544,8 +567,8 @@ class BrokerServer( | |
| if (replicaManager != null) | ||
| CoreUtils.swallow(replicaManager.shutdown(), this) | ||
|
|
||
| if (alterIsrManager != null) | ||
| CoreUtils.swallow(alterIsrManager.shutdown(), this) | ||
| if (alterPartitionManager != null) | ||
| CoreUtils.swallow(alterPartitionManager.shutdown(), this) | ||
|
|
||
| if (clientToControllerChannelManager != null) | ||
| CoreUtils.swallow(clientToControllerChannelManager.shutdown(), this) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -122,7 +122,8 @@ object BrokerToControllerChannelManager { | |
| config: KafkaConfig, | ||
| channelName: String, | ||
| threadNamePrefix: Option[String], | ||
| retryTimeoutMs: Long | ||
| retryTimeoutMs: Long, | ||
| currentNodeControllerApiVersions: Option[NodeApiVersions] | ||
| ): BrokerToControllerChannelManager = { | ||
| new BrokerToControllerChannelManagerImpl( | ||
| controllerNodeProvider, | ||
|
|
@@ -131,7 +132,8 @@ object BrokerToControllerChannelManager { | |
| config, | ||
| channelName, | ||
| threadNamePrefix, | ||
| retryTimeoutMs | ||
| retryTimeoutMs, | ||
| currentNodeControllerApiVersions | ||
| ) | ||
| } | ||
| } | ||
|
|
@@ -160,12 +162,12 @@ class BrokerToControllerChannelManagerImpl( | |
| config: KafkaConfig, | ||
| channelName: String, | ||
| threadNamePrefix: Option[String], | ||
| retryTimeoutMs: Long | ||
| retryTimeoutMs: Long, | ||
| currentNodeControllerApiVersions: Option[NodeApiVersions] | ||
| ) extends BrokerToControllerChannelManager with Logging { | ||
| private val logContext = new LogContext(s"[BrokerToControllerChannelManager broker=${config.brokerId} name=$channelName] ") | ||
| private val manualMetadataUpdater = new ManualMetadataUpdater() | ||
| private val apiVersions = new ApiVersions() | ||
| private val currentNodeApiVersions = NodeApiVersions.create() | ||
|
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. This is the source of the bug, we always assume currentNodeApiVersions= |
||
| private val requestThread = newRequestThread | ||
|
|
||
| def start(): Unit = { | ||
|
|
@@ -254,7 +256,7 @@ class BrokerToControllerChannelManagerImpl( | |
| def controllerApiVersions(): Option[NodeApiVersions] = { | ||
| requestThread.activeControllerAddress().flatMap { activeController => | ||
| if (activeController.id == config.brokerId) | ||
| Some(currentNodeApiVersions) | ||
| currentNodeControllerApiVersions | ||
|
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. I wonder if this optimization is worth preserving. Any harm using the same negotiation even when the controller is co-resident?
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. I didn't find any harm in using the same negotiation so I removed this optimization here. Yet we should notice one thing here, the
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. If I understand correctly, you are saying that the range of supported api versions may change while a client is still connected to the broker. In general it's only a problem if the range of supported versions is reduced (e.g. in a downgrade). The way the code is supposed to handle this is by catching the unsupported version in the Envelope response and disconnecting the client in order to ensure that it reconnects and gets the latest range of api version support. Check
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. Yes, there may be a similar problem with finalized features since they will also change, I will check |
||
| else | ||
| Option(apiVersions.get(activeController.idString)) | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,14 +34,14 @@ import kafka.security.CredentialProvider | |
| import kafka.server.metadata.{ZkConfigRepository, ZkMetadataCache} | ||
| import kafka.utils._ | ||
| import kafka.zk.{AdminZkClient, BrokerInfo, KafkaZkClient} | ||
| import org.apache.kafka.clients.{ApiVersions, ManualMetadataUpdater, NetworkClient, NetworkClientUtils} | ||
| import org.apache.kafka.clients.{ApiVersions, ManualMetadataUpdater, NetworkClient, NetworkClientUtils, NodeApiVersions} | ||
| import org.apache.kafka.common.internals.Topic | ||
| import org.apache.kafka.common.message.ApiMessageType.ListenerType | ||
| import org.apache.kafka.common.message.ControlledShutdownRequestData | ||
| import org.apache.kafka.common.metrics.Metrics | ||
| import org.apache.kafka.common.network._ | ||
| import org.apache.kafka.common.protocol.Errors | ||
| import org.apache.kafka.common.requests.{ControlledShutdownRequest, ControlledShutdownResponse} | ||
| import org.apache.kafka.common.protocol.{ApiKeys, Errors} | ||
| import org.apache.kafka.common.requests.{ApiVersionsResponse, ControlledShutdownRequest, ControlledShutdownResponse} | ||
| import org.apache.kafka.common.security.scram.internals.ScramMechanism | ||
| import org.apache.kafka.common.security.token.delegation.internals.DelegationTokenCache | ||
| import org.apache.kafka.common.security.{JaasContext, JaasUtils} | ||
|
|
@@ -140,7 +140,7 @@ class KafkaServer( | |
|
|
||
| var clientToControllerChannelManager: BrokerToControllerChannelManager = null | ||
|
|
||
| var alterIsrManager: AlterPartitionManager = null | ||
| var alterPartitionManager: AlterPartitionManager = null | ||
|
|
||
| var kafkaScheduler: KafkaScheduler = null | ||
|
|
||
|
|
@@ -228,6 +228,9 @@ class KafkaServer( | |
| logContext = new LogContext(s"[KafkaServer id=${config.brokerId}] ") | ||
| this.logIdent = logContext.logPrefix | ||
|
|
||
| val controllerNodeProvider = MetadataCacheControllerNodeProvider(config, metadataCache) | ||
| val currentNodeControllerApiVersions = Some(NodeApiVersions.create(ApiKeys.zkBrokerApis.asScala.map(ApiVersionsResponse.toApiVersion).asJava)) | ||
|
|
||
| // initialize dynamic broker configs from ZooKeeper. Any updates made after this will be | ||
| // applied after ZkConfigManager starts. | ||
| config.dynamicConfig.initialize(Some(zkClient)) | ||
|
|
@@ -276,13 +279,15 @@ class KafkaServer( | |
| credentialProvider = new CredentialProvider(ScramMechanism.mechanismNames, tokenCache) | ||
|
|
||
| clientToControllerChannelManager = BrokerToControllerChannelManager( | ||
| controllerNodeProvider = MetadataCacheControllerNodeProvider(config, metadataCache), | ||
| controllerNodeProvider = controllerNodeProvider, | ||
| time = time, | ||
| metrics = metrics, | ||
| config = config, | ||
| channelName = "forwarding", | ||
| threadNamePrefix = threadNamePrefix, | ||
| retryTimeoutMs = config.requestTimeoutMs.longValue) | ||
| retryTimeoutMs = config.requestTimeoutMs.longValue, | ||
| currentNodeControllerApiVersions | ||
| ) | ||
| clientToControllerChannelManager.start() | ||
|
|
||
| /* start forwarding manager */ | ||
|
|
@@ -309,20 +314,29 @@ class KafkaServer( | |
| socketServer = new SocketServer(config, metrics, time, credentialProvider, apiVersionManager) | ||
|
|
||
| // Start alter partition manager based on the IBP version | ||
| alterIsrManager = if (config.interBrokerProtocolVersion.isAlterPartitionSupported) { | ||
| alterPartitionManager = if (config.interBrokerProtocolVersion.isAlterPartitionSupported) { | ||
| val alterPartitionChannelManager = BrokerToControllerChannelManager( | ||
|
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. nit: I think I favor creating the
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. I guess we create
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. Consolidation is a little difficult because of head-of-line blocking (sigh). It might be reasonable to combine the alter partition and broker lifecycle channel managers though. Anyway, this is definitely a separate patch. |
||
| controllerNodeProvider, | ||
| time = time, | ||
| metrics = metrics, | ||
| config = config, | ||
| channelName = "alterIsr", | ||
| threadNamePrefix = threadNamePrefix, | ||
| retryTimeoutMs = Long.MaxValue, | ||
| currentNodeControllerApiVersions | ||
| ) | ||
| AlterPartitionManager( | ||
| config = config, | ||
| metadataCache = metadataCache, | ||
| alterPartitionChannelManager, | ||
| scheduler = kafkaScheduler, | ||
| time = time, | ||
| metrics = metrics, | ||
| threadNamePrefix = threadNamePrefix, | ||
| brokerEpochSupplier = () => kafkaController.brokerEpoch | ||
| ) | ||
| } else { | ||
| AlterPartitionManager(kafkaScheduler, time, zkClient) | ||
| } | ||
| alterIsrManager.start() | ||
| alterPartitionManager.start() | ||
|
|
||
| // Start replica manager | ||
| _replicaManager = createReplicaManager(isShuttingDown) | ||
|
|
@@ -478,7 +492,7 @@ class KafkaServer( | |
| quotaManagers = quotaManagers, | ||
| metadataCache = metadataCache, | ||
| logDirFailureChannel = logDirFailureChannel, | ||
| alterPartitionManager = alterIsrManager, | ||
| alterPartitionManager = alterPartitionManager, | ||
| brokerTopicStats = brokerTopicStats, | ||
| isShuttingDown = isShuttingDown, | ||
| zkClient = Some(zkClient), | ||
|
|
@@ -755,8 +769,8 @@ class KafkaServer( | |
| if (replicaManager != null) | ||
| CoreUtils.swallow(replicaManager.shutdown(), this) | ||
|
|
||
| if (alterIsrManager != null) | ||
| CoreUtils.swallow(alterIsrManager.shutdown(), this) | ||
| if (alterPartitionManager != null) | ||
| CoreUtils.swallow(alterPartitionManager.shutdown(), this) | ||
|
|
||
| if (clientToControllerChannelManager != null) | ||
| CoreUtils.swallow(clientToControllerChannelManager.shutdown(), this) | ||
|
|
||
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.
nit: I think
alterPartitionis redundant here. Could we call itchannelManager?