You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2021/03/30 08:45:47 UTC
[GitHub] [kafka] dajac opened a new pull request #10437: MINOR: Remove deprecated `host.name`, `advertised.host.name`, `port` and `advertised.port` for 3.0
dajac opened a new pull request #10437:
URL: https://github.com/apache/kafka/pull/10437
TODO
### 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma edited a comment on pull request #10437: MINOR: Remove deprecated `host.name`, `advertised.host.name`, `port` and `advertised.port` for 3.0
Posted by GitBox <gi...@apache.org>.
ijuma edited a comment on pull request #10437:
URL: https://github.com/apache/kafka/pull/10437#issuecomment-810327340
Makes sense. I assume you're planning to file a JIRA and the upgrade note still?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac commented on pull request #10437: MINOR: Remove deprecated `host.name`, `advertised.host.name`, `port` and `advertised.port` for 3.0
Posted by GitBox <gi...@apache.org>.
dajac commented on pull request #10437:
URL: https://github.com/apache/kafka/pull/10437#issuecomment-810332800
@ijuma Yes, will do. The PR is not fully completed yet. I'll ping you once it is ready.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on a change in pull request #10437: MINOR: Remove deprecated `host.name`, `advertised.host.name`, `port` and `advertised.port` for 3.0
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #10437:
URL: https://github.com/apache/kafka/pull/10437#discussion_r650625978
##########
File path: core/src/test/scala/unit/kafka/server/KafkaConfigTest.scala
##########
@@ -143,73 +143,6 @@ class KafkaConfigTest {
}
- @Test
- def testAdvertiseDefaults(): Unit = {
- val port = "9999"
- val hostName = "fake-host"
-
- val props = TestUtils.createBrokerConfig(0, TestUtils.MockZkConnect)
- props.remove(KafkaConfig.ListenersProp)
- props.put(KafkaConfig.HostNameProp, hostName)
- props.put(KafkaConfig.PortProp, port)
- val serverConfig = KafkaConfig.fromProps(props)
- val endpoints = serverConfig.advertisedListeners
- val endpoint = endpoints.find(_.securityProtocol == SecurityProtocol.PLAINTEXT).get
- assertEquals(endpoint.host, hostName)
- assertEquals(endpoint.port, port.toInt)
- }
-
- @Test
- def testAdvertiseConfigured(): Unit = {
Review comment:
I saw you removed all the `AdvertiseListener`'s tests here. Should we add or keep and update some tests for it?
##########
File path: core/src/main/scala/kafka/server/KafkaConfig.scala
##########
@@ -1804,9 +1773,7 @@ class KafkaConfig(val props: java.util.Map[_, _], doLog: Boolean, dynamicConfigO
// If the user did not define listeners but did define host or port, let's use them in backward compatible way
Review comment:
+1
##########
File path: core/src/main/scala/kafka/server/KafkaConfig.scala
##########
@@ -1838,8 +1805,6 @@ class KafkaConfig(val props: java.util.Map[_, _], doLog: Boolean, dynamicConfigO
val advertisedListenersProp = getString(KafkaConfig.AdvertisedListenersProp)
if (advertisedListenersProp != null)
CoreUtils.listenerListToEndPoints(advertisedListenersProp, listenerSecurityProtocolMap, requireDistinctPorts=false)
Review comment:
+1
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on a change in pull request #10437: MINOR: Remove deprecated `host.name`, `advertised.host.name`, `port` and `advertised.port` for 3.0
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #10437:
URL: https://github.com/apache/kafka/pull/10437#discussion_r608500887
##########
File path: connect/runtime/src/test/java/org/apache/kafka/connect/util/clusters/EmbeddedKafkaCluster.java
##########
@@ -232,7 +232,9 @@ public String bootstrapServers() {
}
public String address(KafkaServer server) {
- return server.config().hostName() + ":" + server.boundPort(listenerName);
+ final String hostName = JavaConverters.asJavaCollection(server.config().advertisedListeners())
Review comment:
Maybe we can move this method to `KafkaConfig`. It is useful for testing (connect and streams)
##########
File path: core/src/main/scala/kafka/server/KafkaConfig.scala
##########
@@ -1838,8 +1805,6 @@ class KafkaConfig(val props: java.util.Map[_, _], doLog: Boolean, dynamicConfigO
val advertisedListenersProp = getString(KafkaConfig.AdvertisedListenersProp)
if (advertisedListenersProp != null)
CoreUtils.listenerListToEndPoints(advertisedListenersProp, listenerSecurityProtocolMap, requireDistinctPorts=false)
Review comment:
the docs of this method need to be updated.
##########
File path: streams/src/test/java/org/apache/kafka/streams/integration/utils/KafkaEmbedded.java
##########
@@ -108,8 +107,10 @@ private Properties effectiveConfigFrom(final Properties initialConfig) {
@SuppressWarnings("WeakerAccess")
public String brokerList() {
final Object listenerConfig = effectiveConfig.get(KafkaConfig$.MODULE$.InterBrokerListenerNameProp());
Review comment:
not sure why it tried to get inter listeners. The return value of this method is used for producer/consumer.
##########
File path: streams/src/test/java/org/apache/kafka/streams/integration/utils/EmbeddedKafkaCluster.java
##########
@@ -101,7 +101,7 @@ public void start() throws IOException {
for (int i = 0; i < brokers.length; i++) {
brokerConfig.put(KafkaConfig$.MODULE$.BrokerIdProp(), i);
- log.debug("Starting a Kafka instance on port {} ...", brokerConfig.get(KafkaConfig$.MODULE$.PortProp()));
+ log.debug("Starting a Kafka instance on {} ...", brokerConfig.get(KafkaConfig$.MODULE$.ListenersProp()));
Review comment:
The default port is 0 so the return value of `brokerConfig.get(KafkaConfig$.MODULE$.ListenersProp())` is 0, right? If so, it is weird to log 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #10437: MINOR: Remove deprecated `host.name`, `advertised.host.name`, `port` and `advertised.port` for 3.0
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #10437:
URL: https://github.com/apache/kafka/pull/10437#discussion_r650629480
##########
File path: streams/src/test/java/org/apache/kafka/streams/integration/utils/EmbeddedKafkaCluster.java
##########
@@ -101,7 +101,7 @@ public void start() throws IOException {
for (int i = 0; i < brokers.length; i++) {
brokerConfig.put(KafkaConfig$.MODULE$.BrokerIdProp(), i);
- log.debug("Starting a Kafka instance on port {} ...", brokerConfig.get(KafkaConfig$.MODULE$.PortProp()));
+ log.debug("Starting a Kafka instance on {} ...", brokerConfig.get(KafkaConfig$.MODULE$.ListenersProp()));
Review comment:
This is logging the configured listener and after the broker is started, the resolved listener is logged. The default could have been changed, right?
##########
File path: streams/src/test/java/org/apache/kafka/streams/integration/utils/EmbeddedKafkaCluster.java
##########
@@ -101,7 +101,7 @@ public void start() throws IOException {
for (int i = 0; i < brokers.length; i++) {
brokerConfig.put(KafkaConfig$.MODULE$.BrokerIdProp(), i);
- log.debug("Starting a Kafka instance on port {} ...", brokerConfig.get(KafkaConfig$.MODULE$.PortProp()));
+ log.debug("Starting a Kafka instance on {} ...", brokerConfig.get(KafkaConfig$.MODULE$.ListenersProp()));
Review comment:
This is logging the configured listener and after the broker is started, the resolved listener is logged. The default could have been changed, so this seems ok. 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on pull request #10437: MINOR: Remove deprecated `host.name`, `advertised.host.name`, `port` and `advertised.port` for 3.0
Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #10437:
URL: https://github.com/apache/kafka/pull/10437#issuecomment-860334061
Also, the jenkins build didn't complete last time. Please remember to trigger it again. 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac commented on pull request #10437: MINOR: Remove deprecated `host.name`, `advertised.host.name`, `port` and `advertised.port` for 3.0
Posted by GitBox <gi...@apache.org>.
dajac commented on pull request #10437:
URL: https://github.com/apache/kafka/pull/10437#issuecomment-860797976
Replaced by https://github.com/apache/kafka/pull/10872.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #10437: MINOR: Remove deprecated `host.name`, `advertised.host.name`, `port` and `advertised.port` for 3.0
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #10437:
URL: https://github.com/apache/kafka/pull/10437#discussion_r606609310
##########
File path: core/src/main/scala/kafka/server/KafkaConfig.scala
##########
@@ -1804,9 +1773,7 @@ class KafkaConfig(val props: java.util.Map[_, _], doLog: Boolean, dynamicConfigO
// If the user did not define listeners but did define host or port, let's use them in backward compatible way
Review comment:
Maybe you saw this already, but this comment needs to be updated.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac closed pull request #10437: MINOR: Remove deprecated `host.name`, `advertised.host.name`, `port` and `advertised.port` for 3.0
Posted by GitBox <gi...@apache.org>.
dajac closed pull request #10437:
URL: https://github.com/apache/kafka/pull/10437
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on pull request #10437: MINOR: Remove deprecated `host.name`, `advertised.host.name`, `port` and `advertised.port` for 3.0
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #10437:
URL: https://github.com/apache/kafka/pull/10437#issuecomment-810327340
Makes sense. I assume you're planning to file a 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org