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/12 22:48:01 UTC

[GitHub] [kafka] splett2 opened a new pull request #9417: MINOR: Fix flaky testQuotaMetric

splett2 opened a new pull request #9417:
URL: https://github.com/apache/kafka/pull/9417


   ### What
   `ClientQuotaManager.updateQuota` updates `quotaCallback` before updating metric configs.
   `ClientQuotaManager.quota()` performs an unlocked call to `quotaLimit()`, so we can run into a case where `quota` returns the updated quota, but the metric config hasn't been updated.
   
   The fix is to acquire the read lock before attempting to read the quota. An alternative would be to have `verifyQuotaMetric` run in a `waitUntilTrue`. 
   
   ### 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] dajac merged pull request #9417: MINOR: Fix flaky ControllerMutationQuotaTest.testQuotaMetric

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


   


----------------------------------------------------------------
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 #9417: MINOR: Fix flaky testQuotaMetric

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



##########
File path: core/src/main/scala/kafka/server/ClientQuotaManager.scala
##########
@@ -360,8 +360,14 @@ class ClientQuotaManager(private val config: ClientQuotaManagerConfig,
    * Note: this method is expensive, it is meant to be used by tests only
    */
   def quota(userPrincipal: KafkaPrincipal, clientId: String): Quota = {
-    val metricTags = quotaCallback.quotaMetricTags(clientQuotaType, userPrincipal, clientId)
-    Quota.upperBound(quotaLimit(metricTags))
+    // acquire read lock to ensure that both quota limit and metric config are updated atomically
+    lock.readLock().lock()
+    try {
+      val metricTags = quotaCallback.quotaMetricTags(clientQuotaType, userPrincipal, clientId)
+      Quota.upperBound(quotaLimit(metricTags))
+    } finally {
+      lock.readLock().unlock()
+    }

Review comment:
       While this works, I wonder if this is the right way to overcome the issue. My concern is that there are other read paths which remain unprotected so we are not consistent. I would rather prefer to update the test as you suggested.




----------------------------------------------------------------
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 pull request #9417: MINOR: Fix flaky testQuotaMetric

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


   @splett2 Thanks for the update. Could you rebase the PR to include https://github.com/apache/kafka/commit/2db67db8e1329cb2e047322cff81d97ff98b4328? Tests won't run otherwise.


----------------------------------------------------------------
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 #9417: MINOR: Fix flaky testQuotaMetric

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



##########
File path: core/src/test/scala/unit/kafka/server/ControllerMutationQuotaTest.scala
##########
@@ -381,16 +381,18 @@ class ControllerMutationQuotaTest extends BaseRequestTest {
     Option(servers.head.metrics.metric(metricName))
   }
 
-  private def verifyQuotaMetric(user: String, expectedQuota: Double): Unit = {
-    quotaMetric(user) match {
-      case Some(metric) =>
-        val config = metric.config()
-        assertEquals(expectedQuota, config.quota().bound(), 0.1)
-        assertEquals(ControllerQuotaSamples, config.samples())
-        assertEquals(ControllerQuotaWindowSizeSeconds * 1000, config.timeWindowMs())
-
-      case None =>
-        fail(s"Quota metric of $user is not defined")
+  private def waitQuotaMetric(user: String, expectedQuota: Double): Unit = {
+    TestUtils.retry(200) {

Review comment:
       nit: Could we use `DEFAULT_MAX_WAIT_MS` (defined in `o.a.k.t.TestUtils`) here?




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