You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "machi1990 (via GitHub)" <gi...@apache.org> on 2023/05/10 15:56:48 UTC
[GitHub] [kafka] machi1990 opened a new pull request, #13700: KAFKA-14959: remove delayed queue and exempt sensors during ClientQuota and ClientRequestQuota managers shutdown
machi1990 opened a new pull request, #13700:
URL: https://github.com/apache/kafka/pull/13700
Follows up on https://github.com/apache/kafka/pull/13623#discussion_r1182592921
### 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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
RE: [GitHub] [kafka] machi1990 commented on pull request #13700: KAFKA-14959: remove delayed queue and exempt sensors during ClientQuota and ClientRequestQuota managers shutdown
Posted by sm...@votecgroup.com.
Please remove me
-----Original Message-----
From: machi1990 (via GitHub) <gi...@apache.org>
Sent: Wednesday, June 14, 2023 7:26 PM
To: jira@kafka.apache.org
Subject: [GitHub] [kafka] machi1990 commented on pull request #13700: KAFKA-14959: remove delayed queue and exempt sensors during ClientQuota and ClientRequestQuota managers shutdown
machi1990 commented on PR #13700:
URL: https://github.com/apache/kafka/pull/13700#issuecomment-1591262415
@divijvaidya since you worked on https://github.com/apache/kafka/pull/13623, would you be open to give this PR a review? Thanks
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] divijvaidya commented on a diff in pull request #13700: KAFKA-14959: remove delayed queue and exempt sensors during ClientQuota and ClientRequestQuota managers shutdown
Posted by "divijvaidya (via GitHub)" <gi...@apache.org>.
divijvaidya commented on code in PR #13700:
URL: https://github.com/apache/kafka/pull/13700#discussion_r1234166769
##########
core/src/main/scala/kafka/server/ClientQuotaManager.scala:
##########
@@ -574,6 +574,7 @@ class ClientQuotaManager(private val config: ClientQuotaManagerConfig,
def shutdown(): Unit = {
initiateShutdown()
throttledChannelReaper.awaitShutdown()
+ metrics.removeSensor(delayQueueSensor.name())
Review Comment:
in a finally block please.
##########
core/src/main/scala/kafka/server/ClientRequestQuotaManager.scala:
##########
@@ -92,4 +91,9 @@ class ClientRequestQuotaManager(private val config: ClientQuotaManagerConfig,
private def nanosToPercentage(nanos: Long): Double =
nanos * ClientRequestQuotaManager.NanosToPercentagePerSecond
+
+ override def shutdown(): Unit = {
+ super.shutdown()
+ metrics.removeSensor(exemptSensor.name())
Review Comment:
same. in a finally block please.
##########
core/src/main/scala/kafka/server/ClientRequestQuotaManager.scala:
##########
@@ -28,13 +28,12 @@ import org.apache.kafka.server.quota.ClientQuotaCallback
import scala.jdk.CollectionConverters._
object ClientRequestQuotaManager {
- val QuotaRequestPercentDefault = Int.MaxValue.toDouble
Review Comment:
please investigate why do we have this unused variable. Asking because it could be a bug where we want to use this but are not using this,
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] machi1990 commented on pull request #13700: KAFKA-14959: remove delayed queue and exempt sensors during ClientQuota and ClientRequestQuota managers shutdown
Posted by "machi1990 (via GitHub)" <gi...@apache.org>.
machi1990 commented on PR #13700:
URL: https://github.com/apache/kafka/pull/13700#issuecomment-1597319479
> Thank you for the PR. It's worth noting that the ClientQuotaManagers are shutdown on broker shutdown and hence, it's not a big deal if we are leaking metrics right now. Nevertheless, it's good practice to close metrics properly and remove them from the JMX server.
Thanks for the review @divijvaidya I've addressed all of your comments except for one where I've left a suggested course of action. Let me know what you think
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] divijvaidya commented on pull request #13700: KAFKA-14959: remove delayed queue and exempt sensors during ClientQuota and ClientRequestQuota managers shutdown
Posted by "divijvaidya (via GitHub)" <gi...@apache.org>.
divijvaidya commented on PR #13700:
URL: https://github.com/apache/kafka/pull/13700#issuecomment-1607659721
I wanted to let you know that this is on my radar and I need a few more days to get to this.
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] divijvaidya commented on a diff in pull request #13700: KAFKA-14959: remove delayed queue and exempt sensors during ClientQuota and ClientRequestQuota managers shutdown
Posted by "divijvaidya (via GitHub)" <gi...@apache.org>.
divijvaidya commented on code in PR #13700:
URL: https://github.com/apache/kafka/pull/13700#discussion_r1233939989
##########
core/src/test/scala/unit/kafka/server/ClientQuotaManagerTest.scala:
##########
@@ -418,6 +418,17 @@ class ClientQuotaManagerTest extends BaseClientQuotaManagerTest {
}
}
+ @Test
+ def testDelayedQueueSensorShouldShouldExistAfterInstantiationAndBeRemovedAfterShutdown(): Unit = {
Review Comment:
typo
\shouldshould\should
##########
core/src/test/scala/unit/kafka/server/ClientQuotaManagerTest.scala:
##########
@@ -418,6 +418,17 @@ class ClientQuotaManagerTest extends BaseClientQuotaManagerTest {
}
}
+ @Test
+ def testDelayedQueueSensorShouldShouldExistAfterInstantiationAndBeRemovedAfterShutdown(): Unit = {
+ val sensorName = Produce.toString + "-delayQueue"
+ val clientQuotaManager = new ClientQuotaManager(config, metrics, Produce, time, "")
+ var delayedQueueSensor = metrics.getSensor(sensorName)
+ assertNotNull(delayedQueueSensor, "delayed queue sensor should exist")
+ clientQuotaManager.shutdown()
+ delayedQueueSensor = metrics.getSensor(sensorName)
Review Comment:
Another validation we can perform is to ensure that
`assertTrue(metrics.metrics().isEmpty)` at `@AfterEach` of BaseClientQuotaManagerTest
##########
core/src/main/scala/kafka/server/ClientQuotaManager.scala:
##########
@@ -572,6 +572,7 @@ class ClientQuotaManager(private val config: ClientQuotaManagerConfig,
}
def shutdown(): Unit = {
+ metrics.removeSensor(delayQueueSensor.name())
Review Comment:
we should clean up the metric at the very end of the shutdown call (inside a finally). This is because the metric may still get values while the thread hasn't completed shutdown.
##########
core/src/main/scala/kafka/server/ClientRequestQuotaManager.scala:
##########
@@ -28,13 +28,12 @@ import org.apache.kafka.server.quota.ClientQuotaCallback
import scala.jdk.CollectionConverters._
object ClientRequestQuotaManager {
- val QuotaRequestPercentDefault = Int.MaxValue.toDouble
val NanosToPercentagePerSecond = 100.0 / TimeUnit.SECONDS.toNanos(1)
// Since exemptSensor is for all clients and has a constant name, we do not expire exemptSensor and only
// create once.
val DefaultInactiveExemptSensorExpirationTimeSeconds = Long.MaxValue
- private val ExemptSensorName = "exempt-" + QuotaType.Request
+ val ExemptSensorName = "exempt-" + QuotaType.Request
Review Comment:
this could be `private[server]`
##########
core/src/test/scala/unit/kafka/server/ClientQuotaManagerTest.scala:
##########
@@ -418,6 +418,17 @@ class ClientQuotaManagerTest extends BaseClientQuotaManagerTest {
}
}
+ @Test
+ def testDelayedQueueSensorShouldShouldExistAfterInstantiationAndBeRemovedAfterShutdown(): Unit = {
+ val sensorName = Produce.toString + "-delayQueue"
+ val clientQuotaManager = new ClientQuotaManager(config, metrics, Produce, time, "")
+ var delayedQueueSensor = metrics.getSensor(sensorName)
+ assertNotNull(delayedQueueSensor, "delayed queue sensor should exist")
+ clientQuotaManager.shutdown()
Review Comment:
please ensure that this gets cleaned up at the end of the test if the above assertion fails.
(same comment for other 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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] machi1990 commented on a diff in pull request #13700: KAFKA-14959: remove delayed queue and exempt sensors during ClientQuota and ClientRequestQuota managers shutdown
Posted by "machi1990 (via GitHub)" <gi...@apache.org>.
machi1990 commented on code in PR #13700:
URL: https://github.com/apache/kafka/pull/13700#discussion_r1234159704
##########
core/src/test/scala/unit/kafka/server/ClientQuotaManagerTest.scala:
##########
@@ -418,6 +418,17 @@ class ClientQuotaManagerTest extends BaseClientQuotaManagerTest {
}
}
+ @Test
+ def testDelayedQueueSensorShouldShouldExistAfterInstantiationAndBeRemovedAfterShutdown(): Unit = {
+ val sensorName = Produce.toString + "-delayQueue"
+ val clientQuotaManager = new ClientQuotaManager(config, metrics, Produce, time, "")
+ var delayedQueueSensor = metrics.getSensor(sensorName)
+ assertNotNull(delayedQueueSensor, "delayed queue sensor should exist")
+ clientQuotaManager.shutdown()
+ delayedQueueSensor = metrics.getSensor(sensorName)
Review Comment:
I looked onto this and apparently the metrics.metrics() List is never empty because this metric https://github.com/apache/kafka/blob/ca11a87e86e8e3c65043f747f35cae770b1efb7c/clients/src/main/java/org/apache/kafka/common/metrics/Metrics.java#L180 isn't at the moment removed when we `Metrics#close()` is called.
Is that to be expected? If not, I can create a separate JIRA and address it separate from this 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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] machi1990 commented on a diff in pull request #13700: KAFKA-14959: remove delayed queue and exempt sensors during ClientQuota and ClientRequestQuota managers shutdown
Posted by "machi1990 (via GitHub)" <gi...@apache.org>.
machi1990 commented on code in PR #13700:
URL: https://github.com/apache/kafka/pull/13700#discussion_r1234194014
##########
core/src/main/scala/kafka/server/ClientRequestQuotaManager.scala:
##########
@@ -28,13 +28,12 @@ import org.apache.kafka.server.quota.ClientQuotaCallback
import scala.jdk.CollectionConverters._
object ClientRequestQuotaManager {
- val QuotaRequestPercentDefault = Int.MaxValue.toDouble
Review Comment:
The const was defining the default value for quota request percent: Before #10025 this was done in the DynamicConfig.scala class.
After #10025, we stopped using by just hardcoding the value: https://github.com/mumrah/kafka/blob/a9ede6eaedc1cdef2757bd2f03de1cb0efe170f8/clients/src/main/java/org/apache/kafka/common/config/internals/QuotaConfigs.java#L62
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] machi1990 commented on pull request #13700: KAFKA-14959: remove delayed queue and exempt sensors during ClientQuota and ClientRequestQuota managers shutdown
Posted by "machi1990 (via GitHub)" <gi...@apache.org>.
machi1990 commented on PR #13700:
URL: https://github.com/apache/kafka/pull/13700#issuecomment-1692115498
I've limited bandwidth thus I won't be able to carry this work forward. I'll happily close this MR and unassign myself from the JIRA for someone else to take it over.
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] machi1990 commented on pull request #13700: KAFKA-14959: remove delayed queue and exempt sensors during ClientQuota and ClientRequestQuota managers shutdown
Posted by "machi1990 (via GitHub)" <gi...@apache.org>.
machi1990 commented on PR #13700:
URL: https://github.com/apache/kafka/pull/13700#issuecomment-1637571740
Hi @divijvaidya friendly ping on this PR, I know you are busy preparing 3.5.1 release, so whenever you've some free cycle, please review, thanks.
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] machi1990 commented on a diff in pull request #13700: KAFKA-14959: remove delayed queue and exempt sensors during ClientQuota and ClientRequestQuota managers shutdown
Posted by "machi1990 (via GitHub)" <gi...@apache.org>.
machi1990 commented on code in PR #13700:
URL: https://github.com/apache/kafka/pull/13700#discussion_r1234119510
##########
core/src/test/scala/unit/kafka/server/ClientQuotaManagerTest.scala:
##########
@@ -418,6 +418,17 @@ class ClientQuotaManagerTest extends BaseClientQuotaManagerTest {
}
}
+ @Test
+ def testDelayedQueueSensorShouldShouldExistAfterInstantiationAndBeRemovedAfterShutdown(): Unit = {
Review Comment:
good catch, I'll 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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] machi1990 commented on a diff in pull request #13700: KAFKA-14959: remove delayed queue and exempt sensors during ClientQuota and ClientRequestQuota managers shutdown
Posted by "machi1990 (via GitHub)" <gi...@apache.org>.
machi1990 commented on code in PR #13700:
URL: https://github.com/apache/kafka/pull/13700#discussion_r1234178162
##########
core/src/main/scala/kafka/server/ClientQuotaManager.scala:
##########
@@ -574,6 +574,7 @@ class ClientQuotaManager(private val config: ClientQuotaManagerConfig,
def shutdown(): Unit = {
initiateShutdown()
throttledChannelReaper.awaitShutdown()
+ metrics.removeSensor(delayQueueSensor.name())
Review Comment:
You are right!
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] machi1990 closed pull request #13700: KAFKA-14959: remove delayed queue and exempt sensors during ClientQuota and ClientRequestQuota managers shutdown
Posted by "machi1990 (via GitHub)" <gi...@apache.org>.
machi1990 closed pull request #13700: KAFKA-14959: remove delayed queue and exempt sensors during ClientQuota and ClientRequestQuota managers shutdown
URL: https://github.com/apache/kafka/pull/13700
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] machi1990 commented on pull request #13700: KAFKA-14959: remove delayed queue and exempt sensors during ClientQuota and ClientRequestQuota managers shutdown
Posted by "machi1990 (via GitHub)" <gi...@apache.org>.
machi1990 commented on PR #13700:
URL: https://github.com/apache/kafka/pull/13700#issuecomment-1591262415
@divijvaidya since you worked on https://github.com/apache/kafka/pull/13623, would you be open to give this PR a review? Thanks
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] machi1990 commented on a diff in pull request #13700: KAFKA-14959: remove delayed queue and exempt sensors during ClientQuota and ClientRequestQuota managers shutdown
Posted by "machi1990 (via GitHub)" <gi...@apache.org>.
machi1990 commented on code in PR #13700:
URL: https://github.com/apache/kafka/pull/13700#discussion_r1234120168
##########
core/src/test/scala/unit/kafka/server/ClientQuotaManagerTest.scala:
##########
@@ -418,6 +418,17 @@ class ClientQuotaManagerTest extends BaseClientQuotaManagerTest {
}
}
+ @Test
+ def testDelayedQueueSensorShouldShouldExistAfterInstantiationAndBeRemovedAfterShutdown(): Unit = {
+ val sensorName = Produce.toString + "-delayQueue"
+ val clientQuotaManager = new ClientQuotaManager(config, metrics, Produce, time, "")
+ var delayedQueueSensor = metrics.getSensor(sensorName)
+ assertNotNull(delayedQueueSensor, "delayed queue sensor should exist")
+ clientQuotaManager.shutdown()
Review Comment:
Good call.
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] divijvaidya commented on a diff in pull request #13700: KAFKA-14959: remove delayed queue and exempt sensors during ClientQuota and ClientRequestQuota managers shutdown
Posted by "divijvaidya (via GitHub)" <gi...@apache.org>.
divijvaidya commented on code in PR #13700:
URL: https://github.com/apache/kafka/pull/13700#discussion_r1234237168
##########
core/src/main/scala/kafka/server/ClientRequestQuotaManager.scala:
##########
@@ -28,13 +28,12 @@ import org.apache.kafka.server.quota.ClientQuotaCallback
import scala.jdk.CollectionConverters._
object ClientRequestQuotaManager {
- val QuotaRequestPercentDefault = Int.MaxValue.toDouble
Review Comment:
ack. 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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] machi1990 commented on pull request #13700: KAFKA-14959: remove delayed queue and exempt sensors during ClientQuota and ClientRequestQuota managers shutdown
Posted by "machi1990 (via GitHub)" <gi...@apache.org>.
machi1990 commented on PR #13700:
URL: https://github.com/apache/kafka/pull/13700#issuecomment-1542448724
I am wondering if these sensors that are dynamically created should be removed as well: https://github.com/apache/kafka/blob/ca11a87e86e8e3c65043f747f35cae770b1efb7c/core/src/main/scala/kafka/server/ClientQuotaManager.scala#L393-L402 looking for suggestion 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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] machi1990 commented on pull request #13700: KAFKA-14959: remove delayed queue and exempt sensors during ClientQuota and ClientRequestQuota managers shutdown
Posted by "machi1990 (via GitHub)" <gi...@apache.org>.
machi1990 commented on PR #13700:
URL: https://github.com/apache/kafka/pull/13700#issuecomment-1542446635
@rajinisivaram @dajac please review when you've the chance
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] machi1990 commented on pull request #13700: KAFKA-14959: remove delayed queue and exempt sensors during ClientQuota and ClientRequestQuota managers shutdown
Posted by "machi1990 (via GitHub)" <gi...@apache.org>.
machi1990 commented on PR #13700:
URL: https://github.com/apache/kafka/pull/13700#issuecomment-1621288580
> I wanted to let you know that this is on my radar and I need a few more days to get to this.
Thanks @divijvaidya
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] machi1990 commented on a diff in pull request #13700: KAFKA-14959: remove delayed queue and exempt sensors during ClientQuota and ClientRequestQuota managers shutdown
Posted by "machi1990 (via GitHub)" <gi...@apache.org>.
machi1990 commented on code in PR #13700:
URL: https://github.com/apache/kafka/pull/13700#discussion_r1234120547
##########
core/src/main/scala/kafka/server/ClientQuotaManager.scala:
##########
@@ -572,6 +572,7 @@ class ClientQuotaManager(private val config: ClientQuotaManagerConfig,
}
def shutdown(): Unit = {
+ metrics.removeSensor(delayQueueSensor.name())
Review Comment:
I'll adjust. Thanks
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org