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/08/02 18:50:31 UTC

[GitHub] [kafka] RamanVerma opened a new pull request #9112: KAFKA-10312 Fix error code returned by getPartitionMetadata

RamanVerma opened a new pull request #9112:
URL: https://github.com/apache/kafka/pull/9112


   MetadataCache#getPartitionMetadata returns an error when the topic's leader Id
   is present at MetadataCache but listener endpoint is not present for this leader.
   For older version, LEADER_NOT_AVAILABLE is returned while LISTENER_NOT_FOUND is
   returned for new metadata version.
   
   getPartitionMetadata was looking up MetadataCache's host brokerId rather
   than the topic's leader id while determining what error to return. This
   could result in the call returning LISTENER_NOT_FOUND when it should
   have returned LEADER_NOT_AVAILABLE. This commit corrects this behavior.
   
   Unit tests were already present to test out the error codes returned
   under different situations but they were giving out a false positive.
   The test was using same broker id for both the MetadataCache's host as
   well as for the topic's leader. Error manifests when the MetadataCache's
   host id is changed. Improved the test.
   
   This commit also consolidated couple of related tests to reduce code
   duplication.
   
   *More detailed description of your change,
   if necessary. The PR title and PR message become
   the squashed commit message, so use a separate
   comment to ping reviewers.*
   
   *Summary of testing strategy (including rationale)
   for the feature or bug fix. Unit and/or integration
   tests are expected for any behaviour change and
   system tests should be considered for larger changes.*
   
   ### 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] hachikuji commented on a change in pull request #9112: KAFKA-10312 Fix error code returned by getPartitionMetadata

Posted by GitBox <gi...@apache.org>.
hachikuji commented on a change in pull request #9112:
URL: https://github.com/apache/kafka/pull/9112#discussion_r468761215



##########
File path: core/src/test/scala/unit/kafka/server/MetadataCacheTest.scala
##########
@@ -187,49 +191,24 @@ class MetadataCacheTest {
       new UpdateMetadataBroker()
         .setId(1)
         .setEndpoints(broker1Endpoints.asJava))
-    verifyTopicMetadataPartitionLeaderOrEndpointNotAvailable(brokers, sslListenerName,
+    val metadataCacheBrokerId = 9

Review comment:
       Would 0 work just as well? It would make the test more coherent if we used one of the ids included in the UpdateMetadata request.




----------------------------------------------------------------
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] RamanVerma commented on pull request #9112: KAFKA-10312 Fix error code returned by getPartitionMetadata

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


   cc @hachikuji 
   Not sure why I am not able to add anyone as a reviewer to the PR. I am not getting the add reviewers/recommended reviewers options.


----------------------------------------------------------------
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] hachikuji merged pull request #9112: KAFKA-10312 Fix error code returned by getPartitionMetadata

Posted by GitBox <gi...@apache.org>.
hachikuji merged pull request #9112:
URL: https://github.com/apache/kafka/pull/9112


   


----------------------------------------------------------------
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] hachikuji commented on pull request #9112: KAFKA-10312 Fix error code returned by getPartitionMetadata

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


   retest this please


----------------------------------------------------------------
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] hachikuji commented on pull request #9112: KAFKA-10312 Fix error code returned by getPartitionMetadata

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


   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] RamanVerma commented on a change in pull request #9112: KAFKA-10312 Fix error code returned by getPartitionMetadata

Posted by GitBox <gi...@apache.org>.
RamanVerma commented on a change in pull request #9112:
URL: https://github.com/apache/kafka/pull/9112#discussion_r469698016



##########
File path: core/src/test/scala/unit/kafka/server/MetadataCacheTest.scala
##########
@@ -187,49 +191,24 @@ class MetadataCacheTest {
       new UpdateMetadataBroker()
         .setId(1)
         .setEndpoints(broker1Endpoints.asJava))
-    verifyTopicMetadataPartitionLeaderOrEndpointNotAvailable(brokers, sslListenerName,
+    val metadataCacheBrokerId = 9

Review comment:
       Done




----------------------------------------------------------------
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] hachikuji commented on a change in pull request #9112: KAFKA-10312 Fix error code returned by getPartitionMetadata

Posted by GitBox <gi...@apache.org>.
hachikuji commented on a change in pull request #9112:
URL: https://github.com/apache/kafka/pull/9112#discussion_r468270027



##########
File path: core/src/test/scala/unit/kafka/server/MetadataCacheTest.scala
##########
@@ -229,7 +205,7 @@ class MetadataCacheTest {
                                                                        errorUnavailableListeners: Boolean): Unit = {
     val topic = "topic"
 
-    val cache = new MetadataCache(1)
+    val cache = new MetadataCache(9)

Review comment:
       nit: maybe it would be clearer if we passed `brokerId` as an argument? Then we could use broker 0 in the test case, for example, so that the UpdateMetadata request makes more sense.




----------------------------------------------------------------
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] RamanVerma commented on a change in pull request #9112: KAFKA-10312 Fix error code returned by getPartitionMetadata

Posted by GitBox <gi...@apache.org>.
RamanVerma commented on a change in pull request #9112:
URL: https://github.com/apache/kafka/pull/9112#discussion_r468377991



##########
File path: core/src/test/scala/unit/kafka/server/MetadataCacheTest.scala
##########
@@ -229,7 +205,7 @@ class MetadataCacheTest {
                                                                        errorUnavailableListeners: Boolean): Unit = {
     val topic = "topic"
 
-    val cache = new MetadataCache(1)
+    val cache = new MetadataCache(9)

Review comment:
       @hachikuji done




----------------------------------------------------------------
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] RamanVerma commented on pull request #9112: KAFKA-10312 Fix error code returned by getPartitionMetadata

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


   Ran `./gradlew core:test` locally. All tests passed.


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