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 2020/10/14 18:18:45 UTC
[GitHub] [kafka] Lincong opened a new pull request #9435: MINOR KAFKA-10606 Disable auto topic creation for fetch-all-topic-metadata request
Lincong opened a new pull request #9435:
URL: https://github.com/apache/kafka/pull/9435
There is a bug that causes fetch-all-topic-metadata requests triggering
auto topic creation. Details are described in KAFKA-10606. This is the
simplest way to fix this bug on the broker side.
----------------------------------------------------------------
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 pull request #9435: KAFKA-10606: Disable auto topic creation for fetch-all-topic-metadata request
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #9435:
URL: https://github.com/apache/kafka/pull/9435#issuecomment-741590565
@ijuma I'd like to merge it tomorrow. would be helpful if you could give a final review :)
----------------------------------------------------------------
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] lmr3796 commented on a change in pull request #9435: KAFKA-10606: Disable auto topic creation for fetch-all-topic-metadata request
Posted by GitBox <gi...@apache.org>.
lmr3796 commented on a change in pull request #9435:
URL: https://github.com/apache/kafka/pull/9435#discussion_r533154067
##########
File path: core/src/main/scala/kafka/server/KafkaApis.scala
##########
@@ -1250,13 +1251,23 @@ class KafkaApis(val requestChannel: RequestChannel,
metadataResponseTopic(Errors.INVALID_REPLICATION_FACTOR, topic, true, util.Collections.emptyList())
else
topicMetadata
- } else if (allowAutoTopicCreation && config.autoCreateTopicsEnable) {
+ } else if (!isFetchAllMetadata && allowAutoTopicCreation && config.autoCreateTopicsEnable) {
+ // KAFKA-10606: If this request is to get metadata for all topics, auto topic creation should not be allowed
+ // The special handling is necessary on broker side because allowAutoTopicCreation is hard coded to true
+ // for backward compatibility on client side.
createTopic(topic, config.numPartitions, config.defaultReplicationFactor)
} else {
metadataResponseTopic(Errors.UNKNOWN_TOPIC_OR_PARTITION, topic, false, util.Collections.emptyList())
Review comment:
Also, I added the assertion of no UNKNOWN_TOPIC_OR_PARTITION into the unit test
----------------------------------------------------------------
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 pull request #9435: KAFKA-10606: Disable auto topic creation for fetch-all-topic-metadata request
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #9435:
URL: https://github.com/apache/kafka/pull/9435#issuecomment-732051836
+1 to last commit. Will merge it to trunk tomorrow if no object.
----------------------------------------------------------------
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] lmr3796 commented on a change in pull request #9435: KAFKA-10606: Disable auto topic creation for fetch-all-topic-metadata request
Posted by GitBox <gi...@apache.org>.
lmr3796 commented on a change in pull request #9435:
URL: https://github.com/apache/kafka/pull/9435#discussion_r533153634
##########
File path: core/src/main/scala/kafka/server/KafkaApis.scala
##########
@@ -1250,13 +1251,23 @@ class KafkaApis(val requestChannel: RequestChannel,
metadataResponseTopic(Errors.INVALID_REPLICATION_FACTOR, topic, true, util.Collections.emptyList())
else
topicMetadata
- } else if (allowAutoTopicCreation && config.autoCreateTopicsEnable) {
+ } else if (!isFetchAllMetadata && allowAutoTopicCreation && config.autoCreateTopicsEnable) {
+ // KAFKA-10606: If this request is to get metadata for all topics, auto topic creation should not be allowed
+ // The special handling is necessary on broker side because allowAutoTopicCreation is hard coded to true
+ // for backward compatibility on client side.
createTopic(topic, config.numPartitions, config.defaultReplicationFactor)
} else {
metadataResponseTopic(Errors.UNKNOWN_TOPIC_OR_PARTITION, topic, false, util.Collections.emptyList())
Review comment:
@ijuma I slightly changed it (and rebased latest) so the processing is a branch in the middle about `isFetchAllMetadata`.
It's still a `map` and `filter` structure because I don't want to sacrifice the readability, but it's handled in the middle instead of at the returning statement, which I think is more elegant and readable than the original version.
Let me know what you think!
----------------------------------------------------------------
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 #9435: KAFKA-10606: Disable auto topic creation for fetch-all-topic-metadata request
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #9435:
URL: https://github.com/apache/kafka/pull/9435#discussion_r530446068
##########
File path: core/src/main/scala/kafka/server/KafkaApis.scala
##########
@@ -1250,13 +1251,23 @@ class KafkaApis(val requestChannel: RequestChannel,
metadataResponseTopic(Errors.INVALID_REPLICATION_FACTOR, topic, true, util.Collections.emptyList())
else
topicMetadata
- } else if (allowAutoTopicCreation && config.autoCreateTopicsEnable) {
+ } else if (!isFetchAllMetadata && allowAutoTopicCreation && config.autoCreateTopicsEnable) {
+ // KAFKA-10606: If this request is to get metadata for all topics, auto topic creation should not be allowed
+ // The special handling is necessary on broker side because allowAutoTopicCreation is hard coded to true
+ // for backward compatibility on client side.
createTopic(topic, config.numPartitions, config.defaultReplicationFactor)
} else {
metadataResponseTopic(Errors.UNKNOWN_TOPIC_OR_PARTITION, topic, false, util.Collections.emptyList())
Review comment:
Could we not avoid this altogether instead of doing the `filter` later?
----------------------------------------------------------------
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 #9435: KAFKA-10606: Disable auto topic creation for fetch-all-topic-metadata request
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #9435:
URL: https://github.com/apache/kafka/pull/9435#discussion_r530269737
##########
File path: core/src/main/scala/kafka/server/KafkaApis.scala
##########
@@ -1333,7 +1343,17 @@ class KafkaApis(val requestChannel: RequestChannel,
}
}
- val completeTopicMetadata = topicMetadata ++ unauthorizedForCreateTopicMetadata ++ unauthorizedForDescribeTopicMetadata
+ val completeTopicMetadata = (if (metadataRequest.isAllTopics) {
Review comment:
@ijuma I will merge this PR tomorrow if there is no objection.
----------------------------------------------------------------
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 pull request #9435: KAFKA-10606: Disable auto topic creation for fetch-all-topic-metadata request
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #9435:
URL: https://github.com/apache/kafka/pull/9435#issuecomment-740351361
@lmr3796 Thanks for your updating. Could you rebase code to include recent big commits to trigger QA again?
----------------------------------------------------------------
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] Lincong commented on pull request #9435: KAFKA-10606: Disable auto topic creation for fetch-all-topic-metadata request
Posted by GitBox <gi...@apache.org>.
Lincong commented on pull request #9435:
URL: https://github.com/apache/kafka/pull/9435#issuecomment-726495188
> @Lincong Thanks for updating code. Could you fix the conflicting files as well?
Conflicts resolved
----------------------------------------------------------------
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 #9435: KAFKA-10606: Disable auto topic creation for fetch-all-topic-metadata request
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #9435:
URL: https://github.com/apache/kafka/pull/9435#discussion_r526616584
##########
File path: core/src/main/scala/kafka/server/KafkaApis.scala
##########
@@ -1333,7 +1343,17 @@ class KafkaApis(val requestChannel: RequestChannel,
}
}
- val completeTopicMetadata = topicMetadata ++ unauthorizedForCreateTopicMetadata ++ unauthorizedForDescribeTopicMetadata
+ val completeTopicMetadata = (if (metadataRequest.isAllTopics) {
Review comment:
Could we do this filter early? For example, how about filtering ```getTopicMetadata``` directly? That makes all fixes be in one place and following authorized ops can skip "unknown partition/topic" as well.
----------------------------------------------------------------
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 #9435: KAFKA-10606: Disable auto topic creation for fetch-all-topic-metadata request
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #9435:
URL: https://github.com/apache/kafka/pull/9435#discussion_r528585768
##########
File path: core/src/test/scala/unit/kafka/server/KafkaApisTest.scala
##########
@@ -107,6 +108,11 @@ class KafkaApisTest {
private val time = new MockTime
private val clientId = ""
+ @Before
+ def setUp(): Unit = {
Review comment:
Could we remove this ```@Before``` code? Changing modifier of ```metadataCache``` from "val" to "var" is enough.
----------------------------------------------------------------
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 edited a comment on pull request #9435: KAFKA-10606: Disable auto topic creation for fetch-all-topic-metadata request
Posted by GitBox <gi...@apache.org>.
chia7712 edited a comment on pull request #9435:
URL: https://github.com/apache/kafka/pull/9435#issuecomment-741600635
> I thought my suggested comment in the following was clear and concise, but I'm interested in other opinions:
> #9435 (comment)
@ijuma Sorry that I neglected that conversation.
@lmr3796 FYI ~
----------------------------------------------------------------
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 #9435: KAFKA-10606: Disable auto topic creation for fetch-all-topic-metadata request
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #9435:
URL: https://github.com/apache/kafka/pull/9435#discussion_r535321971
##########
File path: core/src/main/scala/kafka/server/KafkaApis.scala
##########
@@ -1243,19 +1244,30 @@ class KafkaApis(val requestChannel: RequestChannel,
topicResponses
} else {
val nonExistentTopics = topics.diff(topicResponses.map(_.name).toSet)
- val responsesForNonExistentTopics = nonExistentTopics.map { topic =>
+ val responsesForNonExistentTopics = nonExistentTopics.flatMap { topic =>
if (isInternal(topic)) {
val topicMetadata = createInternalTopic(topic)
- if (topicMetadata.errorCode == Errors.COORDINATOR_NOT_AVAILABLE.code)
- metadataResponseTopic(Errors.INVALID_REPLICATION_FACTOR, topic, true, util.Collections.emptyList())
- else
- topicMetadata
+ Some(
+ if (topicMetadata.errorCode == Errors.COORDINATOR_NOT_AVAILABLE.code)
+ metadataResponseTopic(Errors.INVALID_REPLICATION_FACTOR, topic, true, util.Collections.emptyList())
+ else
+ topicMetadata
+ )
+ } else if (isFetchAllMetadata) {
+ // KAFKA-10606: If this request is to get metadata for all topics, auto topic creation should not be allowed
+ // The special handling is necessary on broker side because allowAutoTopicCreation is hard coded to true
+ // for backward compatibility on client side.
+ //
+ // However, in previous versions, UNKNOWN_TOPIC_OR_PARTITION won't happen on fetch all metadata,
+ // so, for backward-compatibility, we need to skip these not founds during fetch all metadata here.
Review comment:
We don't usually include JIRA references in the code unless it's a very complex issue. I think the comment here could be something like:
"A metadata request for all topics should never result in topic auto creation. A topic may be deleted between the creation of `topics` and `topicResponses`, so we make sure to always return `None` for this case."
----------------------------------------------------------------
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 #9435: KAFKA-10606: Disable auto topic creation for fetch-all-topic-metadata request
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #9435:
URL: https://github.com/apache/kafka/pull/9435#discussion_r533402852
##########
File path: core/src/main/scala/kafka/server/KafkaApis.scala
##########
@@ -1250,13 +1251,23 @@ class KafkaApis(val requestChannel: RequestChannel,
metadataResponseTopic(Errors.INVALID_REPLICATION_FACTOR, topic, true, util.Collections.emptyList())
else
topicMetadata
- } else if (allowAutoTopicCreation && config.autoCreateTopicsEnable) {
+ } else if (!isFetchAllMetadata && allowAutoTopicCreation && config.autoCreateTopicsEnable) {
+ // KAFKA-10606: If this request is to get metadata for all topics, auto topic creation should not be allowed
+ // The special handling is necessary on broker side because allowAutoTopicCreation is hard coded to true
+ // for backward compatibility on client side.
createTopic(topic, config.numPartitions, config.defaultReplicationFactor)
} else {
metadataResponseTopic(Errors.UNKNOWN_TOPIC_OR_PARTITION, topic, false, util.Collections.emptyList())
Review comment:
I think if you use `flatMap`, you don't need the `filter` and it's still readable. Thoughts?
----------------------------------------------------------------
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] lmr3796 commented on pull request #9435: KAFKA-10606: Disable auto topic creation for fetch-all-topic-metadata request
Posted by GitBox <gi...@apache.org>.
lmr3796 commented on pull request #9435:
URL: https://github.com/apache/kafka/pull/9435#issuecomment-741649483
Hi thanks @ijuma @chia7712 ,
I updated the comments accordingly to make it more concise and rebased latest trunk.
Please let me know if there are any other issues.
----------------------------------------------------------------
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] lmr3796 commented on a change in pull request #9435: KAFKA-10606: Disable auto topic creation for fetch-all-topic-metadata request
Posted by GitBox <gi...@apache.org>.
lmr3796 commented on a change in pull request #9435:
URL: https://github.com/apache/kafka/pull/9435#discussion_r533648988
##########
File path: core/src/main/scala/kafka/server/KafkaApis.scala
##########
@@ -1250,13 +1251,23 @@ class KafkaApis(val requestChannel: RequestChannel,
metadataResponseTopic(Errors.INVALID_REPLICATION_FACTOR, topic, true, util.Collections.emptyList())
else
topicMetadata
- } else if (allowAutoTopicCreation && config.autoCreateTopicsEnable) {
+ } else if (!isFetchAllMetadata && allowAutoTopicCreation && config.autoCreateTopicsEnable) {
+ // KAFKA-10606: If this request is to get metadata for all topics, auto topic creation should not be allowed
+ // The special handling is necessary on broker side because allowAutoTopicCreation is hard coded to true
+ // for backward compatibility on client side.
createTopic(topic, config.numPartitions, config.defaultReplicationFactor)
} else {
metadataResponseTopic(Errors.UNKNOWN_TOPIC_OR_PARTITION, topic, false, util.Collections.emptyList())
Review comment:
@ijuma That's a good point. I've updated 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] Lincong commented on a change in pull request #9435: KAFKA-10606: Disable auto topic creation for fetch-all-topic-metadata request
Posted by GitBox <gi...@apache.org>.
Lincong commented on a change in pull request #9435:
URL: https://github.com/apache/kafka/pull/9435#discussion_r522441321
##########
File path: core/src/main/scala/kafka/server/KafkaApis.scala
##########
@@ -1236,9 +1236,19 @@ class KafkaApis(val requestChannel: RequestChannel,
val topicMetadata =
if (authorizedTopics.isEmpty)
Seq.empty[MetadataResponseTopic]
- else
- getTopicMetadata(metadataRequest.allowAutoTopicCreation, authorizedTopics, request.context.listenerName,
- errorUnavailableEndpoints, errorUnavailableListeners)
+ else {
+ // KAFKA-10606: If this request is to get metadata for all topics, auto topic creation should not be allowed
+ // The special handling is necessary on broker side because allowAutoTopicCreation is hard coded to true
+ // for backward compatibility on client side.
+ val allowAutoTopicCreation = (!metadataRequest.isAllTopics) && metadataRequest.allowAutoTopicCreation
Review comment:
Hi @chia7712 , thank you for bringing up this issue and it's totally valid!
In order to preserve its behavior (not including `UNKNOWN_TOPIC_OR_PARTITION ` in the response to fetch all topic metadata request), we implemented the below logic which filters out all entries in the response with `UNKNOWN_TOPIC_OR_PARTITION ` if the metadata request is to get metadata for all topisc.
```
val completeTopicMetadata = (if (metadataRequest.isAllTopics) {
opicMetadata.filter(_.errorCode() != Errors.UNKNOWN_TOPIC_OR_PARTITION.code())
} else {
topicMetadata
}) ++ unauthorizedForCreateTopicMetadata ++ unauthorizedForDescribeTopicMetadata
```
We have added a unit test as well. Hope to get some feedback from you soon!
----------------------------------------------------------------
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 merged pull request #9435: KAFKA-10606: Disable auto topic creation for fetch-all-topic-metadata request
Posted by GitBox <gi...@apache.org>.
chia7712 merged pull request #9435:
URL: https://github.com/apache/kafka/pull/9435
----------------------------------------------------------------
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 #9435: KAFKA-10606: Disable auto topic creation for fetch-all-topic-metadata request
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #9435:
URL: https://github.com/apache/kafka/pull/9435#discussion_r529240711
##########
File path: core/src/test/scala/unit/kafka/server/KafkaApisTest.scala
##########
@@ -107,6 +108,11 @@ class KafkaApisTest {
private val time = new MockTime
private val clientId = ""
+ @Before
+ def setUp(): Unit = {
Review comment:
It seems to me following code is able to generate a clean, uncorrupted ```MetadataCache``` for each test case.
```
var metadataCache = new MetadataCache(brokerId)
```
Also, it is simpler than ```@Before``` block.
----------------------------------------------------------------
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] lmr3796 commented on a change in pull request #9435: KAFKA-10606: Disable auto topic creation for fetch-all-topic-metadata request
Posted by GitBox <gi...@apache.org>.
lmr3796 commented on a change in pull request #9435:
URL: https://github.com/apache/kafka/pull/9435#discussion_r527192802
##########
File path: core/src/main/scala/kafka/server/KafkaApis.scala
##########
@@ -1333,7 +1343,17 @@ class KafkaApis(val requestChannel: RequestChannel,
}
}
- val completeTopicMetadata = topicMetadata ++ unauthorizedForCreateTopicMetadata ++ unauthorizedForDescribeTopicMetadata
+ val completeTopicMetadata = (if (metadataRequest.isAllTopics) {
Review comment:
Hi @chia7712 @ijuma ,
This is Joseph and I'm @Lincong 's colleague working on this patch with him. I think this is a good point and just updated the PR accordingly
----------------------------------------------------------------
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 #9435: MINOR KAFKA-10606 Disable auto topic creation for fetch-all-topic-metadata request
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #9435:
URL: https://github.com/apache/kafka/pull/9435#issuecomment-708617750
Thanks for the PR. Thoughts on how to test this?
----------------------------------------------------------------
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] lmr3796 commented on a change in pull request #9435: KAFKA-10606: Disable auto topic creation for fetch-all-topic-metadata request
Posted by GitBox <gi...@apache.org>.
lmr3796 commented on a change in pull request #9435:
URL: https://github.com/apache/kafka/pull/9435#discussion_r529203652
##########
File path: core/src/test/scala/unit/kafka/server/KafkaApisTest.scala
##########
@@ -107,6 +108,11 @@ class KafkaApisTest {
private val time = new MockTime
private val clientId = ""
+ @Before
+ def setUp(): Unit = {
Review comment:
hey @chia7712 ,
I add this to let each test case to have a clean, uncorrupted `MetadataCache`. Is there any historical context that we should avoid that cleanup?
----------------------------------------------------------------
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 pull request #9435: KAFKA-10606: Disable auto topic creation for fetch-all-topic-metadata request
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #9435:
URL: https://github.com/apache/kafka/pull/9435#issuecomment-726491192
@Lincong Thanks for updating code. Could you fix the conflicting files as well?
----------------------------------------------------------------
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 pull request #9435: KAFKA-10606: Disable auto topic creation for fetch-all-topic-metadata request
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #9435:
URL: https://github.com/apache/kafka/pull/9435#issuecomment-741744599
> I updated the comments accordingly to make it more concise and rebased latest trunk.
Please let me know if there are any other issues.
@lmr3796 Thanks for updating. waiting for response from @ijuma :)
----------------------------------------------------------------
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 #9435: MINOR KAFKA-10606 Disable auto topic creation for fetch-all-topic-metadata request
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #9435:
URL: https://github.com/apache/kafka/pull/9435#issuecomment-708671677
`KafkaApisTest` has a few tests like that. Could we add one there? The main concern is that we will regress here if we don't have tests.
----------------------------------------------------------------
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] Lincong commented on pull request #9435: MINOR KAFKA-10606 Disable auto topic creation for fetch-all-topic-metadata request
Posted by GitBox <gi...@apache.org>.
Lincong commented on pull request #9435:
URL: https://github.com/apache/kafka/pull/9435#issuecomment-708650004
@ijuma One way to test it could be to mock objects passed to the `KafkaApis` class. The `MetadataCache` object should be mocked in a way that upon the first invocation on the method `getAllTopics` to simulate the scenario in which the metadata cache got updated "concurrently" with `handleTopicMetadataRequest`.
However, IMO, this change is pretty minor. So, I am wondering how necessary it is to add tests for 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] lmr3796 commented on a change in pull request #9435: KAFKA-10606: Disable auto topic creation for fetch-all-topic-metadata request
Posted by GitBox <gi...@apache.org>.
lmr3796 commented on a change in pull request #9435:
URL: https://github.com/apache/kafka/pull/9435#discussion_r535489887
##########
File path: core/src/main/scala/kafka/server/KafkaApis.scala
##########
@@ -1243,19 +1244,30 @@ class KafkaApis(val requestChannel: RequestChannel,
topicResponses
} else {
val nonExistentTopics = topics.diff(topicResponses.map(_.name).toSet)
- val responsesForNonExistentTopics = nonExistentTopics.map { topic =>
+ val responsesForNonExistentTopics = nonExistentTopics.flatMap { topic =>
if (isInternal(topic)) {
val topicMetadata = createInternalTopic(topic)
- if (topicMetadata.errorCode == Errors.COORDINATOR_NOT_AVAILABLE.code)
- metadataResponseTopic(Errors.INVALID_REPLICATION_FACTOR, topic, true, util.Collections.emptyList())
- else
- topicMetadata
+ Some(
+ if (topicMetadata.errorCode == Errors.COORDINATOR_NOT_AVAILABLE.code)
+ metadataResponseTopic(Errors.INVALID_REPLICATION_FACTOR, topic, true, util.Collections.emptyList())
+ else
+ topicMetadata
+ )
+ } else if (isFetchAllMetadata) {
+ // KAFKA-10606: If this request is to get metadata for all topics, auto topic creation should not be allowed
+ // The special handling is necessary on broker side because allowAutoTopicCreation is hard coded to true
+ // for backward compatibility on client side.
+ //
+ // However, in previous versions, UNKNOWN_TOPIC_OR_PARTITION won't happen on fetch all metadata,
+ // so, for backward-compatibility, we need to skip these not founds during fetch all metadata here.
Review comment:
Thanks @ijuma ,
To follow the convention in Kafka, I just updated the comments. Let me know if you have there are any other thoughts!
----------------------------------------------------------------
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] Lincong commented on pull request #9435: MINOR KAFKA-10606 Disable auto topic creation for fetch-all-topic-metadata request
Posted by GitBox <gi...@apache.org>.
Lincong commented on pull request #9435:
URL: https://github.com/apache/kafka/pull/9435#issuecomment-708681626
@ijuma Will do! 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] chia7712 commented on pull request #9435: KAFKA-10606: Disable auto topic creation for fetch-all-topic-metadata request
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #9435:
URL: https://github.com/apache/kafka/pull/9435#issuecomment-741847741
@Lincong @lmr3796 Thanks for your efforts!
----------------------------------------------------------------
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] lmr3796 commented on a change in pull request #9435: KAFKA-10606: Disable auto topic creation for fetch-all-topic-metadata request
Posted by GitBox <gi...@apache.org>.
lmr3796 commented on a change in pull request #9435:
URL: https://github.com/apache/kafka/pull/9435#discussion_r530139420
##########
File path: core/src/test/scala/unit/kafka/server/KafkaApisTest.scala
##########
@@ -107,6 +108,11 @@ class KafkaApisTest {
private val time = new MockTime
private val clientId = ""
+ @Before
+ def setUp(): Unit = {
Review comment:
Hey @chia7712 you're right. I did some reading and found that I misunderstood JUnit's behavior. Just updated that part and rebased latest 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on pull request #9435: KAFKA-10606: Disable auto topic creation for fetch-all-topic-metadata request
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #9435:
URL: https://github.com/apache/kafka/pull/9435#issuecomment-729399596
@Lincong Could you take a look at those failed tests? I will give a review later :)
----------------------------------------------------------------
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 pull request #9435: KAFKA-10606: Disable auto topic creation for fetch-all-topic-metadata request
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #9435:
URL: https://github.com/apache/kafka/pull/9435#issuecomment-741600635
> I thought my suggested comment in the following was clear and concise, but I'm interested in other opinions:
> #9435 (comment)
@ijuma Sorry that I neglected that conversion.
@lmr3796 FYI ~
----------------------------------------------------------------
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] lmr3796 commented on a change in pull request #9435: KAFKA-10606: Disable auto topic creation for fetch-all-topic-metadata request
Posted by GitBox <gi...@apache.org>.
lmr3796 commented on a change in pull request #9435:
URL: https://github.com/apache/kafka/pull/9435#discussion_r535019810
##########
File path: core/src/main/scala/kafka/server/KafkaApis.scala
##########
@@ -1243,19 +1244,30 @@ class KafkaApis(val requestChannel: RequestChannel,
topicResponses
} else {
val nonExistentTopics = topics.diff(topicResponses.map(_.name).toSet)
- val responsesForNonExistentTopics = nonExistentTopics.map { topic =>
+ val responsesForNonExistentTopics = nonExistentTopics.flatMap { topic =>
if (isInternal(topic)) {
val topicMetadata = createInternalTopic(topic)
- if (topicMetadata.errorCode == Errors.COORDINATOR_NOT_AVAILABLE.code)
- metadataResponseTopic(Errors.INVALID_REPLICATION_FACTOR, topic, true, util.Collections.emptyList())
- else
- topicMetadata
+ List(
+ if (topicMetadata.errorCode == Errors.COORDINATOR_NOT_AVAILABLE.code)
+ metadataResponseTopic(Errors.INVALID_REPLICATION_FACTOR, topic, true, util.Collections.emptyList())
+ else
+ topicMetadata
+ )
+ } else if (isFetchAllMetadata) {
+ // KAFKA-10606: If this request is to get metadata for all topics, auto topic creation should not be allowed
+ // The special handling is necessary on broker side because allowAutoTopicCreation is hard coded to true
+ // for backward compatibility on client side.
+ //
+ // However, in previous versions, UNKNOWN_TOPIC_OR_PARTITION won't happen on fetch all metadata,
+ // so, for backward-compatibility, we need to skip these not founds during fetch all metadata here.
+ Nil
Review comment:
@chia7712 Thanks, you're right. Just updated accordingly
----------------------------------------------------------------
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 #9435: KAFKA-10606: Disable auto topic creation for fetch-all-topic-metadata request
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #9435:
URL: https://github.com/apache/kafka/pull/9435#discussion_r534752326
##########
File path: core/src/main/scala/kafka/server/KafkaApis.scala
##########
@@ -1243,19 +1244,30 @@ class KafkaApis(val requestChannel: RequestChannel,
topicResponses
} else {
val nonExistentTopics = topics.diff(topicResponses.map(_.name).toSet)
- val responsesForNonExistentTopics = nonExistentTopics.map { topic =>
+ val responsesForNonExistentTopics = nonExistentTopics.flatMap { topic =>
if (isInternal(topic)) {
val topicMetadata = createInternalTopic(topic)
- if (topicMetadata.errorCode == Errors.COORDINATOR_NOT_AVAILABLE.code)
- metadataResponseTopic(Errors.INVALID_REPLICATION_FACTOR, topic, true, util.Collections.emptyList())
- else
- topicMetadata
+ List(
+ if (topicMetadata.errorCode == Errors.COORDINATOR_NOT_AVAILABLE.code)
+ metadataResponseTopic(Errors.INVALID_REPLICATION_FACTOR, topic, true, util.Collections.emptyList())
+ else
+ topicMetadata
+ )
+ } else if (isFetchAllMetadata) {
+ // KAFKA-10606: If this request is to get metadata for all topics, auto topic creation should not be allowed
+ // The special handling is necessary on broker side because allowAutoTopicCreation is hard coded to true
+ // for backward compatibility on client side.
+ //
+ // However, in previous versions, UNKNOWN_TOPIC_OR_PARTITION won't happen on fetch all metadata,
+ // so, for backward-compatibility, we need to skip these not founds during fetch all metadata here.
+ Nil
Review comment:
Is ```Some/None``` more readable than ```List/Nil```?
----------------------------------------------------------------
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 #9435: KAFKA-10606: Disable auto topic creation for fetch-all-topic-metadata request
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #9435:
URL: https://github.com/apache/kafka/pull/9435#discussion_r526911200
##########
File path: core/src/main/scala/kafka/server/KafkaApis.scala
##########
@@ -1333,7 +1343,17 @@ class KafkaApis(val requestChannel: RequestChannel,
}
}
- val completeTopicMetadata = topicMetadata ++ unauthorizedForCreateTopicMetadata ++ unauthorizedForDescribeTopicMetadata
+ val completeTopicMetadata = (if (metadataRequest.isAllTopics) {
Review comment:
This is a good point. We should pass a boolean to `getTopicMetadata` indicating that it's an "allTopics" request and have that method handle everything.
----------------------------------------------------------------
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 #9435: KAFKA-10606: Disable auto topic creation for fetch-all-topic-metadata request
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #9435:
URL: https://github.com/apache/kafka/pull/9435#issuecomment-741597240
@chia7712 The code changes seen fine. The comment in KafkaApis still seems a bit convoluted. For example, not returning UnknownTopicOrPartition is not related to backwards compatibility, it doesn't make sense to return that if the given topic wasn't explicitly requested. Similarly, the fact that allowAutoTopicCreation is true for some clients isn't that relevant, the code needs to handle it irrespective of what some clients do or don't do.
I thought my suggested comment in the following was clear and concise, but I'm interested in other opinions:
https://github.com/apache/kafka/pull/9435#discussion_r535321971
----------------------------------------------------------------
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] lmr3796 commented on a change in pull request #9435: KAFKA-10606: Disable auto topic creation for fetch-all-topic-metadata request
Posted by GitBox <gi...@apache.org>.
lmr3796 commented on a change in pull request #9435:
URL: https://github.com/apache/kafka/pull/9435#discussion_r535019810
##########
File path: core/src/main/scala/kafka/server/KafkaApis.scala
##########
@@ -1243,19 +1244,30 @@ class KafkaApis(val requestChannel: RequestChannel,
topicResponses
} else {
val nonExistentTopics = topics.diff(topicResponses.map(_.name).toSet)
- val responsesForNonExistentTopics = nonExistentTopics.map { topic =>
+ val responsesForNonExistentTopics = nonExistentTopics.flatMap { topic =>
if (isInternal(topic)) {
val topicMetadata = createInternalTopic(topic)
- if (topicMetadata.errorCode == Errors.COORDINATOR_NOT_AVAILABLE.code)
- metadataResponseTopic(Errors.INVALID_REPLICATION_FACTOR, topic, true, util.Collections.emptyList())
- else
- topicMetadata
+ List(
+ if (topicMetadata.errorCode == Errors.COORDINATOR_NOT_AVAILABLE.code)
+ metadataResponseTopic(Errors.INVALID_REPLICATION_FACTOR, topic, true, util.Collections.emptyList())
+ else
+ topicMetadata
+ )
+ } else if (isFetchAllMetadata) {
+ // KAFKA-10606: If this request is to get metadata for all topics, auto topic creation should not be allowed
+ // The special handling is necessary on broker side because allowAutoTopicCreation is hard coded to true
+ // for backward compatibility on client side.
+ //
+ // However, in previous versions, UNKNOWN_TOPIC_OR_PARTITION won't happen on fetch all metadata,
+ // so, for backward-compatibility, we need to skip these not founds during fetch all metadata here.
+ Nil
Review comment:
@chia7712 Thanks, you're right. Just updated accordingly and rebased latest 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] Lincong commented on a change in pull request #9435: KAFKA-10606: Disable auto topic creation for fetch-all-topic-metadata request
Posted by GitBox <gi...@apache.org>.
Lincong commented on a change in pull request #9435:
URL: https://github.com/apache/kafka/pull/9435#discussion_r522441321
##########
File path: core/src/main/scala/kafka/server/KafkaApis.scala
##########
@@ -1236,9 +1236,19 @@ class KafkaApis(val requestChannel: RequestChannel,
val topicMetadata =
if (authorizedTopics.isEmpty)
Seq.empty[MetadataResponseTopic]
- else
- getTopicMetadata(metadataRequest.allowAutoTopicCreation, authorizedTopics, request.context.listenerName,
- errorUnavailableEndpoints, errorUnavailableListeners)
+ else {
+ // KAFKA-10606: If this request is to get metadata for all topics, auto topic creation should not be allowed
+ // The special handling is necessary on broker side because allowAutoTopicCreation is hard coded to true
+ // for backward compatibility on client side.
+ val allowAutoTopicCreation = (!metadataRequest.isAllTopics) && metadataRequest.allowAutoTopicCreation
Review comment:
Hi @chia7712 , thank you for bringing up that issue and it's totally valid!
In order to preserve its behavior (not including `UNKNOWN_TOPIC_OR_PARTITION ` in the response to fetch all topic metadata request), we implemented the below logic which filters out all entries in the response with `UNKNOWN_TOPIC_OR_PARTITION ` if the metadata request is to get metadata for all topisc.
```
val completeTopicMetadata = (if (metadataRequest.isAllTopics) {
opicMetadata.filter(_.errorCode() != Errors.UNKNOWN_TOPIC_OR_PARTITION.code())
} else {
topicMetadata
}) ++ unauthorizedForCreateTopicMetadata ++ unauthorizedForDescribeTopicMetadata
```
We have added a unit test as well. Hope to get some feedback from you soon!
----------------------------------------------------------------
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] lmr3796 commented on a change in pull request #9435: KAFKA-10606: Disable auto topic creation for fetch-all-topic-metadata request
Posted by GitBox <gi...@apache.org>.
lmr3796 commented on a change in pull request #9435:
URL: https://github.com/apache/kafka/pull/9435#discussion_r533153634
##########
File path: core/src/main/scala/kafka/server/KafkaApis.scala
##########
@@ -1250,13 +1251,23 @@ class KafkaApis(val requestChannel: RequestChannel,
metadataResponseTopic(Errors.INVALID_REPLICATION_FACTOR, topic, true, util.Collections.emptyList())
else
topicMetadata
- } else if (allowAutoTopicCreation && config.autoCreateTopicsEnable) {
+ } else if (!isFetchAllMetadata && allowAutoTopicCreation && config.autoCreateTopicsEnable) {
+ // KAFKA-10606: If this request is to get metadata for all topics, auto topic creation should not be allowed
+ // The special handling is necessary on broker side because allowAutoTopicCreation is hard coded to true
+ // for backward compatibility on client side.
createTopic(topic, config.numPartitions, config.defaultReplicationFactor)
} else {
metadataResponseTopic(Errors.UNKNOWN_TOPIC_OR_PARTITION, topic, false, util.Collections.emptyList())
Review comment:
@ijuma I slightly changed it so the processing is a branch in the middle about `isFetchAllMetadata`.
It's still a `map` and `filter` structure because I don't want to sacrifice the readability, but it's handled in the middle instead of at the returning statement, which I think is more elegant and readable than the original version.
Let me know what you think!
----------------------------------------------------------------
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 #9435: KAFKA-10606: Disable auto topic creation for fetch-all-topic-metadata request
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #9435:
URL: https://github.com/apache/kafka/pull/9435#discussion_r509055559
##########
File path: core/src/main/scala/kafka/server/KafkaApis.scala
##########
@@ -1236,9 +1236,19 @@ class KafkaApis(val requestChannel: RequestChannel,
val topicMetadata =
if (authorizedTopics.isEmpty)
Seq.empty[MetadataResponseTopic]
- else
- getTopicMetadata(metadataRequest.allowAutoTopicCreation, authorizedTopics, request.context.listenerName,
- errorUnavailableEndpoints, errorUnavailableListeners)
+ else {
+ // KAFKA-10606: If this request is to get metadata for all topics, auto topic creation should not be allowed
+ // The special handling is necessary on broker side because allowAutoTopicCreation is hard coded to true
+ // for backward compatibility on client side.
+ val allowAutoTopicCreation = (!metadataRequest.isAllTopics) && metadataRequest.allowAutoTopicCreation
Review comment:
If we pass ```false``` to getTopicMetadata, it generates ```UNKNOWN_TOPIC_OR_PARTITION``` when the topic is removed, right? If so, does client-side need to handle such error? For example, KafkaAdminClient#listTopics should filter out those "nonexistent" topics (https://github.com/apache/kafka/blob/trunk/clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java#L1717). Otherwise, users want to get all "existent" topics but response say a_topic is "nonexistent".
----------------------------------------------------------------
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] lmr3796 commented on pull request #9435: KAFKA-10606: Disable auto topic creation for fetch-all-topic-metadata request
Posted by GitBox <gi...@apache.org>.
lmr3796 commented on pull request #9435:
URL: https://github.com/apache/kafka/pull/9435#issuecomment-740431014
@chia7712 @ijuma I just rebased to latest trunk. Please take a look
----------------------------------------------------------------
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 #9435: KAFKA-10606: Disable auto topic creation for fetch-all-topic-metadata request
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #9435:
URL: https://github.com/apache/kafka/pull/9435#issuecomment-741810105
@chia7712 feel free to merge if it looks good to you. Thanks @lmr3796 for the patience.
----------------------------------------------------------------
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