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/11/09 13:20:43 UTC
[GitHub] [kafka] mdedetrich opened a new pull request #11478: KAFKA-13299: Accept duplicate listener on port for IPv4/IPv6
mdedetrich opened a new pull request #11478:
URL: https://github.com/apache/kafka/pull/11478
Loosens the validation so that we accept duplicate listeners on the same port but if and only if the listeners are valid IP addresses with one address being an IPv4 address and the other being an IPv6 address. The detection of whether an address is a valid IP/IPv4/IPv6 address is done using Apache's commons-validator. Outside of this specific case the validations are kept the same albeit error messages have been updated to reflect that there is this new exception case of accepting IPv4/IPv6 on the same port.
This PR tests different corner cases by checking whether the changed `listenerListToEndPoints` either throws an exception or not. There is a good argument that the testing can be done better by using specific classes for the different types of exceptions, i.e.
```scala
class EachListenerDifferentPort(listeners: String) extends IllegalArgumentException {
override def getMessage: String = s"Each listener must have a different port, listeners: $listeners"
}
```
And then add a new version of `require` that allows us to throw a specific class that extends exception so that we can actually check each specific case, i.e.
```scala
assertThrows(classOf[EachListenerDifferentPort], () =>
listenerListToEndPoints("PLAINTEXT://127.0.0.1:9092,PLAINTEXT://127.0.0.1:9092,PLAINTEXT://127.0.0.1:9092")
)
```
If you want this to be done then please let me know.
### 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
[GitHub] [kafka] mdedetrich edited a comment on pull request #11478: KAFKA-13299: Accept duplicate listener on port for IPv4/IPv6
Posted by GitBox <gi...@apache.org>.
mdedetrich edited a comment on pull request #11478:
URL: https://github.com/apache/kafka/pull/11478#issuecomment-964997401
No worries, will write one today. In regards to the CI tests, in general they seem to be flaky. When I ran `:core:integrationTest` locally only the following tests failed and they seem unrelated. Is there a place in this PR/repo where I should update the documentation?
```
DynamicBrokerReconfigurationTest > testAdvertisedListenerUpdate() FAILED
org.opentest4j.AssertionFailedError: Unexpected exception type thrown ==> expected: <java.util.concurrent.ExecutionException> but was: <java.util.concurrent.TimeoutException>
at app//org.junit.jupiter.api.AssertThrows.assertThrows(AssertThrows.java:65)
at app//org.junit.jupiter.api.AssertThrows.assertThrows(AssertThrows.java:37)
at app//org.junit.jupiter.api.Assertions.assertThrows(Assertions.java:3007)
at app//kafka.server.DynamicBrokerReconfigurationTest.testAdvertisedListenerUpdate(DynamicBrokerReconfigurationTest.scala:990)
Caused by:
java.util.concurrent.TimeoutException: Timeout after waiting for 2000 ms.
at org.apache.kafka.clients.producer.internals.FutureRecordMetadata.get(FutureRecordMetadata.java:76)
at org.apache.kafka.clients.producer.internals.FutureRecordMetadata.get(FutureRecordMetadata.java:30)
at kafka.server.DynamicBrokerReconfigurationTest.$anonfun$testAdvertisedListenerUpdate$8(DynamicBrokerReconfigurationTest.scala:990)
at org.junit.jupiter.api.AssertThrows.assertThrows(AssertThrows.java:55)
... 3 more
```
```
DeleteTopicTest > testAddPartitionDuringDeleteTopic() FAILED
kafka.admin.AdminOperationException: org.apache.zookeeper.KeeperException$NoNodeException: KeeperErrorCode = NoNode for /brokers/topics/test
at app//kafka.zk.AdminZkClient.writeTopicPartitionAssignment(AdminZkClient.scala:178)
at app//kafka.zk.AdminZkClient.createPartitionsWithAssignment(AdminZkClient.scala:298)
at app//kafka.zk.AdminZkClient.addPartitions(AdminZkClient.scala:231)
at app//kafka.admin.DeleteTopicTest.testAddPartitionDuringDeleteTopic(DeleteTopicTest.scala:290)
```
--
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
[GitHub] [kafka] mdedetrich commented on pull request #11478: KAFKA-13299: Accept duplicate listener on port for IPv4/IPv6
Posted by GitBox <gi...@apache.org>.
mdedetrich commented on pull request #11478:
URL: https://github.com/apache/kafka/pull/11478#issuecomment-965514655
@dajac KIP created at https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=195726330
--
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
[GitHub] [kafka] mdedetrich edited a comment on pull request #11478: KAFKA-13299: Accept duplicate listener on port for IPv4/IPv6
Posted by GitBox <gi...@apache.org>.
mdedetrich edited a comment on pull request #11478:
URL: https://github.com/apache/kafka/pull/11478#issuecomment-964997401
No worries, will write one today. In regards to the CI tests, in general they seem to be flaky. When I ran `:core:integrationTest` locally only the following tests failed and they seem unrelated.
```
DynamicBrokerReconfigurationTest > testAdvertisedListenerUpdate() FAILED
org.opentest4j.AssertionFailedError: Unexpected exception type thrown ==> expected: <java.util.concurrent.ExecutionException> but was: <java.util.concurrent.TimeoutException>
at app//org.junit.jupiter.api.AssertThrows.assertThrows(AssertThrows.java:65)
at app//org.junit.jupiter.api.AssertThrows.assertThrows(AssertThrows.java:37)
at app//org.junit.jupiter.api.Assertions.assertThrows(Assertions.java:3007)
at app//kafka.server.DynamicBrokerReconfigurationTest.testAdvertisedListenerUpdate(DynamicBrokerReconfigurationTest.scala:990)
Caused by:
java.util.concurrent.TimeoutException: Timeout after waiting for 2000 ms.
at org.apache.kafka.clients.producer.internals.FutureRecordMetadata.get(FutureRecordMetadata.java:76)
at org.apache.kafka.clients.producer.internals.FutureRecordMetadata.get(FutureRecordMetadata.java:30)
at kafka.server.DynamicBrokerReconfigurationTest.$anonfun$testAdvertisedListenerUpdate$8(DynamicBrokerReconfigurationTest.scala:990)
at org.junit.jupiter.api.AssertThrows.assertThrows(AssertThrows.java:55)
... 3 more
```
```
DeleteTopicTest > testAddPartitionDuringDeleteTopic() FAILED
kafka.admin.AdminOperationException: org.apache.zookeeper.KeeperException$NoNodeException: KeeperErrorCode = NoNode for /brokers/topics/test
at app//kafka.zk.AdminZkClient.writeTopicPartitionAssignment(AdminZkClient.scala:178)
at app//kafka.zk.AdminZkClient.createPartitionsWithAssignment(AdminZkClient.scala:298)
at app//kafka.zk.AdminZkClient.addPartitions(AdminZkClient.scala:231)
at app//kafka.admin.DeleteTopicTest.testAddPartitionDuringDeleteTopic(DeleteTopicTest.scala:290)
```
On another note Is there a place in this PR/repo where I should update the documentation?
--
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
[GitHub] [kafka] dajac commented on pull request #11478: KAFKA-13299: Accept duplicate listener on port for IPv4/IPv6
Posted by GitBox <gi...@apache.org>.
dajac commented on pull request #11478:
URL: https://github.com/apache/kafka/pull/11478#issuecomment-964867210
Yeah... I would say that this one is a bit on the edge. I feel like we need a small KIP because the semantic of the configuration changes with your patch. 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
[GitHub] [kafka] dajac commented on pull request #11478: KAFKA-13299: Accept duplicate listener on port for IPv4/IPv6
Posted by GitBox <gi...@apache.org>.
dajac commented on pull request #11478:
URL: https://github.com/apache/kafka/pull/11478#issuecomment-964434998
@mdedetrich Thanks for the PR. As this changes a bit the semantic of the configuration, we might need a small KIP for it. Have you considered doing one?
--
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
[GitHub] [kafka] mdedetrich commented on pull request #11478: KAFKA-13299: Accept duplicate listener on port for IPv4/IPv6
Posted by GitBox <gi...@apache.org>.
mdedetrich commented on pull request #11478:
URL: https://github.com/apache/kafka/pull/11478#issuecomment-972784982
I have updated the PR to add some upgrade notes to `docs/upgrade.html`. I am not sure if additional documentation is needed elsewhere (I had a look at `docs` in general and couldn't find anything specific enough but I may have missed something).
--
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
[GitHub] [kafka] mdedetrich commented on pull request #11478: KAFKA-13299: Accept duplicate listener on port for IPv4/IPv6
Posted by GitBox <gi...@apache.org>.
mdedetrich commented on pull request #11478:
URL: https://github.com/apache/kafka/pull/11478#issuecomment-1035687499
I have rebased the PR against the latest Kafka trunk
--
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
[GitHub] [kafka] mdedetrich commented on pull request #11478: KAFKA-13299: Accept duplicate listener on port for IPv4/IPv6
Posted by GitBox <gi...@apache.org>.
mdedetrich commented on pull request #11478:
URL: https://github.com/apache/kafka/pull/11478#issuecomment-964997401
No worries, will write one today. In regards to the CI tests, in general they seem to be flaky. When I ran `:core:integrationTest` locally only the following tests failed and they seem unrelated.
```
DynamicBrokerReconfigurationTest > testAdvertisedListenerUpdate() FAILED
org.opentest4j.AssertionFailedError: Unexpected exception type thrown ==> expected: <java.util.concurrent.ExecutionException> but was: <java.util.concurrent.TimeoutException>
at app//org.junit.jupiter.api.AssertThrows.assertThrows(AssertThrows.java:65)
at app//org.junit.jupiter.api.AssertThrows.assertThrows(AssertThrows.java:37)
at app//org.junit.jupiter.api.Assertions.assertThrows(Assertions.java:3007)
at app//kafka.server.DynamicBrokerReconfigurationTest.testAdvertisedListenerUpdate(DynamicBrokerReconfigurationTest.scala:990)
Caused by:
java.util.concurrent.TimeoutException: Timeout after waiting for 2000 ms.
at org.apache.kafka.clients.producer.internals.FutureRecordMetadata.get(FutureRecordMetadata.java:76)
at org.apache.kafka.clients.producer.internals.FutureRecordMetadata.get(FutureRecordMetadata.java:30)
at kafka.server.DynamicBrokerReconfigurationTest.$anonfun$testAdvertisedListenerUpdate$8(DynamicBrokerReconfigurationTest.scala:990)
at org.junit.jupiter.api.AssertThrows.assertThrows(AssertThrows.java:55)
... 3 more
```
```
DeleteTopicTest > testAddPartitionDuringDeleteTopic() FAILED
kafka.admin.AdminOperationException: org.apache.zookeeper.KeeperException$NoNodeException: KeeperErrorCode = NoNode for /brokers/topics/test
at app//kafka.zk.AdminZkClient.writeTopicPartitionAssignment(AdminZkClient.scala:178)
at app//kafka.zk.AdminZkClient.createPartitionsWithAssignment(AdminZkClient.scala:298)
at app//kafka.zk.AdminZkClient.addPartitions(AdminZkClient.scala:231)
at app//kafka.admin.DeleteTopicTest.testAddPartitionDuringDeleteTopic(DeleteTopicTest.scala:290)
```
--
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
[GitHub] [kafka] mdedetrich commented on pull request #11478: KAFKA-13299: Accept duplicate listener on port for IPv4/IPv6
Posted by GitBox <gi...@apache.org>.
mdedetrich commented on pull request #11478:
URL: https://github.com/apache/kafka/pull/11478#issuecomment-964752828
Sure I will create one tomorrow, I didn't realize that adjusting the behavior while keep the exact same public interface required a KIP.
--
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
[GitHub] [kafka] mdedetrich commented on pull request #11478: KAFKA-13299: Accept duplicate listener on port for IPv4/IPv6
Posted by GitBox <gi...@apache.org>.
mdedetrich commented on pull request #11478:
URL: https://github.com/apache/kafka/pull/11478#issuecomment-972827297
I have updated the PR to add documentation to https://kafka.apache.org/documentation/#brokerconfigs_listeners
--
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
[GitHub] [kafka] mdedetrich commented on pull request #11478: KAFKA-13299: Accept duplicate listener on port for IPv4/IPv6
Posted by GitBox <gi...@apache.org>.
mdedetrich commented on pull request #11478:
URL: https://github.com/apache/kafka/pull/11478#issuecomment-965043983
I just updated/rebased the PR, apart from a very minor syntactic fixes we also now output the `port` when its context is relevant in the validation to help users. This was mainly done because the additional effort to do this was trivial (since we already went through the process of grouping ports we can say which port specifically failed the validation).
--
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