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/12/22 10:14:41 UTC
[GitHub] [kafka] g1geordie opened a new pull request #9778: KAFKA-10874 Fix flaky ClientQuotasRequestTest.testAlterIpQuotasRequest
g1geordie opened a new pull request #9778:
URL: https://github.com/apache/kafka/pull/9778
The error occcur when dynamic ip config update after borker's config get .
solve this by waiting dynamic ip config update.
----------------------------------------------------------------
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] g1geordie commented on a change in pull request #9778: KAFKA-10874 Fix flaky ClientQuotasRequestTest.testAlterIpQuotasRequest
Posted by GitBox <gi...@apache.org>.
g1geordie commented on a change in pull request #9778:
URL: https://github.com/apache/kafka/pull/9778#discussion_r551467279
##########
File path: core/src/test/scala/unit/kafka/server/ClientQuotasRequestTest.scala
##########
@@ -212,7 +214,9 @@ class ClientQuotasRequestTest extends BaseRequestTest {
InetAddress.getByName(unknownHost)
else
InetAddress.getByName(entityName)
- assertEquals(expectedMatches(entity), servers.head.socketServer.connectionQuotas.connectionRateForIp(entityIp), 0.01)
+ TestUtils.waitUntilTrue(
+ () => expectedMatches(entity) - servers.head.socketServer.connectionQuotas.connectionRateForIp(entityIp) < 0.01
+ ,"Broker didn't update quotas from Zookeeper")
Review comment:
@dajac
hello~
I remain the message `Broker didn't update quotas from Zookeeper` which clearly indicate the error.
And add `expect` and `got` to error message
Can you help me take a look 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] dajac commented on a change in pull request #9778: KAFKA-10874 Fix flaky ClientQuotasRequestTest.testAlterIpQuotasRequest
Posted by GitBox <gi...@apache.org>.
dajac commented on a change in pull request #9778:
URL: https://github.com/apache/kafka/pull/9778#discussion_r551298159
##########
File path: core/src/test/scala/unit/kafka/server/ClientQuotasRequestTest.scala
##########
@@ -212,7 +214,9 @@ class ClientQuotasRequestTest extends BaseRequestTest {
InetAddress.getByName(unknownHost)
else
InetAddress.getByName(entityName)
- assertEquals(expectedMatches(entity), servers.head.socketServer.connectionQuotas.connectionRateForIp(entityIp), 0.01)
+ TestUtils.waitUntilTrue(
+ () => expectedMatches(entity) - servers.head.socketServer.connectionQuotas.connectionRateForIp(entityIp) < 0.01
+ ,"Broker didn't update quotas from Zookeeper")
Review comment:
* We usually put the `,` at the end of the previous line when we break a line.
* Could we add the expected value and the current value in the message? We could say something like `Expected connection quota $expected for $entity, got $actual.`
----------------------------------------------------------------
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 #9778: KAFKA-10874 Fix flaky ClientQuotasRequestTest.testAlterIpQuotasRequest
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #9778:
URL: https://github.com/apache/kafka/pull/9778#discussion_r548375601
##########
File path: core/src/test/scala/unit/kafka/server/ClientQuotasRequestTest.scala
##########
@@ -212,7 +214,9 @@ class ClientQuotasRequestTest extends BaseRequestTest {
InetAddress.getByName(unknownHost)
else
InetAddress.getByName(entityName)
- assertEquals(expectedMatches(entity), servers.head.socketServer.connectionQuotas.connectionRateForIp(entityIp), 0.01)
+ TestUtils.retry(10000L) {
Review comment:
Could you add comment for this retry?
----------------------------------------------------------------
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 #9778: KAFKA-10874 Fix flaky ClientQuotasRequestTest.testAlterIpQuotasRequest
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #9778:
URL: https://github.com/apache/kafka/pull/9778#discussion_r549221378
##########
File path: core/src/test/scala/unit/kafka/server/ClientQuotasRequestTest.scala
##########
@@ -212,7 +214,10 @@ class ClientQuotasRequestTest extends BaseRequestTest {
InetAddress.getByName(unknownHost)
else
InetAddress.getByName(entityName)
- assertEquals(expectedMatches(entity), servers.head.socketServer.connectionQuotas.connectionRateForIp(entityIp), 0.01)
+ TestUtils.retry(10000L) {
Review comment:
It seems to me ```waitUntilTrue``` is more suitable since the ```exception``` is not necessary in this test 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] g1geordie commented on a change in pull request #9778: KAFKA-10874 Fix flaky ClientQuotasRequestTest.testAlterIpQuotasRequest
Posted by GitBox <gi...@apache.org>.
g1geordie commented on a change in pull request #9778:
URL: https://github.com/apache/kafka/pull/9778#discussion_r551408267
##########
File path: core/src/test/scala/unit/kafka/server/ClientQuotasRequestTest.scala
##########
@@ -212,7 +214,9 @@ class ClientQuotasRequestTest extends BaseRequestTest {
InetAddress.getByName(unknownHost)
else
InetAddress.getByName(entityName)
- assertEquals(expectedMatches(entity), servers.head.socketServer.connectionQuotas.connectionRateForIp(entityIp), 0.01)
+ TestUtils.waitUntilTrue(
+ () => expectedMatches(entity) - servers.head.socketServer.connectionQuotas.connectionRateForIp(entityIp) < 0.01
+ ,"Broker didn't update quotas from Zookeeper")
Review comment:
Thank you ! Both are very helpful . let me fix 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] dajac commented on a change in pull request #9778: KAFKA-10874 Fix flaky ClientQuotasRequestTest.testAlterIpQuotasRequest
Posted by GitBox <gi...@apache.org>.
dajac commented on a change in pull request #9778:
URL: https://github.com/apache/kafka/pull/9778#discussion_r552704458
##########
File path: core/src/test/scala/unit/kafka/server/ClientQuotasRequestTest.scala
##########
@@ -212,7 +214,9 @@ class ClientQuotasRequestTest extends BaseRequestTest {
InetAddress.getByName(unknownHost)
else
InetAddress.getByName(entityName)
- assertEquals(expectedMatches(entity), servers.head.socketServer.connectionQuotas.connectionRateForIp(entityIp), 0.01)
+ TestUtils.waitUntilTrue(
+ () => expectedMatches(entity) - servers.head.socketServer.connectionQuotas.connectionRateForIp(entityIp) < 0.01
+ ,"Broker didn't update quotas from Zookeeper")
Review comment:
Thanks for the update. I think that we should not mention ZK here as ZK is going away. How about `Connection quota of $entity is not $expected but $actual`?
----------------------------------------------------------------
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] g1geordie commented on pull request #9778: KAFKA-10874 Fix flaky ClientQuotasRequestTest.testAlterIpQuotasRequest
Posted by GitBox <gi...@apache.org>.
g1geordie commented on pull request #9778:
URL: https://github.com/apache/kafka/pull/9778#issuecomment-749958296
@chia7712 thank for you comment . help me take a look 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] g1geordie commented on a change in pull request #9778: KAFKA-10874 Fix flaky ClientQuotasRequestTest.testAlterIpQuotasRequest
Posted by GitBox <gi...@apache.org>.
g1geordie commented on a change in pull request #9778:
URL: https://github.com/apache/kafka/pull/9778#discussion_r549631973
##########
File path: core/src/test/scala/unit/kafka/server/ClientQuotasRequestTest.scala
##########
@@ -212,7 +214,9 @@ class ClientQuotasRequestTest extends BaseRequestTest {
InetAddress.getByName(unknownHost)
else
InetAddress.getByName(entityName)
- assertEquals(expectedMatches(entity), servers.head.socketServer.connectionQuotas.connectionRateForIp(entityIp), 0.01)
+ TestUtils.waitUntilTrue(
+ () => expectedMatches(entity) - servers.head.socketServer.connectionQuotas.connectionRateForIp(entityIp) < 0.01
+ ,"Broker didn't update prop from Zookeeper")
Review comment:
sure!!
----------------------------------------------------------------
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] g1geordie commented on pull request #9778: KAFKA-10874 Fix flaky ClientQuotasRequestTest.testAlterIpQuotasRequest
Posted by GitBox <gi...@apache.org>.
g1geordie commented on pull request #9778:
URL: https://github.com/apache/kafka/pull/9778#issuecomment-749685636
@chia7712 can you help my 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] g1geordie commented on a change in pull request #9778: KAFKA-10874 Fix flaky ClientQuotasRequestTest.testAlterIpQuotasRequest
Posted by GitBox <gi...@apache.org>.
g1geordie commented on a change in pull request #9778:
URL: https://github.com/apache/kafka/pull/9778#discussion_r547421729
##########
File path: core/src/test/scala/unit/kafka/server/ClientQuotasRequestTest.scala
##########
@@ -200,7 +202,7 @@ class ClientQuotasRequestTest extends BaseRequestTest {
val defaultEntityFilter = ClientQuotaFilterComponent.ofDefaultEntity(ClientQuotaEntity.IP)
val allIpEntityFilter = ClientQuotaFilterComponent.ofEntityType(ClientQuotaEntity.IP)
- def verifyIpQuotas(entityFilter: ClientQuotaFilterComponent, expectedMatches: Map[ClientQuotaEntity, Double]): Unit = {
+ def verifyIpQuotas(entityFilter: ClientQuotaFilterComponent, expectedMatches: Map[ClientQuotaEntity, Double]): Unit = TestUtils.retry(10000L){
Review comment:
yes , I will change to only retry `assertEquals(expectedMatches(entity), servers.head.socketServer.connectionQuotas.connectionRateForIp(entityIp), 0.01)`
----------------------------------------------------------------
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 #9778: KAFKA-10874 Fix flaky ClientQuotasRequestTest.testAlterIpQuotasRequest
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #9778:
URL: https://github.com/apache/kafka/pull/9778#discussion_r547415122
##########
File path: core/src/test/scala/unit/kafka/server/ClientQuotasRequestTest.scala
##########
@@ -200,7 +202,7 @@ class ClientQuotasRequestTest extends BaseRequestTest {
val defaultEntityFilter = ClientQuotaFilterComponent.ofDefaultEntity(ClientQuotaEntity.IP)
val allIpEntityFilter = ClientQuotaFilterComponent.ofEntityType(ClientQuotaEntity.IP)
- def verifyIpQuotas(entityFilter: ClientQuotaFilterComponent, expectedMatches: Map[ClientQuotaEntity, Double]): Unit = {
+ def verifyIpQuotas(entityFilter: ClientQuotaFilterComponent, expectedMatches: Map[ClientQuotaEntity, Double]): Unit = TestUtils.retry(10000L){
Review comment:
Is it overkill to retry whole 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] g1geordie commented on a change in pull request #9778: KAFKA-10874 Fix flaky ClientQuotasRequestTest.testAlterIpQuotasRequest
Posted by GitBox <gi...@apache.org>.
g1geordie commented on a change in pull request #9778:
URL: https://github.com/apache/kafka/pull/9778#discussion_r552816376
##########
File path: core/src/test/scala/unit/kafka/server/ClientQuotasRequestTest.scala
##########
@@ -212,7 +214,9 @@ class ClientQuotasRequestTest extends BaseRequestTest {
InetAddress.getByName(unknownHost)
else
InetAddress.getByName(entityName)
- assertEquals(expectedMatches(entity), servers.head.socketServer.connectionQuotas.connectionRateForIp(entityIp), 0.01)
+ TestUtils.waitUntilTrue(
+ () => expectedMatches(entity) - servers.head.socketServer.connectionQuotas.connectionRateForIp(entityIp) < 0.01
+ ,"Broker didn't update quotas from Zookeeper")
Review comment:
Thank you ! I have fixed 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] dajac merged pull request #9778: KAFKA-10874 Fix flaky ClientQuotasRequestTest.testAlterIpQuotasRequest
Posted by GitBox <gi...@apache.org>.
dajac merged pull request #9778:
URL: https://github.com/apache/kafka/pull/9778
----------------------------------------------------------------
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] g1geordie commented on a change in pull request #9778: KAFKA-10874 Fix flaky ClientQuotasRequestTest.testAlterIpQuotasRequest
Posted by GitBox <gi...@apache.org>.
g1geordie commented on a change in pull request #9778:
URL: https://github.com/apache/kafka/pull/9778#discussion_r549545680
##########
File path: core/src/test/scala/unit/kafka/server/ClientQuotasRequestTest.scala
##########
@@ -212,7 +214,10 @@ class ClientQuotasRequestTest extends BaseRequestTest {
InetAddress.getByName(unknownHost)
else
InetAddress.getByName(entityName)
- assertEquals(expectedMatches(entity), servers.head.socketServer.connectionQuotas.connectionRateForIp(entityIp), 0.01)
+ TestUtils.retry(10000L) {
Review comment:
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] g1geordie edited a comment on pull request #9778: KAFKA-10874 Fix flaky ClientQuotasRequestTest.testAlterIpQuotasRequest
Posted by GitBox <gi...@apache.org>.
g1geordie edited a comment on pull request #9778:
URL: https://github.com/apache/kafka/pull/9778#issuecomment-749958296
@chia7712 thank for you comment . please help me take a look 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] g1geordie commented on a change in pull request #9778: KAFKA-10874 Fix flaky ClientQuotasRequestTest.testAlterIpQuotasRequest
Posted by GitBox <gi...@apache.org>.
g1geordie commented on a change in pull request #9778:
URL: https://github.com/apache/kafka/pull/9778#discussion_r548427248
##########
File path: core/src/test/scala/unit/kafka/server/ClientQuotasRequestTest.scala
##########
@@ -212,7 +214,9 @@ class ClientQuotasRequestTest extends BaseRequestTest {
InetAddress.getByName(unknownHost)
else
InetAddress.getByName(entityName)
- assertEquals(expectedMatches(entity), servers.head.socketServer.connectionQuotas.connectionRateForIp(entityIp), 0.01)
+ TestUtils.retry(10000L) {
Review comment:
Yes sir
----------------------------------------------------------------
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 #9778: KAFKA-10874 Fix flaky ClientQuotasRequestTest.testAlterIpQuotasRequest
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #9778:
URL: https://github.com/apache/kafka/pull/9778#discussion_r549598781
##########
File path: core/src/test/scala/unit/kafka/server/ClientQuotasRequestTest.scala
##########
@@ -212,7 +214,9 @@ class ClientQuotasRequestTest extends BaseRequestTest {
InetAddress.getByName(unknownHost)
else
InetAddress.getByName(entityName)
- assertEquals(expectedMatches(entity), servers.head.socketServer.connectionQuotas.connectionRateForIp(entityIp), 0.01)
+ TestUtils.waitUntilTrue(
+ () => expectedMatches(entity) - servers.head.socketServer.connectionQuotas.connectionRateForIp(entityIp) < 0.01
+ ,"Broker didn't update prop from Zookeeper")
Review comment:
Could we rename "prop" to "quotas"?
----------------------------------------------------------------
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