Skip to content

KAFKA-18353: Remove zk config control.plane.listener.name#18329

Merged
chia7712 merged 12 commits intoapache:trunkfrom
TaiJuWu:contol.plnae
Jan 8, 2025
Merged

KAFKA-18353: Remove zk config control.plane.listener.name#18329
chia7712 merged 12 commits intoapache:trunkfrom
TaiJuWu:contol.plnae

Conversation

@TaiJuWu
Copy link
Copy Markdown
Collaborator

@TaiJuWu TaiJuWu commented Dec 27, 2024

JIRA: https://issues.apache.org/jira/browse/KAFKA-18353

Committer Checklist (excluded from commit message)

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

@github-actions github-actions Bot added triage PRs from the community core Kafka Broker labels Dec 27, 2024
Comment thread core/src/main/scala/kafka/network/SocketServer.scala Outdated
Comment thread core/src/main/scala/kafka/server/KafkaConfig.scala
Comment thread docs/upgrade.html Outdated
@github-actions github-actions Bot removed the triage PRs from the community label Dec 29, 2024
Comment thread docs/upgrade.html Outdated
Copy link
Copy Markdown
Member

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TaiJuWu thanks for this cleanup. please take a look at two small comments

Comment thread core/src/test/scala/unit/kafka/server/SaslApiVersionsRequestTest.scala Outdated
Comment thread core/src/main/scala/kafka/network/SocketServer.scala Outdated
Comment thread docs/upgrade.html Outdated
</li>
<li>The function <code>onNewBatch</code> in <code>org.apache.kafka.clients.producer.Partitioner</code> class was removed.
</li>
<li>The <code>control.plane.listener.name</code> config was removed.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chia7712 My point is that a lot more than this config will be removed as part of ZK removal. Why are we listing this one specifically? As I stated elsewhere, we should have a note saying that ZK support has been removed with a link to all the implications as well as the migration details.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My point is that a lot more than this config will be removed as part of ZK removal. Why are we listing this one specifically? As I stated elsewhere, we should have a note saying that ZK support has been removed with a link to all the implications as well as the migration details.

I completely agree that comprehensive migration details are necessary. However, since the documentation for these details may require further discussion, let's address them as a follow-up to avoid delaying the code cleanup. @TaiJuWu has already opened https://issues.apache.org/jira/browse/KAFKA-18364 to track the documentation effort.

In the meantime, could we create a temporary section listing the configurations removed due to Zookeeper? Once the migration details are finalized in KAFKA-18364, this temporary section can be replaced with a more thorough explanation. The temporary section is a note in KAFKA-18364 to ensure all implications are considered.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think we can make a note in that JIRA instead of the upgrade notes.

Copy link
Copy Markdown
Collaborator Author

@TaiJuWu TaiJuWu Dec 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove upgrade notes and this JIRA already link to KAFKA-18364

Copy link
Copy Markdown
Member

@chia7712 chia7712 Dec 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@frankvicky could you please file a minor to remove the password configs form the upgrade.html in order to follow @ijuma suggestion? I have added the link to KAFKA-18364

@chia7712
Copy link
Copy Markdown
Member

chia7712 commented Jan 6, 2025

@TaiJuWu please fix the build error

@TaiJuWu
Copy link
Copy Markdown
Collaborator Author

TaiJuWu commented Jan 7, 2025

@TaiJuWu please fix the build error

Thanks for remind, it should be fixed now.

}.sum
})
if (config.requiresZookeeper) {
metricsGroup.newGauge(s"${ControlPlaneAcceptor.MetricPrefix}ExpiredConnectionsKilledCount", () => SocketServer.this.synchronized {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@m1a2st please include this in #18365

}
})
if (config.requiresZookeeper) {
metricsGroup.newGauge(s"${ControlPlaneAcceptor.MetricPrefix}NetworkProcessorAvgIdlePercent", () => SocketServer.this.synchronized {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@m1a2st please include this in #18365

// Configure control plane listener to make sure we have separate listeners for testing.
val serverProperties = new java.util.HashMap[String, String]()
serverProperties.put(SocketServerConfigs.CONTROL_PLANE_LISTENER_NAME_CONFIG, controlPlaneListenerName)
serverProperties.put(SocketServerConfigs.LISTENER_SECURITY_PROTOCOL_MAP_CONFIG, s"$controlPlaneListenerName:$securityProtocol,$securityProtocol:$securityProtocol")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think controlPlaneListenerName is unnecessary to this test, but this test is disabled for now. Maybe we can revisit it in KAFKA-17631

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, #18330 handles it already

@chia7712 chia7712 merged commit 0c435e3 into apache:trunk Jan 8, 2025
@TaiJuWu TaiJuWu deleted the contol.plnae branch January 8, 2025 10:39
chia7712 pushed a commit that referenced this pull request Jan 8, 2025
Reviewers: Chia-Ping Tsai <chia7712@gmail.com>
pranavt84 pushed a commit to pranavt84/kafka that referenced this pull request Jan 27, 2025
)

Reviewers: Chia-Ping Tsai <chia7712@gmail.com>
manoj-mathivanan pushed a commit to manoj-mathivanan/kafka that referenced this pull request Feb 19, 2025
)

Reviewers: Chia-Ping Tsai <chia7712@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants