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/11/24 11:21:38 UTC
[GitHub] [kafka] dengziming opened a new pull request #9649: MINOR: combine repeated top level test of AlterIsrManagerTest
dengziming opened a new pull request #9649:
URL: https://github.com/apache/kafka/pull/9649
The content of the 3 top-level errors are identical, just merge them.
----------------------------------------------------------------
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 #9649: MINOR: Combine repeated top-level error test in AlterIsrManagerTest
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #9649:
URL: https://github.com/apache/kafka/pull/9649#discussion_r551683555
##########
File path: core/src/test/scala/unit/kafka/server/AlterIsrManagerTest.scala
##########
@@ -174,13 +172,24 @@ class AlterIsrManagerTest {
}
@Test
- def testPartitionErrors(): Unit = {
- val errors = Seq(Errors.INVALID_UPDATE_VERSION, Errors.UNKNOWN_TOPIC_OR_PARTITION, Errors.NOT_LEADER_OR_FOLLOWER)
- errors.foreach(error => {
- val alterIsrManager = testPartitionError(tp0, error)
- // Any partition-level error should clear the item from the pending queue allowing for future updates
- assertTrue(alterIsrManager.submit(AlterIsrItem(tp0, null, _ => { }, 0)))
- })
+ def testInvalidUpdateVersion(): Unit = {
+ checkPartitionError(Errors.INVALID_UPDATE_VERSION)
+ }
+
+ @Test
+ def testUnknownTopicPartition(): Unit = {
+ checkPartitionError(Errors.UNKNOWN_TOPIC_OR_PARTITION)
+ }
+
+ @Test
+ def testNotLeaderOrFollower(): Unit = {
+ checkPartitionError(Errors.NOT_LEADER_OR_FOLLOWER)
+ }
+
+ def checkPartitionError(error: Errors): Unit = {
Review comment:
ditto
##########
File path: core/src/test/scala/unit/kafka/server/AlterIsrManagerTest.scala
##########
@@ -127,26 +127,24 @@ class AlterIsrManagerTest {
@Test
def testAuthorizationFailed(): Unit = {
- val isrs = Seq(AlterIsrItem(tp0, new LeaderAndIsr(1, 1, List(1,2,3), 10), _ => { }, 0))
- val manager = testTopLevelError(isrs, Errors.CLUSTER_AUTHORIZATION_FAILED)
- // On authz error, we log the exception and keep retrying
- assertFalse(manager.submit(AlterIsrItem(tp0, null, _ => { }, 0)))
+ checkTopLevelError(Errors.CLUSTER_AUTHORIZATION_FAILED)
}
@Test
def testStaleBrokerEpoch(): Unit = {
- val isrs = Seq(AlterIsrItem(tp0, new LeaderAndIsr(1, 1, List(1,2,3), 10), _ => { }, 0))
- val manager = testTopLevelError(isrs, Errors.STALE_BROKER_EPOCH)
- // On stale broker epoch, we want to retry, so we don't clear items from the pending map
- assertFalse(manager.submit(AlterIsrItem(tp0, null, _ => { }, 0)))
+ checkTopLevelError(Errors.STALE_BROKER_EPOCH)
}
@Test
- def testOtherErrors(): Unit = {
+ def testUnknownServer(): Unit = {
+ checkTopLevelError(Errors.UNKNOWN_SERVER_ERROR)
+ }
+
+ def checkTopLevelError(error: Errors): Unit = {
Review comment:
Could you change it to ```private``` method?
----------------------------------------------------------------
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] dengziming commented on pull request #9649: MINOR: Combine repeated top-level error test in AlterIsrManagerTest
Posted by GitBox <gi...@apache.org>.
dengziming commented on pull request #9649:
URL: https://github.com/apache/kafka/pull/9649#issuecomment-739116990
@mumrah Hi, this class is created by you so I think you can have a look, thank you.
----------------------------------------------------------------
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 #9649: MINOR: Combine repeated top-level error test in AlterIsrManagerTest
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #9649:
URL: https://github.com/apache/kafka/pull/9649#issuecomment-755185322
I will merge this PR tomorrow if there is no objection. @mumrah 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] dengziming commented on pull request #9649: MINOR: Combine repeated top-level error test in AlterIsrManagerTest
Posted by GitBox <gi...@apache.org>.
dengziming commented on pull request #9649:
URL: https://github.com/apache/kafka/pull/9649#issuecomment-745013161
@mumrah Hi, Davad, PTAL.
----------------------------------------------------------------
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] dengziming commented on pull request #9649: MINOR: Combine repeated top-level error test in AlterIsrManagerTest
Posted by GitBox <gi...@apache.org>.
dengziming commented on pull request #9649:
URL: https://github.com/apache/kafka/pull/9649#issuecomment-754638476
@chia7712 I tried to rebase my pr and push it again, but still failed. this is weird, maybe I triggered a bug of jenkins and should tried again tomorrow.
----------------------------------------------------------------
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] dengziming edited a comment on pull request #9649: MINOR: Combine repeated top-level error test in AlterIsrManagerTest
Posted by GitBox <gi...@apache.org>.
dengziming edited a comment on pull request #9649:
URL: https://github.com/apache/kafka/pull/9649#issuecomment-745013161
@mumrah Hi, David, PTAL.
----------------------------------------------------------------
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] mumrah merged pull request #9649: MINOR: Combine repeated top-level error test in AlterIsrManagerTest
Posted by GitBox <gi...@apache.org>.
mumrah merged pull request #9649:
URL: https://github.com/apache/kafka/pull/9649
----------------------------------------------------------------
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] dengziming commented on a change in pull request #9649: MINOR: Combine repeated top-level error test in AlterIsrManagerTest
Posted by GitBox <gi...@apache.org>.
dengziming commented on a change in pull request #9649:
URL: https://github.com/apache/kafka/pull/9649#discussion_r551722375
##########
File path: core/src/test/scala/unit/kafka/server/AlterIsrManagerTest.scala
##########
@@ -174,13 +172,24 @@ class AlterIsrManagerTest {
}
@Test
- def testPartitionErrors(): Unit = {
- val errors = Seq(Errors.INVALID_UPDATE_VERSION, Errors.UNKNOWN_TOPIC_OR_PARTITION, Errors.NOT_LEADER_OR_FOLLOWER)
- errors.foreach(error => {
- val alterIsrManager = testPartitionError(tp0, error)
- // Any partition-level error should clear the item from the pending queue allowing for future updates
- assertTrue(alterIsrManager.submit(AlterIsrItem(tp0, null, _ => { }, 0)))
- })
+ def testInvalidUpdateVersion(): Unit = {
+ checkPartitionError(Errors.INVALID_UPDATE_VERSION)
+ }
+
+ @Test
+ def testUnknownTopicPartition(): Unit = {
+ checkPartitionError(Errors.UNKNOWN_TOPIC_OR_PARTITION)
+ }
+
+ @Test
+ def testNotLeaderOrFollower(): Unit = {
+ checkPartitionError(Errors.NOT_LEADER_OR_FOLLOWER)
+ }
+
+ def checkPartitionError(error: Errors): Unit = {
Review comment:
Thanks, 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] dengziming commented on a change in pull request #9649: MINOR: Combine repeated top-level error test in AlterIsrManagerTest
Posted by GitBox <gi...@apache.org>.
dengziming commented on a change in pull request #9649:
URL: https://github.com/apache/kafka/pull/9649#discussion_r551719940
##########
File path: core/src/test/scala/unit/kafka/server/AlterIsrManagerTest.scala
##########
@@ -174,13 +172,24 @@ class AlterIsrManagerTest {
}
@Test
- def testPartitionErrors(): Unit = {
- val errors = Seq(Errors.INVALID_UPDATE_VERSION, Errors.UNKNOWN_TOPIC_OR_PARTITION, Errors.NOT_LEADER_OR_FOLLOWER)
- errors.foreach(error => {
- val alterIsrManager = testPartitionError(tp0, error)
- // Any partition-level error should clear the item from the pending queue allowing for future updates
- assertTrue(alterIsrManager.submit(AlterIsrItem(tp0, null, _ => { }, 0)))
- })
+ def testInvalidUpdateVersion(): Unit = {
+ checkPartitionError(Errors.INVALID_UPDATE_VERSION)
+ }
+
+ @Test
+ def testUnknownTopicPartition(): Unit = {
+ checkPartitionError(Errors.UNKNOWN_TOPIC_OR_PARTITION)
+ }
+
+ @Test
+ def testNotLeaderOrFollower(): Unit = {
+ checkPartitionError(Errors.NOT_LEADER_OR_FOLLOWER)
+ }
+
+ def checkPartitionError(error: Errors): Unit = {
Review comment:
Thanks, 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] dengziming commented on a change in pull request #9649: MINOR: Combine repeated top-level error test in AlterIsrManagerTest
Posted by GitBox <gi...@apache.org>.
dengziming commented on a change in pull request #9649:
URL: https://github.com/apache/kafka/pull/9649#discussion_r551722375
##########
File path: core/src/test/scala/unit/kafka/server/AlterIsrManagerTest.scala
##########
@@ -174,13 +172,24 @@ class AlterIsrManagerTest {
}
@Test
- def testPartitionErrors(): Unit = {
- val errors = Seq(Errors.INVALID_UPDATE_VERSION, Errors.UNKNOWN_TOPIC_OR_PARTITION, Errors.NOT_LEADER_OR_FOLLOWER)
- errors.foreach(error => {
- val alterIsrManager = testPartitionError(tp0, error)
- // Any partition-level error should clear the item from the pending queue allowing for future updates
- assertTrue(alterIsrManager.submit(AlterIsrItem(tp0, null, _ => { }, 0)))
- })
+ def testInvalidUpdateVersion(): Unit = {
+ checkPartitionError(Errors.INVALID_UPDATE_VERSION)
+ }
+
+ @Test
+ def testUnknownTopicPartition(): Unit = {
+ checkPartitionError(Errors.UNKNOWN_TOPIC_OR_PARTITION)
+ }
+
+ @Test
+ def testNotLeaderOrFollower(): Unit = {
+ checkPartitionError(Errors.NOT_LEADER_OR_FOLLOWER)
+ }
+
+ def checkPartitionError(error: Errors): Unit = {
Review comment:
Thanks, 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] mumrah commented on a change in pull request #9649: MINOR: Combine repeated top-level error test in AlterIsrManagerTest
Posted by GitBox <gi...@apache.org>.
mumrah commented on a change in pull request #9649:
URL: https://github.com/apache/kafka/pull/9649#discussion_r551414004
##########
File path: core/src/test/scala/unit/kafka/server/AlterIsrManagerTest.scala
##########
@@ -127,26 +127,24 @@ class AlterIsrManagerTest {
@Test
def testAuthorizationFailed(): Unit = {
- val isrs = Seq(AlterIsrItem(tp0, new LeaderAndIsr(1, 1, List(1,2,3), 10), _ => { }, 0))
- val manager = testTopLevelError(isrs, Errors.CLUSTER_AUTHORIZATION_FAILED)
- // On authz error, we log the exception and keep retrying
- assertFalse(manager.submit(AlterIsrItem(tp0, null, _ => { }, 0)))
+ testTopLevelError(Errors.CLUSTER_AUTHORIZATION_FAILED)
}
@Test
def testStaleBrokerEpoch(): Unit = {
- val isrs = Seq(AlterIsrItem(tp0, new LeaderAndIsr(1, 1, List(1,2,3), 10), _ => { }, 0))
- val manager = testTopLevelError(isrs, Errors.STALE_BROKER_EPOCH)
- // On stale broker epoch, we want to retry, so we don't clear items from the pending map
- assertFalse(manager.submit(AlterIsrItem(tp0, null, _ => { }, 0)))
+ testTopLevelError(Errors.STALE_BROKER_EPOCH)
}
@Test
- def testOtherErrors(): Unit = {
+ def testUnknownServer(): Unit = {
+ testTopLevelError(Errors.UNKNOWN_SERVER_ERROR)
+ }
+
+ def testTopLevelError(error: Errors): Unit = {
Review comment:
Maybe rename this and "testPartitionError" to something slightly different so they don't look like actual test cases? Maybe something like "assertTopLevelError" or "checkTopLevelError".
----------------------------------------------------------------
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] dengziming commented on pull request #9649: MINOR: Combine repeated top-level error test in AlterIsrManagerTest
Posted by GitBox <gi...@apache.org>.
dengziming commented on pull request #9649:
URL: https://github.com/apache/kafka/pull/9649#issuecomment-752820081
@chia7712 ,Thank you for your suggestion. In fact, I get the idea from `AlterIsrManagerTest.testPartitionErrors` which test 3 errors in a method, so I also merge the 3 top errors in a method. I think you offer a better idea and I also changed the 3 partition level error.
----------------------------------------------------------------
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 #9649: MINOR: Combine repeated top-level error test in AlterIsrManagerTest
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #9649:
URL: https://github.com/apache/kafka/pull/9649#issuecomment-754633864
@dengziming Could you 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] dengziming commented on pull request #9649: MINOR: Combine repeated top-level error test in AlterIsrManagerTest
Posted by GitBox <gi...@apache.org>.
dengziming commented on pull request #9649:
URL: https://github.com/apache/kafka/pull/9649#issuecomment-733448084
@cmccabe hi, PTAL.
----------------------------------------------------------------
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