> On Aug. 6, 2015, 4:17 p.m., Jun Rao wrote: > > core/src/main/scala/kafka/server/ClientQuotaManager.scala, lines 154-201 > > <https://reviews.apache.org/r/33049/diff/18/?file=1032147#file1032147line154> > > > > Not sure why the lock is needed. metrics.sensor() is synchronized and > > alreayd does the getOrCreate thing.
Couple of reasons: - We always have to pass in the MetricConfig which is potentially different per client-id. - metrics.sensor() does not indicate if the sensor was created or already existed. Since we dont have that information, we dont know whether we should call quotaSensor.add(new MetricName). If we add a duplicate MetricName an Exception is thrown and I didnt want to rely on that pattern to detect if the metrics exist or not. - Also, we need to make sure that both the sensors (throttleTime and quotaSensor) are fully created before any thread attempts to use them. Even if metrics.sensor() is synchronized, we can have a case where multiple threads for the same clientId try to create the sensors at the same time. Only the one creating the sensor should add the rate Metric so I the thread that doesnt create, attempts to record a value, it will also get an exception. Bacause of this I have syncrhonized the operation. In any case, this only acquires a read lock which should be rather cheap. > On Aug. 6, 2015, 4:17 p.m., Jun Rao wrote: > > core/src/main/scala/kafka/server/ClientQuotaManager.scala, lines 187-191 > > <https://reviews.apache.org/r/33049/diff/18/?file=1032147#file1032147line187> > > > > I suggest this earlier but realized now this may not work. The rate > > thing works when there is a single client instance per client-id. However, > > there could be multiple instances in reality. This means the accumlated > > delay time could be larger than the elapsed time and the percentage of time > > delayed can be larger than 1, which is werid. So, we will need some other > > way to measure the degree of throttling (potentially at both client-id and > > global level). How about reporting the average throttle time? Perhaps that is better than rate. - Aditya ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33049/#review94412 ----------------------------------------------------------- On Aug. 5, 2015, 2:08 a.m., Aditya Auradkar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/33049/ > ----------------------------------------------------------- > > (Updated Aug. 5, 2015, 2:08 a.m.) > > > Review request for kafka, Joel Koshy and Jun Rao. > > > Bugs: KAFKA-2084 > https://issues.apache.org/jira/browse/KAFKA-2084 > > > Repository: kafka > > > Description > ------- > > Updated patch for quotas. This patch does the following: > 1. Add per-client metrics for both producer and consumers > 2. Add configuration for quotas > 3. Compute delay times in the metrics package and return the delay times in > QuotaViolationException > 4. Add a DelayQueue in KafkaApi's that can be used to throttle any type of > request. Implemented request throttling for produce and fetch requests. > 5. Added unit and integration test cases for both producer and consumer > 6. This doesn't include a system test. There is a separate ticket for that > 7. Fixed KAFKA-2191 - (Included fix from : > https://reviews.apache.org/r/34418/ ) > > Addressing Joel's comments > > > Diffs > ----- > > clients/src/main/java/org/apache/kafka/common/metrics/Quota.java > d82bb0c055e631425bc1ebbc7d387baac76aeeaa > > clients/src/main/java/org/apache/kafka/common/metrics/QuotaViolationException.java > a451e5385c9eca76b38b425e8ac856b2715fcffe > clients/src/main/java/org/apache/kafka/common/metrics/Sensor.java > ca823fd4639523018311b814fde69b6177e73b97 > clients/src/main/java/org/apache/kafka/common/metrics/stats/Rate.java > 98429da34418f7f1deba1b5e44e2e6025212edb3 > clients/src/test/java/org/apache/kafka/common/metrics/MetricsTest.java > 544e120594de78c43581a980b1e4087b4fb98ccb > clients/src/test/java/org/apache/kafka/common/utils/MockTime.java > core/src/main/scala/kafka/server/ClientQuotaManager.scala PRE-CREATION > core/src/main/scala/kafka/server/KafkaApis.scala > 18f5b5b895af1469876b2223841fd90a2dd255e0 > core/src/main/scala/kafka/server/KafkaConfig.scala > dbe170f87331f43e2dc30165080d2cb7dfe5fdbf > core/src/main/scala/kafka/server/KafkaServer.scala > 84d4730ac634f9a5bf12a656e422fea03ad72da8 > core/src/main/scala/kafka/server/ReplicaManager.scala > 795220e7f63d163be90738b4c1a39687b44c1395 > core/src/main/scala/kafka/server/ThrottledResponse.scala PRE-CREATION > core/src/main/scala/kafka/utils/ShutdownableThread.scala > fc226c863095b7761290292cd8755cd7ad0f155c > core/src/test/scala/integration/kafka/api/QuotasTest.scala PRE-CREATION > core/src/test/scala/unit/kafka/server/ClientQuotaManagerTest.scala > PRE-CREATION > core/src/test/scala/unit/kafka/server/KafkaConfigConfigDefTest.scala > 04a02e08a54139ee1a298c5354731bae009efef3 > core/src/test/scala/unit/kafka/server/ThrottledResponseExpirationTest.scala > PRE-CREATION > > Diff: https://reviews.apache.org/r/33049/diff/ > > > Testing > ------- > > > Thanks, > > Aditya Auradkar > >
