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/05/22 14:30:17 UTC

[GitHub] [kafka] gnkoshelev opened a new pull request #8715: KAFKA-10033: AdminClient should throw UnknownTopicOrPartitionException instead of UnknownServerException if altering configs of non-existing topic

gnkoshelev opened a new pull request #8715:
URL: https://github.com/apache/kafka/pull/8715


   Fixes KAFKA-10033.
   
   Replace AdminOperationException with UnknownTopicOrPartitionException if topic does not exists when validating altering configs of topic in AdminZkClient.


----------------------------------------------------------------
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] omkreddy closed pull request #8715: KAFKA-10033: AdminClient should throw UnknownTopicOrPartitionException instead of UnknownServerException if altering configs of non-existing topic

Posted by GitBox <gi...@apache.org>.
omkreddy closed pull request #8715:
URL: https://github.com/apache/kafka/pull/8715


   


----------------------------------------------------------------
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] bdbyrne commented on pull request #8715: KAFKA-10033: AdminClient should throw UnknownTopicOrPartitionException instead of UnknownServerException if altering configs of non-existing topic

Posted by GitBox <gi...@apache.org>.
bdbyrne commented on pull request #8715:
URL: https://github.com/apache/kafka/pull/8715#issuecomment-632799086


   Hi @gnkoshelev - it's assumed the topic has been validated by the time it attempts to alter its config via AdminZKClient. I've posted the proper fix at https://github.com/apache/kafka/pull/8717. Thanks for finding and reporting!


----------------------------------------------------------------
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] gnkoshelev commented on pull request #8715: KAFKA-10033: AdminClient should throw UnknownTopicOrPartitionException instead of UnknownServerException if altering configs of non-existing topic

Posted by GitBox <gi...@apache.org>.
gnkoshelev commented on pull request #8715:
URL: https://github.com/apache/kafka/pull/8715#issuecomment-632900624


   >Be sure to update `core/src/test/scala/unit/kafka/server/DynamicConfigChangeTest.scala` to the new exception.
   
   My bad - You're absolutely right.
   
   >it'll use the metadata cache to cull unknown topics before hitting ZK.
   
   As for me, **Fail fast** is a good one approach. 👍 


----------------------------------------------------------------
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] omkreddy commented on pull request #8715: KAFKA-10033: AdminClient should throw UnknownTopicOrPartitionException instead of UnknownServerException if altering configs of non-existing topic

Posted by GitBox <gi...@apache.org>.
omkreddy commented on pull request #8715:
URL: https://github.com/apache/kafka/pull/8715#issuecomment-639031210


   @gnkoshelev Thanks for the PR.  LGTM. I think, we can merge both PRs  (#8717 ). `AdminZkClient.validateTopicConfig()` is used in command line tools (This will go when remove ZK dependency). we may want throw same error in all places.


----------------------------------------------------------------
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] gnkoshelev commented on pull request #8715: KAFKA-10033: AdminClient should throw UnknownTopicOrPartitionException instead of UnknownServerException if altering configs of non-existing topic

Posted by GitBox <gi...@apache.org>.
gnkoshelev commented on pull request #8715:
URL: https://github.com/apache/kafka/pull/8715#issuecomment-632864284


   Hi @bdbyrne,
   If so what do you say to
   ```
   def validateTopicCreate(topic: String,
                             partitionReplicaAssignment: Map[Int, Seq[Int]],
                             config: Properties): Unit = {
     /* ... */
     if (zkClient.topicExists(topic))
           throw new TopicExistsException(s"Topic '$topic' already exists.")
     /* ... */
   }
   ```
   or
   ```
     def deleteTopic(topic: String): Unit = {
       if (zkClient.topicExists(topic)) {
         /* ... */
       } else {
         throw new UnknownTopicOrPartitionException(s"Topic `$topic` to delete does not exist")
       }
     }
   ```
   ?  
   
   
   It seems to me that the `AdminZkClient.validateTopicConfig` should throw as specific exception as possible. Also, it preserves current style as in the examples above. 


----------------------------------------------------------------
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] omkreddy commented on pull request #8715: KAFKA-10033: AdminClient should throw UnknownTopicOrPartitionException instead of UnknownServerException if altering configs of non-existing topic

Posted by GitBox <gi...@apache.org>.
omkreddy commented on pull request #8715:
URL: https://github.com/apache/kafka/pull/8715#issuecomment-638386016


   ok to 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] bdbyrne commented on pull request #8715: KAFKA-10033: AdminClient should throw UnknownTopicOrPartitionException instead of UnknownServerException if altering configs of non-existing topic

Posted by GitBox <gi...@apache.org>.
bdbyrne commented on pull request #8715:
URL: https://github.com/apache/kafka/pull/8715#issuecomment-632871693


   Hi @gnkoshelev, 
   
   That's a fair point. It's odd that `AdminOperationException` was chosen there, and appears to go back to 2014, if not earlier. Be sure to update `core/src/test/scala/unit/kafka/server/DynamicConfigChangeTest.scala` to the new exception.
   
   I'll have to let a committer make the call on this one. If not, there's also #8717 that has the (very small) benefit that it'll use the metadata cache to cull unknown topics before hitting ZK. 


----------------------------------------------------------------
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] gnkoshelev commented on pull request #8715: KAFKA-10033: AdminClient should throw UnknownTopicOrPartitionException instead of UnknownServerException if altering configs of non-existing topic

Posted by GitBox <gi...@apache.org>.
gnkoshelev commented on pull request #8715:
URL: https://github.com/apache/kafka/pull/8715#issuecomment-633305898


   Hi, @ijuma,
   You was one of reviewers of `AdminZkClient`. Can you review the PR?


----------------------------------------------------------------
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