You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Aditya Auradkar <aa...@linkedin.com> on 2015/06/03 02:02:12 UTC
Re: Review Request 33049: Patch for KAFKA-2084
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/
-----------------------------------------------------------
(Updated June 3, 2015, 12:02 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)
-------
KAFKA-2186; Follow-up to KAFKA-1650 - add selective offset commit to consumer connector API; reviewed by Joel Koshy
KAFKA-2091; Expose a partitioner interface in the new producer (https://cwiki.apache.org/confluence/display/KAFKA/KIP-+22+-+Expose+a+Partitioner+interface+in+the+new+producer); reviewed by Joel Koshy and Jay Kreps
kafka-2185; Update to Gradle 2.4; patched by Ismael Juma; reviewed by Jun Rao
KAFKA-2199 Make signing artifacts optional and disabled by default for SNAPSHOTs and allow remote Maven repository configuration from the command line.
kafka-2226; NullPointerException in TestPurgatoryPerformance; patched by Yasuhiro Matsuda; reviewed by Onur Karaman, Guozhang Wang and Jun Rao
WIP: First patch for quotas. Changes are 1. Adding per-client throttle time and quota metrics in ClientQuotaMetrics.scala 2. Making changes in QuotaViolationException and Sensor to return delay time changes. 3. Added configuration needed so far for quotas in KafkaConfig. 4. Unit tests
This is currently not being used anywhere in the code because I haven't yet figured out how to enforce delays i.e. purgatory vs delay queue. I'll have a better idea once I look at the new purgatory implementation. Hopefully, this smaller patch is easier to review.
Added more testcases
Some locking changes for reading/creating the sensors
WIP patch
Sample usage in ReplicaManager
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. I've not yet added integration testcases testing the consumer delays.. will update the patch once those are ready
Incorporated Jun's comments
Adding javadoc
KAFKA-2084 - Moved the callbacks to ClientQuotaMetrics
Adding more configs
Don't quota replica traffic
KAFKA-2084
Fixing test failures
Diffs (updated)
-----
README.md 946ec62cc71df93c905c5f35caf5cdb9c78e5c10
build.gradle cd2aa838fd53e8124f308979b1d70efe0c5725a6
clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 8e336a3aa96c73f52beaeb56b931baf4b026cf21
clients/src/main/java/org/apache/kafka/clients/producer/MockProducer.java 3c34610dbc8a68e4561e7264e0b545de3d93cef2
clients/src/main/java/org/apache/kafka/clients/producer/Partitioner.java PRE-CREATION
clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java 187d0004c8c46b6664ddaffecc6166d4b47351e5
clients/src/main/java/org/apache/kafka/clients/producer/internals/Partitioner.java 93e799105fb6cc5c49a129c0db099a3a973b2ab3
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/clients/producer/internals/PartitionerTest.java 5dadd0e3554577ad6be28a18ff5ab08f8b31050f
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/javaapi/consumer/ConsumerConnector.java cc3400ff81fc0db69b5129ad7b440f20a211a79d
core/src/main/scala/kafka/server/ClientQuotaMetrics.scala PRE-CREATION
core/src/main/scala/kafka/server/KafkaApis.scala 387e387998fc3a6c9cb585dab02b5f77b0381fbf
core/src/main/scala/kafka/server/KafkaConfig.scala 9efa15ca5567b295ab412ee9eea7c03eb4cdc18b
core/src/main/scala/kafka/server/KafkaServer.scala e66710d2368334ece66f70d55f57b3f888262620
core/src/main/scala/kafka/server/ReplicaManager.scala 59c9bc3ac3a8afc07a6f8c88c5871304db588d17
core/src/main/scala/kafka/server/ThrottledRequest.scala PRE-CREATION
core/src/main/scala/kafka/utils/ShutdownableThread.scala fc226c863095b7761290292cd8755cd7ad0f155c
core/src/main/scala/kafka/utils/timer/Timer.scala b8cde820a770a4e894804f1c268b24b529940650
core/src/main/scala/kafka/utils/timer/TimerTask.scala 3407138115d579339ffb6b00e32e38c984ac5d6e
core/src/main/scala/kafka/utils/timer/TimerTaskList.scala e7a96570ddc2367583d6d5590628db7e7f6ba39b
core/src/main/scala/kafka/utils/timer/TimingWheel.scala e92aba3844dbf3372182e14536a5d98cf3366d73
core/src/test/scala/integration/kafka/api/QuotasTest.scala PRE-CREATION
core/src/test/scala/unit/kafka/server/ClientQuotaMetricsTest.scala PRE-CREATION
core/src/test/scala/unit/kafka/server/KafkaConfigConfigDefTest.scala 8014a5a6c362785539f24eb03d77278434614fe6
core/src/test/scala/unit/kafka/server/ThrottledRequestExpirationTest.scala PRE-CREATION
gradle.properties 90b1945372e767b9c2d0a50df9eb7063e0629952
Diff: https://reviews.apache.org/r/33049/diff/
Testing
-------
Thanks,
Aditya Auradkar
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Aditya Auradkar <aa...@linkedin.com>.
> On June 3, 2015, 3:57 p.m., Joel Koshy wrote:
> > core/src/main/scala/kafka/server/KafkaServer.scala, line 200
> > <https://reviews.apache.org/r/33049/diff/10/?file=972069#file972069line200>
> >
> > Can we consider migrating the server to use the Time interface in clients and just use that uniformly throughout?
Certainly. I did plan to do that but in a separate jira. https://issues.apache.org/jira/browse/KAFKA-2247
> On June 3, 2015, 3:57 p.m., Joel Koshy wrote:
> > core/src/test/scala/integration/kafka/api/QuotasTest.scala, line 47
> > <https://reviews.apache.org/r/33049/diff/10/?file=972073#file972073line47>
> >
> > Consumer quotas don't seem to be tested in this?
I've added them now. Thanks
> On June 3, 2015, 3:57 p.m., Joel Koshy wrote:
> > core/src/main/scala/kafka/server/ClientQuotaMetrics.scala, line 110
> > <https://reviews.apache.org/r/33049/diff/10/?file=972066#file972066line110>
> >
> > We generally prefer not using tuples. In this case since it is private it is probably okay, but again, reconsider. http://kafka.apache.org/coding-guide.html
> > If you absolutely must use tuples in a local context use something like:
> >
> > `val (quotaSensor, throttleTimeSensor) = (returnedValue._1, returnedValue._2)`
Replaced it with a case class.
> On June 3, 2015, 3:57 p.m., Joel Koshy wrote:
> > core/src/main/scala/kafka/server/ClientQuotaMetrics.scala, line 119
> > <https://reviews.apache.org/r/33049/diff/10/?file=972066#file972066line119>
> >
> > We should expose server-side metrics on per-client * API key throttle rate right?
We already have throttle time metrics per-clientId * API key. Do you think we need rate in addition to those?
- Aditya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/#review86418
-----------------------------------------------------------
On June 3, 2015, 12:16 a.m., Aditya Auradkar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33049/
> -----------------------------------------------------------
>
> (Updated June 3, 2015, 12:16 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.
> 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/ )
>
>
> 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/ClientQuotaMetrics.scala PRE-CREATION
> core/src/main/scala/kafka/server/KafkaApis.scala 387e387998fc3a6c9cb585dab02b5f77b0381fbf
> core/src/main/scala/kafka/server/KafkaConfig.scala 9efa15ca5567b295ab412ee9eea7c03eb4cdc18b
> core/src/main/scala/kafka/server/KafkaServer.scala e66710d2368334ece66f70d55f57b3f888262620
> core/src/main/scala/kafka/server/ReplicaManager.scala 59c9bc3ac3a8afc07a6f8c88c5871304db588d17
> core/src/main/scala/kafka/server/ThrottledRequest.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/ClientQuotaMetricsTest.scala PRE-CREATION
> core/src/test/scala/unit/kafka/server/KafkaConfigConfigDefTest.scala 8014a5a6c362785539f24eb03d77278434614fe6
> core/src/test/scala/unit/kafka/server/ThrottledRequestExpirationTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/33049/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Aditya Auradkar
>
>
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/#review86418
-----------------------------------------------------------
I had collected my comments on this older revision. I have not looked at the latest, so can you incorporate relevant comments into the newest revision.
MockTime: why was this moved out of the test package?
We also had an offline discussion on perhaps adding a maxDelay option for quotas. i.e., this would perhaps be useful for consumers so that they can set their consumer timeout period appropriately. However, this is debatable since that is really a separate config - i.e., how long a consumer is willing to wait before giving up. If it is throttled more than that period then it should in fact stop.
core/src/main/scala/kafka/server/ClientQuotaMetrics.scala
<https://reviews.apache.org/r/33049/#comment138373>
typo
core/src/main/scala/kafka/server/ClientQuotaMetrics.scala
<https://reviews.apache.org/r/33049/#comment138374>
param is not a KafkaConfig
core/src/main/scala/kafka/server/ClientQuotaMetrics.scala
<https://reviews.apache.org/r/33049/#comment138377>
We should probably expose a metric on the current size of the delay queue
core/src/main/scala/kafka/server/ClientQuotaMetrics.scala
<https://reviews.apache.org/r/33049/#comment138382>
This is for the consumer as well right?
core/src/main/scala/kafka/server/ClientQuotaMetrics.scala
<https://reviews.apache.org/r/33049/#comment138381>
recordAndMaybeThrottle ?
core/src/main/scala/kafka/server/ClientQuotaMetrics.scala
<https://reviews.apache.org/r/33049/#comment138383>
We generally prefer not using tuples. In this case since it is private it is probably okay, but again, reconsider. http://kafka.apache.org/coding-guide.html
If you absolutely must use tuples in a local context use something like:
`val (quotaSensor, throttleTimeSensor) = (returnedValue._1, returnedValue._2)`
core/src/main/scala/kafka/server/ClientQuotaMetrics.scala
<https://reviews.apache.org/r/33049/#comment138378>
Spacing. We generally do "a: b", not "a : b". I saw a few other occurrences.
core/src/main/scala/kafka/server/ClientQuotaMetrics.scala
<https://reviews.apache.org/r/33049/#comment138379>
Spacing: a * b
core/src/main/scala/kafka/server/ClientQuotaMetrics.scala
<https://reviews.apache.org/r/33049/#comment138407>
We should expose server-side metrics on per-client * API key throttle rate right?
core/src/main/scala/kafka/server/ClientQuotaMetrics.scala
<https://reviews.apache.org/r/33049/#comment138380>
Should we set this to debug? and just rely on server metrics to know the rate at which requests are getting throttled?
core/src/main/scala/kafka/server/ClientQuotaMetrics.scala
<https://reviews.apache.org/r/33049/#comment138385>
Use: `= overriddenQuota.getOrElse(clientId, defaultQuota)`
core/src/main/scala/kafka/server/ClientQuotaMetrics.scala
<https://reviews.apache.org/r/33049/#comment138375>
This should ideally return an immutable map.
core/src/main/scala/kafka/server/ClientQuotaMetrics.scala
<https://reviews.apache.org/r/33049/#comment138376>
You can use the collectio.map pattern to build the immutable map. It will also make this code more concise without losing clarity.
core/src/main/scala/kafka/server/KafkaApis.scala
<https://reviews.apache.org/r/33049/#comment138386>
Yes this looks like an inadvertent ide-suggested import.
core/src/main/scala/kafka/server/KafkaApis.scala
<https://reviews.apache.org/r/33049/#comment138389>
opening brace should be on same line
core/src/main/scala/kafka/server/KafkaApis.scala
<https://reviews.apache.org/r/33049/#comment138390>
same
core/src/main/scala/kafka/server/KafkaApis.scala
<https://reviews.apache.org/r/33049/#comment138391>
same
core/src/main/scala/kafka/server/KafkaApis.scala
<https://reviews.apache.org/r/33049/#comment138392>
same
core/src/main/scala/kafka/server/KafkaApis.scala
<https://reviews.apache.org/r/33049/#comment138393>
`.recordAndMaybeThrottle` would look better I think
core/src/main/scala/kafka/server/KafkaApis.scala
<https://reviews.apache.org/r/33049/#comment138394>
Cannot throttle producer request...
core/src/main/scala/kafka/server/KafkaApis.scala
<https://reviews.apache.org/r/33049/#comment138395>
`if (fetchRequest.isFromFollower)`
core/src/main/scala/kafka/server/KafkaApis.scala
<https://reviews.apache.org/r/33049/#comment138396>
`cfg: KafkaConfig`
core/src/main/scala/kafka/server/KafkaApis.scala
<https://reviews.apache.org/r/33049/#comment138387>
Can probably drop this comment.
core/src/main/scala/kafka/server/KafkaConfig.scala
<https://reviews.apache.org/r/33049/#comment138398>
There is some inconsistency between configs here. (default vs bytes.per.second). How about BytesPerSecondOverrides and BytesPerSecondDefaults?
core/src/main/scala/kafka/server/KafkaServer.scala
<https://reviews.apache.org/r/33049/#comment138399>
For simple declarations such as these, prefer dropping the type declarations. i.e., `val jmxPrefix = "kafka.server"`
core/src/main/scala/kafka/server/KafkaServer.scala
<https://reviews.apache.org/r/33049/#comment138400>
same
core/src/main/scala/kafka/server/KafkaServer.scala
<https://reviews.apache.org/r/33049/#comment138401>
Can we consider migrating the server to use the Time interface in clients and just use that uniformly throughout?
core/src/main/scala/kafka/server/ThrottledRequest.scala
<https://reviews.apache.org/r/33049/#comment138402>
`override def`
core/src/main/scala/kafka/server/ThrottledRequest.scala
<https://reviews.apache.org/r/33049/#comment138403>
`override def`
core/src/main/scala/kafka/utils/ShutdownableThread.scala
<https://reviews.apache.org/r/33049/#comment138404>
typo
or just: "is repeatedly invoked until..."
core/src/test/scala/integration/kafka/api/QuotasTest.scala
<https://reviews.apache.org/r/33049/#comment138405>
Remove this line
core/src/test/scala/integration/kafka/api/QuotasTest.scala
<https://reviews.apache.org/r/33049/#comment138406>
Consumer quotas don't seem to be tested in this?
- Joel Koshy
On June 3, 2015, 12:16 a.m., Aditya Auradkar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33049/
> -----------------------------------------------------------
>
> (Updated June 3, 2015, 12:16 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.
> 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/ )
>
>
> 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/ClientQuotaMetrics.scala PRE-CREATION
> core/src/main/scala/kafka/server/KafkaApis.scala 387e387998fc3a6c9cb585dab02b5f77b0381fbf
> core/src/main/scala/kafka/server/KafkaConfig.scala 9efa15ca5567b295ab412ee9eea7c03eb4cdc18b
> core/src/main/scala/kafka/server/KafkaServer.scala e66710d2368334ece66f70d55f57b3f888262620
> core/src/main/scala/kafka/server/ReplicaManager.scala 59c9bc3ac3a8afc07a6f8c88c5871304db588d17
> core/src/main/scala/kafka/server/ThrottledRequest.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/ClientQuotaMetricsTest.scala PRE-CREATION
> core/src/test/scala/unit/kafka/server/KafkaConfigConfigDefTest.scala 8014a5a6c362785539f24eb03d77278434614fe6
> core/src/test/scala/unit/kafka/server/ThrottledRequestExpirationTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/33049/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Aditya Auradkar
>
>
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Joel Koshy <jj...@gmail.com>.
> On June 17, 2015, 4:40 p.m., Joel Koshy wrote:
> > core/src/main/scala/kafka/server/ClientQuotaMetrics.scala, line 1
> > <https://reviews.apache.org/r/33049/diff/15/?file=983845#file983845line1>
> >
> > Why was MockTime moved from test to main?
>
> Aditya Auradkar wrote:
> Because I need to depend on MockTime from clients in core for unit tests written using the new metrics package. I don't think core depends on "test" from clients
I think you can avoid this by spec `testCompile` depend on clients-test. I think that would be better - would prefer not allowing `MockTime` from leaking into the non-test packages.
- Joel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/#review88180
-----------------------------------------------------------
On Aug. 12, 2015, 7:09 p.m., Aditya Auradkar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33049/
> -----------------------------------------------------------
>
> (Updated Aug. 12, 2015, 7:09 p.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/ )
>
> Addressed comments from Joel and Jun
>
>
> 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 7ea509c2c41acc00430c74e025e069a833aac4e7
> 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/KafkaConfigTest.scala f32d206d3f52f3f9f4d649c213edd7058f4b6150
> core/src/test/scala/unit/kafka/server/ThrottledResponseExpirationTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/33049/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Aditya Auradkar
>
>
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Aditya Auradkar <aa...@linkedin.com>.
> On June 17, 2015, 4:40 p.m., Joel Koshy wrote:
> > core/src/main/scala/kafka/server/ClientQuotaMetrics.scala, line 1
> > <https://reviews.apache.org/r/33049/diff/15/?file=983845#file983845line1>
> >
> > Why was MockTime moved from test to main?
Because I need to depend on MockTime from clients in core for unit tests written using the new metrics package. I don't think core depends on "test" from clients
- Aditya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/#review88180
-----------------------------------------------------------
On June 30, 2015, 12:53 a.m., Aditya Auradkar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33049/
> -----------------------------------------------------------
>
> (Updated June 30, 2015, 12:53 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
> -------
>
> Signed-off-by: Aditya Auradkar <aa...@linkedin.com>
>
> Addressing Joel's comments
>
>
> Minor imports changes
>
>
> Added testcase to verify that replication traffic is not throttled
>
>
> Tmp commit
>
>
> Fixing test failure
>
>
> Minor
>
>
> 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/ClientQuotaMetrics.scala PRE-CREATION
> core/src/main/scala/kafka/server/KafkaApis.scala d63bc18d795a6f0e6994538ca55a9a46f7fb8ffd
> core/src/main/scala/kafka/server/KafkaConfig.scala 2d75186a110075e0c322db4b9f7a8c964a7a3e88
> core/src/main/scala/kafka/server/KafkaServer.scala b320ce9f6a12c0ee392e91beb82e8804d167f9f4
> core/src/main/scala/kafka/server/ReplicaManager.scala 59c9bc3ac3a8afc07a6f8c88c5871304db588d17
> core/src/main/scala/kafka/server/ThrottledRequest.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/ClientQuotaMetricsTest.scala PRE-CREATION
> core/src/test/scala/unit/kafka/server/KafkaConfigConfigDefTest.scala ace6321b36d809946554d205bc926c9c76a43bd6
> core/src/test/scala/unit/kafka/server/ThrottledRequestExpirationTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/33049/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Aditya Auradkar
>
>
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Aditya Auradkar <aa...@linkedin.com>.
> On June 17, 2015, 4:40 p.m., Joel Koshy wrote:
> > core/src/test/scala/integration/kafka/api/QuotasTest.scala, line 142
> > <https://reviews.apache.org/r/33049/diff/15/?file=983852#file983852line142>
> >
> > This is an important test, but this is a bit non-deterministic no? i.e., the replicas could have been throttled, but caught up soon after that. We would just need to assert (after) this test that the elapsed time is within the expected delay time for an otherwised throttled consumer.
I rewrote the test a little. Basically, I'm creating a SimpleConsumer and sending a request with replicaId = "broker_id of follower". The fetchRequest parameters should get the request throttled but since it is a replica, the throttle_time in the response should be zero. I cant assert the throttle_time because that requires 2136 to be committed.
- Aditya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/#review88180
-----------------------------------------------------------
On June 30, 2015, 12:54 a.m., Aditya Auradkar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33049/
> -----------------------------------------------------------
>
> (Updated June 30, 2015, 12:54 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/ClientQuotaMetrics.scala PRE-CREATION
> core/src/main/scala/kafka/server/KafkaApis.scala d63bc18d795a6f0e6994538ca55a9a46f7fb8ffd
> core/src/main/scala/kafka/server/KafkaConfig.scala 2d75186a110075e0c322db4b9f7a8c964a7a3e88
> core/src/main/scala/kafka/server/KafkaServer.scala b320ce9f6a12c0ee392e91beb82e8804d167f9f4
> core/src/main/scala/kafka/server/ReplicaManager.scala 59c9bc3ac3a8afc07a6f8c88c5871304db588d17
> core/src/main/scala/kafka/server/ThrottledRequest.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/ClientQuotaMetricsTest.scala PRE-CREATION
> core/src/test/scala/unit/kafka/server/KafkaConfigConfigDefTest.scala ace6321b36d809946554d205bc926c9c76a43bd6
> core/src/test/scala/unit/kafka/server/ThrottledRequestExpirationTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/33049/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Aditya Auradkar
>
>
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/#review88180
-----------------------------------------------------------
This is looking very good.
clients/src/main/java/org/apache/kafka/common/metrics/QuotaViolationException.java (line 38)
<https://reviews.apache.org/r/33049/#comment140622>
We can remove the setter.
clients/src/main/java/org/apache/kafka/common/metrics/Sensor.java (line 120)
<https://reviews.apache.org/r/33049/#comment140623>
How about making this a bit more concise and past tense (since you would `getMessage` on the exception after the fact):
`%s violated quota. Actual: %f, Threshold: %f`
clients/src/test/java/org/apache/kafka/common/metrics/MetricsTest.java (line 88)
<https://reviews.apache.org/r/33049/#comment140648>
Occurences -> Occurrences
Also, `(0..%d) = %d` substituted with `count` and `count / elapsedSecs` - similar comment for the asserts below.
clients/src/test/java/org/apache/kafka/common/metrics/MetricsTest.java (line 92)
<https://reviews.apache.org/r/33049/#comment140649>
`long sleepTimeMs = 2000`
core/src/main/scala/kafka/server/ClientQuotaMetrics.scala (line 1)
<https://reviews.apache.org/r/33049/#comment140650>
Why was MockTime moved from test to main?
core/src/main/scala/kafka/server/ClientQuotaMetrics.scala (line 140)
<https://reviews.apache.org/r/33049/#comment140654>
would prefer to see this on a single line (no braces)
core/src/main/scala/kafka/server/KafkaConfig.scala (line 842)
<https://reviews.apache.org/r/33049/#comment140656>
Prefer a: b over a : b
core/src/main/scala/kafka/server/ThrottledRequest.scala (line 42)
<https://reviews.apache.org/r/33049/#comment140657>
`if (`
core/src/main/scala/kafka/server/ThrottledRequest.scala (line 43)
<https://reviews.apache.org/r/33049/#comment140658>
same
core/src/test/scala/integration/kafka/api/QuotasTest.scala (line 142)
<https://reviews.apache.org/r/33049/#comment140659>
This is an important test, but this is a bit non-deterministic no? i.e., the replicas could have been throttled, but caught up soon after that. We would just need to assert (after) this test that the elapsed time is within the expected delay time for an otherwised throttled consumer.
- Joel Koshy
On June 12, 2015, 5:40 p.m., Aditya Auradkar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33049/
> -----------------------------------------------------------
>
> (Updated June 12, 2015, 5:40 p.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/ClientQuotaMetrics.scala PRE-CREATION
> core/src/main/scala/kafka/server/KafkaApis.scala d63bc18d795a6f0e6994538ca55a9a46f7fb8ffd
> core/src/main/scala/kafka/server/KafkaConfig.scala 2d75186a110075e0c322db4b9f7a8c964a7a3e88
> core/src/main/scala/kafka/server/KafkaServer.scala b320ce9f6a12c0ee392e91beb82e8804d167f9f4
> core/src/main/scala/kafka/server/ReplicaManager.scala 59c9bc3ac3a8afc07a6f8c88c5871304db588d17
> core/src/main/scala/kafka/server/ThrottledRequest.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/ClientQuotaMetricsTest.scala PRE-CREATION
> core/src/test/scala/unit/kafka/server/KafkaConfigConfigDefTest.scala ace6321b36d809946554d205bc926c9c76a43bd6
> core/src/test/scala/unit/kafka/server/ThrottledRequestExpirationTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/33049/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Aditya Auradkar
>
>
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Aditya Auradkar <aa...@linkedin.com>.
> On Aug. 12, 2015, 12:42 a.m., Jun Rao wrote:
> > core/src/main/scala/kafka/server/KafkaConfig.scala, line 419
> > <https://reviews.apache.org/r/33049/diff/21/?file=1037170#file1037170line419>
> >
> > I am still not sure that I see the value of the delay factor. If one wants to be a bit conservative, one can always configure a lower quota value.
I'm dropping this config since it is quite unintuitive. Other reviewers have also mentioned this
- Aditya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/#review95035
-----------------------------------------------------------
On Aug. 11, 2015, 4:58 a.m., Aditya Auradkar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33049/
> -----------------------------------------------------------
>
> (Updated Aug. 11, 2015, 4:58 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/ )
>
> Addressed comments from Joel and Jun
>
>
> 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 7ea509c2c41acc00430c74e025e069a833aac4e7
> 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/KafkaConfigTest.scala f32d206d3f52f3f9f4d649c213edd7058f4b6150
> core/src/test/scala/unit/kafka/server/ThrottledResponseExpirationTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/33049/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Aditya Auradkar
>
>
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/#review95035
-----------------------------------------------------------
core/src/main/scala/kafka/server/KafkaConfig.scala (line 418)
<https://reviews.apache.org/r/33049/#comment149807>
I am still not sure that I see the value of the delay factor. If one wants to be a bit conservative, one can always configure a lower quota value.
- Jun Rao
On Aug. 11, 2015, 4:58 a.m., Aditya Auradkar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33049/
> -----------------------------------------------------------
>
> (Updated Aug. 11, 2015, 4:58 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/ )
>
> Addressed comments from Joel and Jun
>
>
> 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 7ea509c2c41acc00430c74e025e069a833aac4e7
> 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/KafkaConfigTest.scala f32d206d3f52f3f9f4d649c213edd7058f4b6150
> core/src/test/scala/unit/kafka/server/ThrottledResponseExpirationTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/33049/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Aditya Auradkar
>
>
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/#review95216
-----------------------------------------------------------
core/src/main/scala/kafka/server/ClientQuotaManager.scala (line 65)
<https://reviews.apache.org/r/33049/#comment150058>
We can edit on check-in: should be `ClientQuotaManagerConfig`
- Joel Koshy
On Aug. 12, 2015, 7:09 p.m., Aditya Auradkar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33049/
> -----------------------------------------------------------
>
> (Updated Aug. 12, 2015, 7:09 p.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/ )
>
> Addressed comments from Joel and Jun
>
>
> 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 7ea509c2c41acc00430c74e025e069a833aac4e7
> 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/KafkaConfigTest.scala f32d206d3f52f3f9f4d649c213edd7058f4b6150
> core/src/test/scala/unit/kafka/server/ThrottledResponseExpirationTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/33049/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Aditya Auradkar
>
>
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Aditya Auradkar <aa...@linkedin.com>.
> On Aug. 14, 2015, 1:57 a.m., Jun Rao wrote:
> > clients/src/main/java/org/apache/kafka/common/metrics/stats/Rate.java, lines 69-77
> > <https://reviews.apache.org/r/33049/diff/25/?file=1039169#file1039169line69>
> >
> > This is probably not the right place to throw QuotaViolationException. Rate.measure() can be called when the JMX value is polled and we don't want to throw an exception there.
> >
> > I was thinking that another way to do this is to perhaps add the logic in Quota.acceptable(). We can probably change it to be
> >
> > // if the check passes, return -1; otherwise return a delay to make the check pass.
> > int Quota.check(KafkaMetrics, long now).
> >
> > In the implementation, we can then check if the KafkaMetrics passed in has a measurable that's an instance of Rate. If so, we will compute the delay for rate (the window should be elapsedCurrent and elapsedPrior as computed in Rate.measure()).
Rate/Measurable stat does not expose the windows currently. So we cannot access elapsedCurrent and elapsedPrior from the KafkaMetric object in Sensor. Couple of options:
1. We can simply record it in Rate.record() which is called from Sensor.record
2. We can check quotas in Sensor.java but not track the precise window boundaries.
- Aditya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/#review95369
-----------------------------------------------------------
On Aug. 14, 2015, 2:09 a.m., Aditya Auradkar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33049/
> -----------------------------------------------------------
>
> (Updated Aug. 14, 2015, 2:09 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/ )
>
> Addressed comments from Joel and Jun
>
>
> Diffs
> -----
>
> build.gradle 1b67e628c2fca897177c12b6afad9a8700fffd1f
> 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
> core/src/main/scala/kafka/server/ClientQuotaManager.scala PRE-CREATION
> core/src/main/scala/kafka/server/KafkaApis.scala 7ea509c2c41acc00430c74e025e069a833aac4e7
> core/src/main/scala/kafka/server/KafkaConfig.scala a06f0bd40e2f90972b44b80a106f98f3d50e5e2b
> core/src/main/scala/kafka/server/KafkaServer.scala 84d4730ac634f9a5bf12a656e422fea03ad72da8
> core/src/main/scala/kafka/server/ReplicaManager.scala 2e0bbcd6e4f0e38997ea18202b249ee3553640ec
> 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/KafkaConfigTest.scala e26a7306a6ea3104b3fa3df60006c0a473bfb2cc
> core/src/test/scala/unit/kafka/server/ThrottledResponseExpirationTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/33049/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Aditya Auradkar
>
>
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Aditya Auradkar <aa...@linkedin.com>.
> On Aug. 14, 2015, 1:57 a.m., Jun Rao wrote:
> > clients/src/main/java/org/apache/kafka/common/metrics/stats/Rate.java, lines 69-77
> > <https://reviews.apache.org/r/33049/diff/25/?file=1039169#file1039169line69>
> >
> > This is probably not the right place to throw QuotaViolationException. Rate.measure() can be called when the JMX value is polled and we don't want to throw an exception there.
> >
> > I was thinking that another way to do this is to perhaps add the logic in Quota.acceptable(). We can probably change it to be
> >
> > // if the check passes, return -1; otherwise return a delay to make the check pass.
> > int Quota.check(KafkaMetrics, long now).
> >
> > In the implementation, we can then check if the KafkaMetrics passed in has a measurable that's an instance of Rate. If so, we will compute the delay for rate (the window should be elapsedCurrent and elapsedPrior as computed in Rate.measure()).
>
> Aditya Auradkar wrote:
> Rate/Measurable stat does not expose the windows currently. So we cannot access elapsedCurrent and elapsedPrior from the KafkaMetric object in Sensor. Couple of options:
> 1. We can simply record it in Rate.record() which is called from Sensor.record
> 2. We can check quotas in Sensor.java but not track the precise window boundaries.
>
> Joel Koshy wrote:
> Discussed offline with Aditya: I actually think the earlier approach with a slightly pessimistic delay in `Sensor` would be sufficient. Option 1 also sounds good but is slightly odd that we will then throw the exception from `Rate` directly and `Sensor.checkQuotas` as well (for other types).
>
> After thinking through this I think it is worthwhile to consider a small change to the implementation (https://cwiki.apache.org/confluence/display/KAFKA/KIP-13+-+Quotas#KIP-13-Quotas-DelaytimeinQuotaViolationException) to _not_ include the delay time in `QuotaViolationException`. i.e., the delay really (currently) applies only to `Rate` and does not fit very well with some other types (e.g., `Max`). Instead we we could compute the delay directly in `ClientQuotaManager` and leave `Sensor.checkQuotas` as is.
Addressed this in my newest patch
- Aditya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/#review95369
-----------------------------------------------------------
On Aug. 15, 2015, 12:43 a.m., Aditya Auradkar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33049/
> -----------------------------------------------------------
>
> (Updated Aug. 15, 2015, 12:43 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/ )
>
> Addressed comments from Joel and Jun
>
>
> Diffs
> -----
>
> build.gradle 1b67e628c2fca897177c12b6afad9a8700fffd1f
> 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
> core/src/main/scala/kafka/server/ClientQuotaManager.scala PRE-CREATION
> core/src/main/scala/kafka/server/KafkaApis.scala 7ea509c2c41acc00430c74e025e069a833aac4e7
> core/src/main/scala/kafka/server/KafkaConfig.scala a06f0bd40e2f90972b44b80a106f98f3d50e5e2b
> core/src/main/scala/kafka/server/KafkaServer.scala 84d4730ac634f9a5bf12a656e422fea03ad72da8
> core/src/main/scala/kafka/server/ReplicaManager.scala 2e0bbcd6e4f0e38997ea18202b249ee3553640ec
> 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/KafkaConfigTest.scala e26a7306a6ea3104b3fa3df60006c0a473bfb2cc
> core/src/test/scala/unit/kafka/server/ThrottledResponseExpirationTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/33049/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Aditya Auradkar
>
>
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Joel Koshy <jj...@gmail.com>.
> On Aug. 14, 2015, 1:57 a.m., Jun Rao wrote:
> > clients/src/main/java/org/apache/kafka/common/metrics/stats/Rate.java, lines 69-77
> > <https://reviews.apache.org/r/33049/diff/25/?file=1039169#file1039169line69>
> >
> > This is probably not the right place to throw QuotaViolationException. Rate.measure() can be called when the JMX value is polled and we don't want to throw an exception there.
> >
> > I was thinking that another way to do this is to perhaps add the logic in Quota.acceptable(). We can probably change it to be
> >
> > // if the check passes, return -1; otherwise return a delay to make the check pass.
> > int Quota.check(KafkaMetrics, long now).
> >
> > In the implementation, we can then check if the KafkaMetrics passed in has a measurable that's an instance of Rate. If so, we will compute the delay for rate (the window should be elapsedCurrent and elapsedPrior as computed in Rate.measure()).
>
> Aditya Auradkar wrote:
> Rate/Measurable stat does not expose the windows currently. So we cannot access elapsedCurrent and elapsedPrior from the KafkaMetric object in Sensor. Couple of options:
> 1. We can simply record it in Rate.record() which is called from Sensor.record
> 2. We can check quotas in Sensor.java but not track the precise window boundaries.
Discussed offline with Aditya: I actually think the earlier approach with a slightly pessimistic delay in `Sensor` would be sufficient. Option 1 also sounds good but is slightly odd that we will then throw the exception from `Rate` directly and `Sensor.checkQuotas` as well (for other types).
After thinking through this I think it is worthwhile to consider a small change to the implementation (https://cwiki.apache.org/confluence/display/KAFKA/KIP-13+-+Quotas#KIP-13-Quotas-DelaytimeinQuotaViolationException) to _not_ include the delay time in `QuotaViolationException`. i.e., the delay really (currently) applies only to `Rate` and does not fit very well with some other types (e.g., `Max`). Instead we we could compute the delay directly in `ClientQuotaManager` and leave `Sensor.checkQuotas` as is.
- Joel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/#review95369
-----------------------------------------------------------
On Aug. 14, 2015, 2:20 a.m., Aditya Auradkar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33049/
> -----------------------------------------------------------
>
> (Updated Aug. 14, 2015, 2:20 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/ )
>
> Addressed comments from Joel and Jun
>
>
> Diffs
> -----
>
> build.gradle 1b67e628c2fca897177c12b6afad9a8700fffd1f
> 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
> core/src/main/scala/kafka/server/ClientQuotaManager.scala PRE-CREATION
> core/src/main/scala/kafka/server/KafkaApis.scala 7ea509c2c41acc00430c74e025e069a833aac4e7
> core/src/main/scala/kafka/server/KafkaConfig.scala a06f0bd40e2f90972b44b80a106f98f3d50e5e2b
> core/src/main/scala/kafka/server/KafkaServer.scala 84d4730ac634f9a5bf12a656e422fea03ad72da8
> core/src/main/scala/kafka/server/ReplicaManager.scala 2e0bbcd6e4f0e38997ea18202b249ee3553640ec
> 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/KafkaConfigTest.scala e26a7306a6ea3104b3fa3df60006c0a473bfb2cc
> core/src/test/scala/unit/kafka/server/ThrottledResponseExpirationTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/33049/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Aditya Auradkar
>
>
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/#review95369
-----------------------------------------------------------
clients/src/main/java/org/apache/kafka/common/metrics/stats/Rate.java (lines 69 - 77)
<https://reviews.apache.org/r/33049/#comment150316>
This is probably not the right place to throw QuotaViolationException. Rate.measure() can be called when the JMX value is polled and we don't want to throw an exception there.
I was thinking that another way to do this is to perhaps add the logic in Quota.acceptable(). We can probably change it to be
// if the check passes, return -1; otherwise return a delay to make the check pass.
int Quota.check(KafkaMetrics, long now).
In the implementation, we can then check if the KafkaMetrics passed in has a measurable that's an instance of Rate. If so, we will compute the delay for rate (the window should be elapsedCurrent and elapsedPrior as computed in Rate.measure()).
- Jun Rao
On Aug. 13, 2015, 4:25 a.m., Aditya Auradkar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33049/
> -----------------------------------------------------------
>
> (Updated Aug. 13, 2015, 4:25 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/ )
>
> Addressed comments from Joel and Jun
>
>
> Diffs
> -----
>
> build.gradle 1b67e628c2fca897177c12b6afad9a8700fffd1f
> 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
> core/src/main/scala/kafka/server/ClientQuotaManager.scala PRE-CREATION
> core/src/main/scala/kafka/server/KafkaApis.scala 7ea509c2c41acc00430c74e025e069a833aac4e7
> 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/KafkaConfigTest.scala f32d206d3f52f3f9f4d649c213edd7058f4b6150
> core/src/test/scala/unit/kafka/server/ThrottledResponseExpirationTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/33049/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Aditya Auradkar
>
>
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/#review101976
-----------------------------------------------------------
core/src/main/scala/kafka/server/ClientQuotaManager.scala (line 141)
<https://reviews.apache.org/r/33049/#comment159483>
Just FYI, Aditya and I discussed today. This delay seems to be slightly incorrect - it is more conservative than it should be. The correct delay is `W * (O - T) / O` We concluded it should be fine to stick to this though, but wondering if it should be documented otherwise others who try to derive it from first principles may scratch their heads as I did.
- Joel Koshy
On Aug. 15, 2015, 12:43 a.m., Aditya Auradkar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33049/
> -----------------------------------------------------------
>
> (Updated Aug. 15, 2015, 12:43 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/ )
>
> Addressed comments from Joel and Jun
>
>
> Diffs
> -----
>
> build.gradle 1b67e628c2fca897177c12b6afad9a8700fffd1f
> 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
> core/src/main/scala/kafka/server/ClientQuotaManager.scala PRE-CREATION
> core/src/main/scala/kafka/server/KafkaApis.scala 7ea509c2c41acc00430c74e025e069a833aac4e7
> core/src/main/scala/kafka/server/KafkaConfig.scala a06f0bd40e2f90972b44b80a106f98f3d50e5e2b
> core/src/main/scala/kafka/server/KafkaServer.scala 84d4730ac634f9a5bf12a656e422fea03ad72da8
> core/src/main/scala/kafka/server/ReplicaManager.scala 2e0bbcd6e4f0e38997ea18202b249ee3553640ec
> 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/KafkaConfigTest.scala e26a7306a6ea3104b3fa3df60006c0a473bfb2cc
> core/src/test/scala/unit/kafka/server/ThrottledResponseExpirationTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/33049/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Aditya Auradkar
>
>
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Aditya Auradkar <aa...@linkedin.com>.
> On Aug. 18, 2015, 11:33 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/server/ClientQuotaManager.scala, line 147
> > <https://reviews.apache.org/r/33049/diff/28/?file=1040947#file1040947line147>
> >
> > The window used here is a bit different from that used in Rate.measure() and it doesn't take into account that the current window is partial. Could we expose a windowSize(long now) method in Rate and use it here?
Hey Jun - Makes sense. Filed a ticket.. shall submit a quick patch for it.
https://issues.apache.org/jira/browse/KAFKA-2443
> On Aug. 18, 2015, 11:33 p.m., Jun Rao wrote:
> > clients/src/main/java/org/apache/kafka/common/metrics/stats/Rate.java, lines 62-65
> > <https://reviews.apache.org/r/33049/diff/28/?file=1040945#file1040945line62>
> >
> > Actually, the original computation on the window seems more accurate since it handles the initial case when not all the sample windows have been populated. Is there a particular reason to change it?
This was discussed in a fair bit of detail between Dong, Jay and myself. Here is a ticket: https://issues.apache.org/jira/browse/KAFKA-2191
https://reviews.apache.org/r/34170/
Basically, we were having issues with very large metric values when the metric was very recently created.
- Aditya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/#review95793
-----------------------------------------------------------
On Aug. 15, 2015, 12:43 a.m., Aditya Auradkar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33049/
> -----------------------------------------------------------
>
> (Updated Aug. 15, 2015, 12:43 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/ )
>
> Addressed comments from Joel and Jun
>
>
> Diffs
> -----
>
> build.gradle 1b67e628c2fca897177c12b6afad9a8700fffd1f
> 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
> core/src/main/scala/kafka/server/ClientQuotaManager.scala PRE-CREATION
> core/src/main/scala/kafka/server/KafkaApis.scala 7ea509c2c41acc00430c74e025e069a833aac4e7
> core/src/main/scala/kafka/server/KafkaConfig.scala a06f0bd40e2f90972b44b80a106f98f3d50e5e2b
> core/src/main/scala/kafka/server/KafkaServer.scala 84d4730ac634f9a5bf12a656e422fea03ad72da8
> core/src/main/scala/kafka/server/ReplicaManager.scala 2e0bbcd6e4f0e38997ea18202b249ee3553640ec
> 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/KafkaConfigTest.scala e26a7306a6ea3104b3fa3df60006c0a473bfb2cc
> core/src/test/scala/unit/kafka/server/ThrottledResponseExpirationTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/33049/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Aditya Auradkar
>
>
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/#review95793
-----------------------------------------------------------
clients/src/main/java/org/apache/kafka/common/metrics/stats/Rate.java (lines 62 - 65)
<https://reviews.apache.org/r/33049/#comment150927>
Actually, the original computation on the window seems more accurate since it handles the initial case when not all the sample windows have been populated. Is there a particular reason to change it?
core/src/main/scala/kafka/server/ClientQuotaManager.scala (line 147)
<https://reviews.apache.org/r/33049/#comment150934>
The window used here is a bit different from that used in Rate.measure() and it doesn't take into account that the current window is partial. Could we expose a windowSize(long now) method in Rate and use it here?
- Jun Rao
On Aug. 15, 2015, 12:43 a.m., Aditya Auradkar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33049/
> -----------------------------------------------------------
>
> (Updated Aug. 15, 2015, 12:43 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/ )
>
> Addressed comments from Joel and Jun
>
>
> Diffs
> -----
>
> build.gradle 1b67e628c2fca897177c12b6afad9a8700fffd1f
> 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
> core/src/main/scala/kafka/server/ClientQuotaManager.scala PRE-CREATION
> core/src/main/scala/kafka/server/KafkaApis.scala 7ea509c2c41acc00430c74e025e069a833aac4e7
> core/src/main/scala/kafka/server/KafkaConfig.scala a06f0bd40e2f90972b44b80a106f98f3d50e5e2b
> core/src/main/scala/kafka/server/KafkaServer.scala 84d4730ac634f9a5bf12a656e422fea03ad72da8
> core/src/main/scala/kafka/server/ReplicaManager.scala 2e0bbcd6e4f0e38997ea18202b249ee3553640ec
> 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/KafkaConfigTest.scala e26a7306a6ea3104b3fa3df60006c0a473bfb2cc
> core/src/test/scala/unit/kafka/server/ThrottledResponseExpirationTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/33049/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Aditya Auradkar
>
>
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/#review95496
-----------------------------------------------------------
Ship it!
Minor edits that I will take care of on check-in.
core/src/main/scala/kafka/server/ClientQuotaManager.scala (line 122)
<https://reviews.apache.org/r/33049/#comment150532>
can remove
core/src/main/scala/kafka/server/ClientQuotaManager.scala (line 148)
<https://reviews.apache.org/r/33049/#comment150531>
Can omit `return`
- Joel Koshy
On Aug. 15, 2015, 12:43 a.m., Aditya Auradkar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33049/
> -----------------------------------------------------------
>
> (Updated Aug. 15, 2015, 12:43 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/ )
>
> Addressed comments from Joel and Jun
>
>
> Diffs
> -----
>
> build.gradle 1b67e628c2fca897177c12b6afad9a8700fffd1f
> 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
> core/src/main/scala/kafka/server/ClientQuotaManager.scala PRE-CREATION
> core/src/main/scala/kafka/server/KafkaApis.scala 7ea509c2c41acc00430c74e025e069a833aac4e7
> core/src/main/scala/kafka/server/KafkaConfig.scala a06f0bd40e2f90972b44b80a106f98f3d50e5e2b
> core/src/main/scala/kafka/server/KafkaServer.scala 84d4730ac634f9a5bf12a656e422fea03ad72da8
> core/src/main/scala/kafka/server/ReplicaManager.scala 2e0bbcd6e4f0e38997ea18202b249ee3553640ec
> 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/KafkaConfigTest.scala e26a7306a6ea3104b3fa3df60006c0a473bfb2cc
> core/src/test/scala/unit/kafka/server/ThrottledResponseExpirationTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/33049/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Aditya Auradkar
>
>
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Aditya Auradkar <aa...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/
-----------------------------------------------------------
(Updated Aug. 15, 2015, 12:43 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)
-------
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/ )
Addressed comments from Joel and Jun
Diffs
-----
build.gradle 1b67e628c2fca897177c12b6afad9a8700fffd1f
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
core/src/main/scala/kafka/server/ClientQuotaManager.scala PRE-CREATION
core/src/main/scala/kafka/server/KafkaApis.scala 7ea509c2c41acc00430c74e025e069a833aac4e7
core/src/main/scala/kafka/server/KafkaConfig.scala a06f0bd40e2f90972b44b80a106f98f3d50e5e2b
core/src/main/scala/kafka/server/KafkaServer.scala 84d4730ac634f9a5bf12a656e422fea03ad72da8
core/src/main/scala/kafka/server/ReplicaManager.scala 2e0bbcd6e4f0e38997ea18202b249ee3553640ec
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/KafkaConfigTest.scala e26a7306a6ea3104b3fa3df60006c0a473bfb2cc
core/src/test/scala/unit/kafka/server/ThrottledResponseExpirationTest.scala PRE-CREATION
Diff: https://reviews.apache.org/r/33049/diff/
Testing
-------
Thanks,
Aditya Auradkar
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Aditya Auradkar <aa...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/
-----------------------------------------------------------
(Updated Aug. 15, 2015, 12:43 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)
-------
Signed-off-by: Aditya Auradkar <aa...@linkedin.com>
Addressing Joel's comments
Minor imports changes
Added testcase to verify that replication traffic is not throttled
Tmp commit
Fixing test failure
Minor
Addressing Joel's comments
Addressing comments
Addressing comments
Addressing Juns comments
Minor checkstyle changes
fixed test case
Addressing Juns comments
Addressing Juns comments
Addressing Juns comments
Minor change
Minor change
Moving MockTime back to test
Addressing Joels comments
Minor import change
Addressing joels comments
Diffs (updated)
-----
build.gradle 1b67e628c2fca897177c12b6afad9a8700fffd1f
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
core/src/main/scala/kafka/server/ClientQuotaManager.scala PRE-CREATION
core/src/main/scala/kafka/server/KafkaApis.scala 7ea509c2c41acc00430c74e025e069a833aac4e7
core/src/main/scala/kafka/server/KafkaConfig.scala a06f0bd40e2f90972b44b80a106f98f3d50e5e2b
core/src/main/scala/kafka/server/KafkaServer.scala 84d4730ac634f9a5bf12a656e422fea03ad72da8
core/src/main/scala/kafka/server/ReplicaManager.scala 2e0bbcd6e4f0e38997ea18202b249ee3553640ec
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/KafkaConfigTest.scala e26a7306a6ea3104b3fa3df60006c0a473bfb2cc
core/src/test/scala/unit/kafka/server/ThrottledResponseExpirationTest.scala PRE-CREATION
Diff: https://reviews.apache.org/r/33049/diff/
Testing
-------
Thanks,
Aditya Auradkar
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Aditya Auradkar <aa...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/
-----------------------------------------------------------
(Updated Aug. 14, 2015, 2:20 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)
-------
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/ )
Addressed comments from Joel and Jun
Diffs
-----
build.gradle 1b67e628c2fca897177c12b6afad9a8700fffd1f
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
core/src/main/scala/kafka/server/ClientQuotaManager.scala PRE-CREATION
core/src/main/scala/kafka/server/KafkaApis.scala 7ea509c2c41acc00430c74e025e069a833aac4e7
core/src/main/scala/kafka/server/KafkaConfig.scala a06f0bd40e2f90972b44b80a106f98f3d50e5e2b
core/src/main/scala/kafka/server/KafkaServer.scala 84d4730ac634f9a5bf12a656e422fea03ad72da8
core/src/main/scala/kafka/server/ReplicaManager.scala 2e0bbcd6e4f0e38997ea18202b249ee3553640ec
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/KafkaConfigTest.scala e26a7306a6ea3104b3fa3df60006c0a473bfb2cc
core/src/test/scala/unit/kafka/server/ThrottledResponseExpirationTest.scala PRE-CREATION
Diff: https://reviews.apache.org/r/33049/diff/
Testing
-------
Thanks,
Aditya Auradkar
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Aditya Auradkar <aa...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/
-----------------------------------------------------------
(Updated Aug. 14, 2015, 2:19 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)
-------
Signed-off-by: Aditya Auradkar <aa...@linkedin.com>
Addressing Joel's comments
Minor imports changes
Added testcase to verify that replication traffic is not throttled
Tmp commit
Fixing test failure
Minor
Addressing Joel's comments
Addressing comments
Addressing comments
Addressing Juns comments
Minor checkstyle changes
fixed test case
Addressing Juns comments
Addressing Juns comments
Addressing Juns comments
Minor change
Minor change
Moving MockTime back to test
Addressing Joels comments
Minor import change
Diffs (updated)
-----
build.gradle 1b67e628c2fca897177c12b6afad9a8700fffd1f
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
core/src/main/scala/kafka/server/ClientQuotaManager.scala PRE-CREATION
core/src/main/scala/kafka/server/KafkaApis.scala 7ea509c2c41acc00430c74e025e069a833aac4e7
core/src/main/scala/kafka/server/KafkaConfig.scala a06f0bd40e2f90972b44b80a106f98f3d50e5e2b
core/src/main/scala/kafka/server/KafkaServer.scala 84d4730ac634f9a5bf12a656e422fea03ad72da8
core/src/main/scala/kafka/server/ReplicaManager.scala 2e0bbcd6e4f0e38997ea18202b249ee3553640ec
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/KafkaConfigTest.scala e26a7306a6ea3104b3fa3df60006c0a473bfb2cc
core/src/test/scala/unit/kafka/server/ThrottledResponseExpirationTest.scala PRE-CREATION
Diff: https://reviews.apache.org/r/33049/diff/
Testing
-------
Thanks,
Aditya Auradkar
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Aditya Auradkar <aa...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/
-----------------------------------------------------------
(Updated Aug. 14, 2015, 2:09 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)
-------
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/ )
Addressed comments from Joel and Jun
Diffs
-----
build.gradle 1b67e628c2fca897177c12b6afad9a8700fffd1f
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
core/src/main/scala/kafka/server/ClientQuotaManager.scala PRE-CREATION
core/src/main/scala/kafka/server/KafkaApis.scala 7ea509c2c41acc00430c74e025e069a833aac4e7
core/src/main/scala/kafka/server/KafkaConfig.scala a06f0bd40e2f90972b44b80a106f98f3d50e5e2b
core/src/main/scala/kafka/server/KafkaServer.scala 84d4730ac634f9a5bf12a656e422fea03ad72da8
core/src/main/scala/kafka/server/ReplicaManager.scala 2e0bbcd6e4f0e38997ea18202b249ee3553640ec
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/KafkaConfigTest.scala e26a7306a6ea3104b3fa3df60006c0a473bfb2cc
core/src/test/scala/unit/kafka/server/ThrottledResponseExpirationTest.scala PRE-CREATION
Diff: https://reviews.apache.org/r/33049/diff/
Testing
-------
Thanks,
Aditya Auradkar
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Aditya Auradkar <aa...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/
-----------------------------------------------------------
(Updated Aug. 14, 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)
-------
Signed-off-by: Aditya Auradkar <aa...@linkedin.com>
Addressing Joel's comments
Minor imports changes
Added testcase to verify that replication traffic is not throttled
Tmp commit
Fixing test failure
Minor
Addressing Joel's comments
Addressing comments
Addressing comments
Addressing Juns comments
Minor checkstyle changes
fixed test case
Addressing Juns comments
Addressing Juns comments
Addressing Juns comments
Minor change
Minor change
Moving MockTime back to test
Addressing Joels comments
Diffs (updated)
-----
build.gradle 1b67e628c2fca897177c12b6afad9a8700fffd1f
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
core/src/main/scala/kafka/server/ClientQuotaManager.scala PRE-CREATION
core/src/main/scala/kafka/server/KafkaApis.scala 7ea509c2c41acc00430c74e025e069a833aac4e7
core/src/main/scala/kafka/server/KafkaConfig.scala a06f0bd40e2f90972b44b80a106f98f3d50e5e2b
core/src/main/scala/kafka/server/KafkaServer.scala 84d4730ac634f9a5bf12a656e422fea03ad72da8
core/src/main/scala/kafka/server/ReplicaManager.scala 2e0bbcd6e4f0e38997ea18202b249ee3553640ec
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/KafkaConfigTest.scala e26a7306a6ea3104b3fa3df60006c0a473bfb2cc
core/src/test/scala/unit/kafka/server/ThrottledResponseExpirationTest.scala PRE-CREATION
Diff: https://reviews.apache.org/r/33049/diff/
Testing
-------
Thanks,
Aditya Auradkar
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Joel Koshy <jj...@gmail.com>.
> On Aug. 13, 2015, 11:13 p.m., Joel Koshy wrote:
> > build.gradle, line 383
> > <https://reviews.apache.org/r/33049/diff/25/?file=1039165#file1039165line383>
> >
> > I don't think these changes 383-385 are needed.
> >
> > Also, (unrelated to this change) I'm seeing NPE errors in running unit tests (e.g., QuotasTest) - can you verify?
Not sure why it didn't seem to affect your environment, but you need the `@Before` and `@After` annotations on your `setUp` and `tearDown` methods.
- Joel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/#review95344
-----------------------------------------------------------
On Aug. 13, 2015, 4:25 a.m., Aditya Auradkar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33049/
> -----------------------------------------------------------
>
> (Updated Aug. 13, 2015, 4:25 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/ )
>
> Addressed comments from Joel and Jun
>
>
> Diffs
> -----
>
> build.gradle 1b67e628c2fca897177c12b6afad9a8700fffd1f
> 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
> core/src/main/scala/kafka/server/ClientQuotaManager.scala PRE-CREATION
> core/src/main/scala/kafka/server/KafkaApis.scala 7ea509c2c41acc00430c74e025e069a833aac4e7
> 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/KafkaConfigTest.scala f32d206d3f52f3f9f4d649c213edd7058f4b6150
> core/src/test/scala/unit/kafka/server/ThrottledResponseExpirationTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/33049/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Aditya Auradkar
>
>
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/#review95344
-----------------------------------------------------------
build.gradle (line 383)
<https://reviews.apache.org/r/33049/#comment150256>
I don't think these changes 383-385 are needed.
Also, (unrelated to this change) I'm seeing NPE errors in running unit tests (e.g., QuotasTest) - can you verify?
- Joel Koshy
On Aug. 13, 2015, 4:25 a.m., Aditya Auradkar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33049/
> -----------------------------------------------------------
>
> (Updated Aug. 13, 2015, 4:25 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/ )
>
> Addressed comments from Joel and Jun
>
>
> Diffs
> -----
>
> build.gradle 1b67e628c2fca897177c12b6afad9a8700fffd1f
> 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
> core/src/main/scala/kafka/server/ClientQuotaManager.scala PRE-CREATION
> core/src/main/scala/kafka/server/KafkaApis.scala 7ea509c2c41acc00430c74e025e069a833aac4e7
> 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/KafkaConfigTest.scala f32d206d3f52f3f9f4d649c213edd7058f4b6150
> core/src/test/scala/unit/kafka/server/ThrottledResponseExpirationTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/33049/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Aditya Auradkar
>
>
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Aditya Auradkar <aa...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/
-----------------------------------------------------------
(Updated Aug. 13, 2015, 4:25 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)
-------
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/ )
Addressed comments from Joel and Jun
Diffs
-----
build.gradle 1b67e628c2fca897177c12b6afad9a8700fffd1f
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
core/src/main/scala/kafka/server/ClientQuotaManager.scala PRE-CREATION
core/src/main/scala/kafka/server/KafkaApis.scala 7ea509c2c41acc00430c74e025e069a833aac4e7
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/KafkaConfigTest.scala f32d206d3f52f3f9f4d649c213edd7058f4b6150
core/src/test/scala/unit/kafka/server/ThrottledResponseExpirationTest.scala PRE-CREATION
Diff: https://reviews.apache.org/r/33049/diff/
Testing
-------
Thanks,
Aditya Auradkar
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Aditya Auradkar <aa...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/
-----------------------------------------------------------
(Updated Aug. 13, 2015, 4:24 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)
-------
Signed-off-by: Aditya Auradkar <aa...@linkedin.com>
Addressing Joel's comments
Minor imports changes
Added testcase to verify that replication traffic is not throttled
Tmp commit
Fixing test failure
Minor
Addressing Joel's comments
Addressing comments
Addressing comments
Addressing Juns comments
Minor checkstyle changes
fixed test case
Addressing Juns comments
Addressing Juns comments
Addressing Juns comments
Minor change
Minor change
Moving MockTime back to test
Diffs (updated)
-----
build.gradle 1b67e628c2fca897177c12b6afad9a8700fffd1f
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
core/src/main/scala/kafka/server/ClientQuotaManager.scala PRE-CREATION
core/src/main/scala/kafka/server/KafkaApis.scala 7ea509c2c41acc00430c74e025e069a833aac4e7
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/KafkaConfigTest.scala f32d206d3f52f3f9f4d649c213edd7058f4b6150
core/src/test/scala/unit/kafka/server/ThrottledResponseExpirationTest.scala PRE-CREATION
Diff: https://reviews.apache.org/r/33049/diff/
Testing
-------
Thanks,
Aditya Auradkar
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Aditya Auradkar <aa...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/
-----------------------------------------------------------
(Updated Aug. 12, 2015, 7:09 p.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)
-------
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/ )
Addressed comments from Joel and Jun
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 7ea509c2c41acc00430c74e025e069a833aac4e7
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/KafkaConfigTest.scala f32d206d3f52f3f9f4d649c213edd7058f4b6150
core/src/test/scala/unit/kafka/server/ThrottledResponseExpirationTest.scala PRE-CREATION
Diff: https://reviews.apache.org/r/33049/diff/
Testing
-------
Thanks,
Aditya Auradkar
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Aditya Auradkar <aa...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/
-----------------------------------------------------------
(Updated Aug. 12, 2015, 7:08 p.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)
-------
Signed-off-by: Aditya Auradkar <aa...@linkedin.com>
Addressing Joel's comments
Minor imports changes
Added testcase to verify that replication traffic is not throttled
Tmp commit
Fixing test failure
Minor
Addressing Joel's comments
Addressing comments
Addressing comments
Addressing Juns comments
Minor checkstyle changes
fixed test case
Addressing Juns comments
Addressing Juns comments
Addressing Juns comments
Minor change
Minor change
Diffs (updated)
-----
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 7ea509c2c41acc00430c74e025e069a833aac4e7
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/KafkaConfigTest.scala f32d206d3f52f3f9f4d649c213edd7058f4b6150
core/src/test/scala/unit/kafka/server/ThrottledResponseExpirationTest.scala PRE-CREATION
Diff: https://reviews.apache.org/r/33049/diff/
Testing
-------
Thanks,
Aditya Auradkar
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Aditya Auradkar <aa...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/
-----------------------------------------------------------
(Updated Aug. 12, 2015, 7:05 p.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)
-------
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/ )
Addressed comments from Joel and Jun
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 7ea509c2c41acc00430c74e025e069a833aac4e7
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/KafkaConfigTest.scala f32d206d3f52f3f9f4d649c213edd7058f4b6150
core/src/test/scala/unit/kafka/server/ThrottledResponseExpirationTest.scala PRE-CREATION
Diff: https://reviews.apache.org/r/33049/diff/
Testing
-------
Thanks,
Aditya Auradkar
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Aditya Auradkar <aa...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/
-----------------------------------------------------------
(Updated Aug. 12, 2015, 7:04 p.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)
-------
Signed-off-by: Aditya Auradkar <aa...@linkedin.com>
Addressing Joel's comments
Minor imports changes
Added testcase to verify that replication traffic is not throttled
Tmp commit
Fixing test failure
Minor
Addressing Joel's comments
Addressing comments
Addressing comments
Addressing Juns comments
Minor checkstyle changes
fixed test case
Addressing Juns comments
Addressing Juns comments
Addressing Juns comments
Minor change
Diffs (updated)
-----
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 7ea509c2c41acc00430c74e025e069a833aac4e7
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/KafkaConfigTest.scala f32d206d3f52f3f9f4d649c213edd7058f4b6150
core/src/test/scala/unit/kafka/server/ThrottledResponseExpirationTest.scala PRE-CREATION
Diff: https://reviews.apache.org/r/33049/diff/
Testing
-------
Thanks,
Aditya Auradkar
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Aditya Auradkar <aa...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/
-----------------------------------------------------------
(Updated Aug. 12, 2015, 7:03 p.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)
-------
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/ )
Addressed comments from Joel and Jun
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 7ea509c2c41acc00430c74e025e069a833aac4e7
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/KafkaConfigTest.scala f32d206d3f52f3f9f4d649c213edd7058f4b6150
core/src/test/scala/unit/kafka/server/ThrottledResponseExpirationTest.scala PRE-CREATION
Diff: https://reviews.apache.org/r/33049/diff/
Testing
-------
Thanks,
Aditya Auradkar
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Aditya Auradkar <aa...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/
-----------------------------------------------------------
(Updated Aug. 12, 2015, 7:02 p.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)
-------
Signed-off-by: Aditya Auradkar <aa...@linkedin.com>
Addressing Joel's comments
Minor imports changes
Added testcase to verify that replication traffic is not throttled
Tmp commit
Fixing test failure
Minor
Addressing Joel's comments
Addressing comments
Addressing comments
Addressing Juns comments
Minor checkstyle changes
fixed test case
Addressing Juns comments
Addressing Juns comments
Addressing Juns comments
Diffs (updated)
-----
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 7ea509c2c41acc00430c74e025e069a833aac4e7
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/KafkaConfigTest.scala f32d206d3f52f3f9f4d649c213edd7058f4b6150
core/src/test/scala/unit/kafka/server/ThrottledResponseExpirationTest.scala PRE-CREATION
Diff: https://reviews.apache.org/r/33049/diff/
Testing
-------
Thanks,
Aditya Auradkar
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Aditya Auradkar <aa...@linkedin.com>.
> On Aug. 12, 2015, 12:34 a.m., Jun Rao wrote:
> > clients/src/main/java/org/apache/kafka/common/metrics/Sensor.java, lines 131-150
> > <https://reviews.apache.org/r/33049/diff/21/?file=1037164#file1037164line131>
> >
> > I think the comment can be a simpler. Basically, if O is the observed rate and T is the target rate over a window of W, to bring O down to T, we need to add a delay of X to W such that O * W / (W + X) = T. Solving for X, we get X = W*(O - T)/T.
Thanks. Your comment is much better
> On Aug. 12, 2015, 12:34 a.m., Jun Rao wrote:
> > clients/src/main/java/org/apache/kafka/common/metrics/Sensor.java, line 153
> > <https://reviews.apache.org/r/33049/diff/21/?file=1037164#file1037164line153>
> >
> > Instead of using config.samples() * config.timeWindowMs(), shouldn't we use the formula elapsedCurrentWindowMs + elapsedPriorWindowsMs that we used in Rate.measure()? We can pass in now all the way from record().
Currently, the metric does not expose the underlying stat so I cannot do this computation in Sensor.java. The newer patch computes this in Rate.java and throws a QuotaViolationException from within. This way, we dont need to have a special comment in Sensor.java regarding rate metrics.
- Aditya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/#review95033
-----------------------------------------------------------
On Aug. 11, 2015, 4:58 a.m., Aditya Auradkar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33049/
> -----------------------------------------------------------
>
> (Updated Aug. 11, 2015, 4:58 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/ )
>
> Addressed comments from Joel and Jun
>
>
> 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 7ea509c2c41acc00430c74e025e069a833aac4e7
> 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/KafkaConfigTest.scala f32d206d3f52f3f9f4d649c213edd7058f4b6150
> core/src/test/scala/unit/kafka/server/ThrottledResponseExpirationTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/33049/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Aditya Auradkar
>
>
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/#review95033
-----------------------------------------------------------
Just a couple of comments below. Otherwise, LGTM.
clients/src/main/java/org/apache/kafka/common/metrics/Sensor.java (lines 131 - 150)
<https://reviews.apache.org/r/33049/#comment149805>
I think the comment can be a simpler. Basically, if O is the observed rate and T is the target rate over a window of W, to bring O down to T, we need to add a delay of X to W such that O * W / (W + X) = T. Solving for X, we get X = W*(O - T)/T.
clients/src/main/java/org/apache/kafka/common/metrics/Sensor.java (line 153)
<https://reviews.apache.org/r/33049/#comment149804>
Instead of using config.samples() * config.timeWindowMs(), shouldn't we use the formula elapsedCurrentWindowMs + elapsedPriorWindowsMs that we used in Rate.measure()? We can pass in now all the way from record().
- Jun Rao
On Aug. 11, 2015, 4:58 a.m., Aditya Auradkar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33049/
> -----------------------------------------------------------
>
> (Updated Aug. 11, 2015, 4:58 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/ )
>
> Addressed comments from Joel and Jun
>
>
> 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 7ea509c2c41acc00430c74e025e069a833aac4e7
> 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/KafkaConfigTest.scala f32d206d3f52f3f9f4d649c213edd7058f4b6150
> core/src/test/scala/unit/kafka/server/ThrottledResponseExpirationTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/33049/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Aditya Auradkar
>
>
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Aditya Auradkar <aa...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/
-----------------------------------------------------------
(Updated Aug. 11, 2015, 4:58 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)
-------
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/ )
Addressed comments from Joel and Jun
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 7ea509c2c41acc00430c74e025e069a833aac4e7
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/KafkaConfigTest.scala f32d206d3f52f3f9f4d649c213edd7058f4b6150
core/src/test/scala/unit/kafka/server/ThrottledResponseExpirationTest.scala PRE-CREATION
Diff: https://reviews.apache.org/r/33049/diff/
Testing
-------
Thanks,
Aditya Auradkar
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Aditya Auradkar <aa...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/
-----------------------------------------------------------
(Updated Aug. 11, 2015, 4:57 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)
-------
Signed-off-by: Aditya Auradkar <aa...@linkedin.com>
Addressing Joel's comments
Minor imports changes
Added testcase to verify that replication traffic is not throttled
Tmp commit
Fixing test failure
Minor
Addressing Joel's comments
Addressing comments
Addressing comments
Addressing Juns comments
Minor checkstyle changes
fixed test case
Addressing Juns comments
Diffs (updated)
-----
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 7ea509c2c41acc00430c74e025e069a833aac4e7
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/KafkaConfigTest.scala f32d206d3f52f3f9f4d649c213edd7058f4b6150
core/src/test/scala/unit/kafka/server/ThrottledResponseExpirationTest.scala PRE-CREATION
Diff: https://reviews.apache.org/r/33049/diff/
Testing
-------
Thanks,
Aditya Auradkar
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Aditya Auradkar <aa...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/
-----------------------------------------------------------
(Updated Aug. 10, 2015, 8:49 p.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)
-------
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/ )
Addressed comments from Joel and Jun
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 7ea509c2c41acc00430c74e025e069a833aac4e7
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/KafkaConfigTest.scala f32d206d3f52f3f9f4d649c213edd7058f4b6150
core/src/test/scala/unit/kafka/server/ThrottledResponseExpirationTest.scala PRE-CREATION
Diff: https://reviews.apache.org/r/33049/diff/
Testing
-------
Thanks,
Aditya Auradkar
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Aditya Auradkar <aa...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/
-----------------------------------------------------------
(Updated Aug. 10, 2015, 8:48 p.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)
-------
Signed-off-by: Aditya Auradkar <aa...@linkedin.com>
Addressing Joel's comments
Minor imports changes
Added testcase to verify that replication traffic is not throttled
Tmp commit
Fixing test failure
Minor
Addressing Joel's comments
Addressing comments
Addressing comments
Addressing Juns comments
Minor checkstyle changes
fixed test case
Diffs (updated)
-----
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 7ea509c2c41acc00430c74e025e069a833aac4e7
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/KafkaConfigTest.scala f32d206d3f52f3f9f4d649c213edd7058f4b6150
core/src/test/scala/unit/kafka/server/ThrottledResponseExpirationTest.scala PRE-CREATION
Diff: https://reviews.apache.org/r/33049/diff/
Testing
-------
Thanks,
Aditya Auradkar
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Aditya Auradkar <aa...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/
-----------------------------------------------------------
(Updated Aug. 7, 2015, 6:28 p.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)
-------
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/ )
Addressed comments from Joel and Jun
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
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Aditya Auradkar <aa...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/
-----------------------------------------------------------
(Updated Aug. 7, 2015, 6:27 p.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)
-------
Signed-off-by: Aditya Auradkar <aa...@linkedin.com>
Addressing Joel's comments
Minor imports changes
Added testcase to verify that replication traffic is not throttled
Tmp commit
Fixing test failure
Minor
Addressing Joel's comments
Addressing comments
Addressing comments
Addressing Juns comments
Diffs (updated)
-----
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
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Aditya Auradkar <aa...@linkedin.com>.
> On Aug. 6, 2015, 4:17 p.m., Jun Rao wrote:
> > A few more comments.
> >
> > We need to be careful with sensors at the client-id level. Clients can come and go (e.g. console consumer). We probably don't want to hold sensors that are not longer actively used since it takes memory. So, we will need some way of removing inactive sensors. Not sure if we should add this at the metric level or at the quota level.
>
> Jun Rao wrote:
> Did you address the comment on removing inactive sensors?
Ah, I missed this comment. Good point.. we should be removing these sensor objects. I think we should handle this in the Metrics library itself.. it would be nice to support sensors that can be garbage collected after a certain period of inactivity (if the sensor is marked as eligible for removal). The new metrics library does not support removal of sensors right now so I filed a ticket as followup since it might need a bit more discussion: https://issues.apache.org/jira/browse/KAFKA-2419
- Aditya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/#review94412
-----------------------------------------------------------
On Aug. 10, 2015, 8:49 p.m., Aditya Auradkar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33049/
> -----------------------------------------------------------
>
> (Updated Aug. 10, 2015, 8:49 p.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/ )
>
> Addressed comments from Joel and Jun
>
>
> 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 7ea509c2c41acc00430c74e025e069a833aac4e7
> 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/KafkaConfigTest.scala f32d206d3f52f3f9f4d649c213edd7058f4b6150
> core/src/test/scala/unit/kafka/server/ThrottledResponseExpirationTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/33049/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Aditya Auradkar
>
>
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Joel Koshy <jj...@gmail.com>.
> On Aug. 6, 2015, 4:17 p.m., Jun Rao wrote:
> > A few more comments.
> >
> > We need to be careful with sensors at the client-id level. Clients can come and go (e.g. console consumer). We probably don't want to hold sensors that are not longer actively used since it takes memory. So, we will need some way of removing inactive sensors. Not sure if we should add this at the metric level or at the quota level.
>
> Jun Rao wrote:
> Did you address the comment on removing inactive sensors?
>
> Aditya Auradkar wrote:
> Ah, I missed this comment. Good point.. we should be removing these sensor objects. I think we should handle this in the Metrics library itself.. it would be nice to support sensors that can be garbage collected after a certain period of inactivity (if the sensor is marked as eligible for removal). The new metrics library does not support removal of sensors right now so I filed a ticket as followup since it might need a bit more discussion: https://issues.apache.org/jira/browse/KAFKA-2419
@Jun - good point. Aditya, minor feedback on that ticket: it may be better to not make it time-based (config driven) but proactively remove sensors when required. E.g., when a client closes a connection (for client-id sensors) or topics get deleted (for topic sensors) and so on.
- Joel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/#review94412
-----------------------------------------------------------
On Aug. 12, 2015, 7:09 p.m., Aditya Auradkar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33049/
> -----------------------------------------------------------
>
> (Updated Aug. 12, 2015, 7:09 p.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/ )
>
> Addressed comments from Joel and Jun
>
>
> 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 7ea509c2c41acc00430c74e025e069a833aac4e7
> 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/KafkaConfigTest.scala f32d206d3f52f3f9f4d649c213edd7058f4b6150
> core/src/test/scala/unit/kafka/server/ThrottledResponseExpirationTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/33049/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Aditya Auradkar
>
>
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Jun Rao <ju...@gmail.com>.
> On Aug. 6, 2015, 4:17 p.m., Jun Rao wrote:
> > A few more comments.
> >
> > We need to be careful with sensors at the client-id level. Clients can come and go (e.g. console consumer). We probably don't want to hold sensors that are not longer actively used since it takes memory. So, we will need some way of removing inactive sensors. Not sure if we should add this at the metric level or at the quota level.
Did you address the comment on removing inactive sensors?
- Jun
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/#review94412
-----------------------------------------------------------
On Aug. 10, 2015, 8:49 p.m., Aditya Auradkar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33049/
> -----------------------------------------------------------
>
> (Updated Aug. 10, 2015, 8:49 p.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/ )
>
> Addressed comments from Joel and Jun
>
>
> 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 7ea509c2c41acc00430c74e025e069a833aac4e7
> 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/KafkaConfigTest.scala f32d206d3f52f3f9f4d649c213edd7058f4b6150
> core/src/test/scala/unit/kafka/server/ThrottledResponseExpirationTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/33049/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Aditya Auradkar
>
>
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Aditya Auradkar <aa...@linkedin.com>.
> 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
>
>
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/#review94412
-----------------------------------------------------------
A few more comments.
We need to be careful with sensors at the client-id level. Clients can come and go (e.g. console consumer). We probably don't want to hold sensors that are not longer actively used since it takes memory. So, we will need some way of removing inactive sensors. Not sure if we should add this at the metric level or at the quota level.
core/src/main/scala/kafka/server/ClientQuotaManager.scala (lines 154 - 201)
<https://reviews.apache.org/r/33049/#comment148998>
Not sure why the lock is needed. metrics.sensor() is synchronized and alreayd does the getOrCreate thing.
core/src/main/scala/kafka/server/ClientQuotaManager.scala (lines 187 - 191)
<https://reviews.apache.org/r/33049/#comment149000>
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).
- Jun Rao
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
>
>
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Aditya Auradkar <aa...@linkedin.com>.
> On Aug. 6, 2015, 2:02 a.m., Jun Rao wrote:
> > core/src/main/scala/kafka/server/ClientQuotaManager.scala, lines 208-211
> > <https://reviews.apache.org/r/33049/diff/18/?file=1032147#file1032147line208>
> >
> > Instead of creating a new metric instance, we probably should just reuse the same metric instance created in KafkaServer. This will make reporting easier.
We do use the same Metrics instance. This is simply the metric config which may be different for quotas (smaller time window, more samples etc..)
> On Aug. 6, 2015, 2:02 a.m., Jun Rao wrote:
> > core/src/main/scala/kafka/server/KafkaConfig.scala, line 419
> > <https://reviews.apache.org/r/33049/diff/18/?file=1032149#file1032149line419>
> >
> > What's the use case of the delay factor?
I added it during development. I felt that we could use this to increase/decrease the actual computed delays in case we want to penalize clients less or more. If you dont think this adds any value, I can remove it
> On Aug. 6, 2015, 2:02 a.m., Jun Rao wrote:
> > clients/src/main/java/org/apache/kafka/common/metrics/Sensor.java, lines 135-139
> > <https://reviews.apache.org/r/33049/diff/18/?file=1032143#file1032143line135>
> >
> > Is that calculation here right? Based on the calculation in Throttler, it should be sth like the following.
> >
> > metricValue/quota.bound() - windowSize
> >
> > Also, the window size is calculated as config.sampels() * config.timeWindows(), which is inaccurate. The last window is not complete. So we need to take the current time into consideration.
> >
> > Finally, it seems that delayTime only makes sense for rates and not for anything else. Perhaps we can at least add a comment.
Hey Jun -
Can you elaborate a little? How would we use the current time exactly? It is not clear to me how subtracting the windowSize (time unit) from a fraction (metricValue/quota.bound()) gives the right delay.
I also added a comment for delayTime making sense for rates only.
- Aditya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/#review94333
-----------------------------------------------------------
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
>
>
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Jun Rao <ju...@gmail.com>.
> On Aug. 6, 2015, 2:02 a.m., Jun Rao wrote:
> > clients/src/main/java/org/apache/kafka/common/metrics/Sensor.java, lines 135-139
> > <https://reviews.apache.org/r/33049/diff/18/?file=1032143#file1032143line135>
> >
> > Is that calculation here right? Based on the calculation in Throttler, it should be sth like the following.
> >
> > metricValue/quota.bound() - windowSize
> >
> > Also, the window size is calculated as config.sampels() * config.timeWindows(), which is inaccurate. The last window is not complete. So we need to take the current time into consideration.
> >
> > Finally, it seems that delayTime only makes sense for rates and not for anything else. Perhaps we can at least add a comment.
>
> Aditya Auradkar wrote:
> Hey Jun -
>
> Can you elaborate a little? How would we use the current time exactly? It is not clear to me how subtracting the windowSize (time unit) from a fraction (metricValue/quota.bound()) gives the right delay.
>
> I also added a comment for delayTime making sense for rates only.
>
> Aditya Auradkar wrote:
> I dug into the Throttler code a bit. Basically - The metricValue is the absolute actual value and not a rate.
> Throttler delay: (currentValueOfCounter/quotaRate) - elapsedTime
> Current Sensor delay: ((currentRateValue - quotaRate)/quotaRate) * elapsedTime
>
>
> Lets take an example (all in seconds):
> quotaRate = 10QPS
> elapedSec = 20 (Lets say 21 windows of 1 second each. The last second has not started yet)
> currentValueOfCounter = 250
> currentRate = (250/20) = 12.5 (assuming 20 elapsed seconds. Current second may be incomplete)
>
> Throttler Formula delay = (currentValueOfCounter/quotaRate) - elapsedTime = (250/10) - 20 = 5 second delay
> Current Sensor delay = ((currentRateValue - quotaRate)/quotaRate) * numWindows * windowSize = ((12.5 - 10)/10) * 21 windows * 1 second window = 2.5/10 * 21 = 21/4 = 5.25 second delay
>
> I think the only discrepancy is in the "elapsedTime". The last window is not complete but should be very similar because we configure many small samples. The rate calculation is done inside Rate.java which does not expose the exact elapsed time and the actual counter value.
> Let's examine how the rate changes because of this: The currentRate returned by the sample will still be 12.5. However, Sensor.java uses 21 as the value because we have 21 windows configured. If we got the exact elapsed time, we would use 20 elapsed seconds
>
> Potential Sensor delay = ((currentRateValue - quotaRate)/quotaRate) * elapsedTime = ((12.5 - 10)/10) * 20 = 2.5/10 * 20 = 20/4 = 5 second delay
>
> The values are now exactly identical. Given that 5.25 second delay is slightly more pessimistic, we basically throttle a bit more aggressively if the last window is not complete. We can improve this in 2 ways:
> - Expose elapsedTime and raw metric value from Stat.java
> - A better solution (IMO), is to throw QuotaViolationException from Rate.java itself. We can also add the delayTime within the Rate class since it does not make sense for other metrics.
>
> Finally, We should consider refactoring Throttler.scala to use the Rate metric for its quota computation since the formulae will be identical and only performed in 1 place. I can tackle all of this pretty quickly in a followup patch if that is acceptable.
Got it. I think your calculation is correct. Perhaps we can add a comment on how the formula is derived. However, it does seem that rate includes the last partial window in calculating the rate (see Rate.measure()). So, in your formula, elapsedTime should probably be convert(now - stat.oldest(now).lastWindowMs) where now is passed in from record().
- Jun
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/#review94333
-----------------------------------------------------------
On Aug. 10, 2015, 8:49 p.m., Aditya Auradkar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33049/
> -----------------------------------------------------------
>
> (Updated Aug. 10, 2015, 8:49 p.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/ )
>
> Addressed comments from Joel and Jun
>
>
> 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 7ea509c2c41acc00430c74e025e069a833aac4e7
> 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/KafkaConfigTest.scala f32d206d3f52f3f9f4d649c213edd7058f4b6150
> core/src/test/scala/unit/kafka/server/ThrottledResponseExpirationTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/33049/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Aditya Auradkar
>
>
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Aditya Auradkar <aa...@linkedin.com>.
> On Aug. 6, 2015, 2:02 a.m., Jun Rao wrote:
> > clients/src/main/java/org/apache/kafka/common/metrics/Sensor.java, lines 135-139
> > <https://reviews.apache.org/r/33049/diff/18/?file=1032143#file1032143line135>
> >
> > Is that calculation here right? Based on the calculation in Throttler, it should be sth like the following.
> >
> > metricValue/quota.bound() - windowSize
> >
> > Also, the window size is calculated as config.sampels() * config.timeWindows(), which is inaccurate. The last window is not complete. So we need to take the current time into consideration.
> >
> > Finally, it seems that delayTime only makes sense for rates and not for anything else. Perhaps we can at least add a comment.
>
> Aditya Auradkar wrote:
> Hey Jun -
>
> Can you elaborate a little? How would we use the current time exactly? It is not clear to me how subtracting the windowSize (time unit) from a fraction (metricValue/quota.bound()) gives the right delay.
>
> I also added a comment for delayTime making sense for rates only.
I dug into the Throttler code a bit. Basically - The metricValue is the absolute actual value and not a rate.
Throttler delay: (currentValueOfCounter/quotaRate) - elapsedTime
Current Sensor delay: ((currentRateValue - quotaRate)/quotaRate) * elapsedTime
Lets take an example (all in seconds):
quotaRate = 10QPS
elapedSec = 20 (Lets say 21 windows of 1 second each. The last second has not started yet)
currentValueOfCounter = 250
currentRate = (250/20) = 12.5 (assuming 20 elapsed seconds. Current second may be incomplete)
Throttler Formula delay = (currentValueOfCounter/quotaRate) - elapsedTime = (250/10) - 20 = 5 second delay
Current Sensor delay = ((currentRateValue - quotaRate)/quotaRate) * numWindows * windowSize = ((12.5 - 10)/10) * 21 windows * 1 second window = 2.5/10 * 21 = 21/4 = 5.25 second delay
I think the only discrepancy is in the "elapsedTime". The last window is not complete but should be very similar because we configure many small samples. The rate calculation is done inside Rate.java which does not expose the exact elapsed time and the actual counter value.
Let's examine how the rate changes because of this: The currentRate returned by the sample will still be 12.5. However, Sensor.java uses 21 as the value because we have 21 windows configured. If we got the exact elapsed time, we would use 20 elapsed seconds
Potential Sensor delay = ((currentRateValue - quotaRate)/quotaRate) * elapsedTime = ((12.5 - 10)/10) * 20 = 2.5/10 * 20 = 20/4 = 5 second delay
The values are now exactly identical. Given that 5.25 second delay is slightly more pessimistic, we basically throttle a bit more aggressively if the last window is not complete. We can improve this in 2 ways:
- Expose elapsedTime and raw metric value from Stat.java
- A better solution (IMO), is to throw QuotaViolationException from Rate.java itself. We can also add the delayTime within the Rate class since it does not make sense for other metrics.
Finally, We should consider refactoring Throttler.scala to use the Rate metric for its quota computation since the formulae will be identical and only performed in 1 place. I can tackle all of this pretty quickly in a followup patch if that is acceptable.
- Aditya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/#review94333
-----------------------------------------------------------
On Aug. 7, 2015, 6:28 p.m., Aditya Auradkar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33049/
> -----------------------------------------------------------
>
> (Updated Aug. 7, 2015, 6:28 p.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/ )
>
> Addressed comments from Joel and Jun
>
>
> 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
>
>
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Aditya Auradkar <aa...@linkedin.com>.
> On Aug. 6, 2015, 2:02 a.m., Jun Rao wrote:
> > clients/src/main/java/org/apache/kafka/common/metrics/Sensor.java, lines 135-139
> > <https://reviews.apache.org/r/33049/diff/18/?file=1032143#file1032143line135>
> >
> > Is that calculation here right? Based on the calculation in Throttler, it should be sth like the following.
> >
> > metricValue/quota.bound() - windowSize
> >
> > Also, the window size is calculated as config.sampels() * config.timeWindows(), which is inaccurate. The last window is not complete. So we need to take the current time into consideration.
> >
> > Finally, it seems that delayTime only makes sense for rates and not for anything else. Perhaps we can at least add a comment.
>
> Aditya Auradkar wrote:
> Hey Jun -
>
> Can you elaborate a little? How would we use the current time exactly? It is not clear to me how subtracting the windowSize (time unit) from a fraction (metricValue/quota.bound()) gives the right delay.
>
> I also added a comment for delayTime making sense for rates only.
>
> Aditya Auradkar wrote:
> I dug into the Throttler code a bit. Basically - The metricValue is the absolute actual value and not a rate.
> Throttler delay: (currentValueOfCounter/quotaRate) - elapsedTime
> Current Sensor delay: ((currentRateValue - quotaRate)/quotaRate) * elapsedTime
>
>
> Lets take an example (all in seconds):
> quotaRate = 10QPS
> elapedSec = 20 (Lets say 21 windows of 1 second each. The last second has not started yet)
> currentValueOfCounter = 250
> currentRate = (250/20) = 12.5 (assuming 20 elapsed seconds. Current second may be incomplete)
>
> Throttler Formula delay = (currentValueOfCounter/quotaRate) - elapsedTime = (250/10) - 20 = 5 second delay
> Current Sensor delay = ((currentRateValue - quotaRate)/quotaRate) * numWindows * windowSize = ((12.5 - 10)/10) * 21 windows * 1 second window = 2.5/10 * 21 = 21/4 = 5.25 second delay
>
> I think the only discrepancy is in the "elapsedTime". The last window is not complete but should be very similar because we configure many small samples. The rate calculation is done inside Rate.java which does not expose the exact elapsed time and the actual counter value.
> Let's examine how the rate changes because of this: The currentRate returned by the sample will still be 12.5. However, Sensor.java uses 21 as the value because we have 21 windows configured. If we got the exact elapsed time, we would use 20 elapsed seconds
>
> Potential Sensor delay = ((currentRateValue - quotaRate)/quotaRate) * elapsedTime = ((12.5 - 10)/10) * 20 = 2.5/10 * 20 = 20/4 = 5 second delay
>
> The values are now exactly identical. Given that 5.25 second delay is slightly more pessimistic, we basically throttle a bit more aggressively if the last window is not complete. We can improve this in 2 ways:
> - Expose elapsedTime and raw metric value from Stat.java
> - A better solution (IMO), is to throw QuotaViolationException from Rate.java itself. We can also add the delayTime within the Rate class since it does not make sense for other metrics.
>
> Finally, We should consider refactoring Throttler.scala to use the Rate metric for its quota computation since the formulae will be identical and only performed in 1 place. I can tackle all of this pretty quickly in a followup patch if that is acceptable.
>
> Jun Rao wrote:
> Got it. I think your calculation is correct. Perhaps we can add a comment on how the formula is derived. However, it does seem that rate includes the last partial window in calculating the rate (see Rate.measure()). So, in your formula, elapsedTime should probably be convert(now - stat.oldest(now).lastWindowMs) where now is passed in from record().
Thanks Jun. I'll add a comment.
You are right about the elapsedTime using the last partial window. I used 20 seconds in my example for simplicity.
- Aditya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/#review94333
-----------------------------------------------------------
On Aug. 10, 2015, 8:49 p.m., Aditya Auradkar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33049/
> -----------------------------------------------------------
>
> (Updated Aug. 10, 2015, 8:49 p.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/ )
>
> Addressed comments from Joel and Jun
>
>
> 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 7ea509c2c41acc00430c74e025e069a833aac4e7
> 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/KafkaConfigTest.scala f32d206d3f52f3f9f4d649c213edd7058f4b6150
> core/src/test/scala/unit/kafka/server/ThrottledResponseExpirationTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/33049/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Aditya Auradkar
>
>
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/#review94333
-----------------------------------------------------------
Thanks for the patch. A few comments below.
clients/src/main/java/org/apache/kafka/common/metrics/Sensor.java (lines 135 - 139)
<https://reviews.apache.org/r/33049/#comment148902>
Is that calculation here right? Based on the calculation in Throttler, it should be sth like the following.
metricValue/quota.bound() - windowSize
Also, the window size is calculated as config.sampels() * config.timeWindows(), which is inaccurate. The last window is not complete. So we need to take the current time into consideration.
Finally, it seems that delayTime only makes sense for rates and not for anything else. Perhaps we can at least add a comment.
core/src/main/scala/kafka/server/ClientQuotaManager.scala (line 131)
<https://reviews.apache.org/r/33049/#comment148907>
Do we need to log the exception? We know exactly what it is.
core/src/main/scala/kafka/server/ClientQuotaManager.scala (lines 208 - 211)
<https://reviews.apache.org/r/33049/#comment148909>
Instead of creating a new metric instance, we probably should just reuse the same metric instance created in KafkaServer. This will make reporting easier.
core/src/main/scala/kafka/server/KafkaApis.scala (line 293)
<https://reviews.apache.org/r/33049/#comment148906>
We can just do quotaManagers(RequestKeys.ProduceKey) since we expect the entry to always exist in the map.
core/src/main/scala/kafka/server/KafkaConfig.scala (line 418)
<https://reviews.apache.org/r/33049/#comment148908>
What's the use case of the delay factor?
- Jun Rao
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
>
>
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Aditya Auradkar <aa...@linkedin.com>.
-----------------------------------------------------------
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)
-------
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
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Aditya Auradkar <aa...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/
-----------------------------------------------------------
(Updated Aug. 5, 2015, 2:07 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)
-------
Signed-off-by: Aditya Auradkar <aa...@linkedin.com>
Addressing Joel's comments
Minor imports changes
Added testcase to verify that replication traffic is not throttled
Tmp commit
Fixing test failure
Minor
Addressing Joel's comments
Addressing comments
Addressing comments
Diffs (updated)
-----
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
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Aditya Auradkar <aa...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/
-----------------------------------------------------------
(Updated Aug. 5, 2015, 1:51 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)
-------
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
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Aditya Auradkar <aa...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/
-----------------------------------------------------------
(Updated Aug. 5, 2015, 1:50 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)
-------
Signed-off-by: Aditya Auradkar <aa...@linkedin.com>
Addressing Joel's comments
Minor imports changes
Added testcase to verify that replication traffic is not throttled
Tmp commit
Fixing test failure
Minor
Addressing Joel's comments
Addressing comments
Diffs (updated)
-----
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
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Aditya Auradkar <aa...@linkedin.com>.
> On Aug. 4, 2015, 3:28 a.m., Edward Ribeiro wrote:
> > clients/src/main/java/org/apache/kafka/common/metrics/QuotaViolationException.java, line 27
> > <https://reviews.apache.org/r/33049/diff/16/?file=995401#file995401line27>
> >
> > It's a very good pratice to make any field ``final`` unless necessary otherwise. Make this field final.
There was a setter we no longer need. changed
- Aditya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/#review93991
-----------------------------------------------------------
On June 30, 2015, 12:54 a.m., Aditya Auradkar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33049/
> -----------------------------------------------------------
>
> (Updated June 30, 2015, 12:54 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/ClientQuotaMetrics.scala PRE-CREATION
> core/src/main/scala/kafka/server/KafkaApis.scala d63bc18d795a6f0e6994538ca55a9a46f7fb8ffd
> core/src/main/scala/kafka/server/KafkaConfig.scala 2d75186a110075e0c322db4b9f7a8c964a7a3e88
> core/src/main/scala/kafka/server/KafkaServer.scala b320ce9f6a12c0ee392e91beb82e8804d167f9f4
> core/src/main/scala/kafka/server/ReplicaManager.scala 59c9bc3ac3a8afc07a6f8c88c5871304db588d17
> core/src/main/scala/kafka/server/ThrottledRequest.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/ClientQuotaMetricsTest.scala PRE-CREATION
> core/src/test/scala/unit/kafka/server/KafkaConfigConfigDefTest.scala ace6321b36d809946554d205bc926c9c76a43bd6
> core/src/test/scala/unit/kafka/server/ThrottledRequestExpirationTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/33049/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Aditya Auradkar
>
>
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Edward Ribeiro <ed...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/#review93991
-----------------------------------------------------------
clients/src/main/java/org/apache/kafka/common/metrics/Quota.java (line 65)
<https://reviews.apache.org/r/33049/#comment148477>
It's considered a best practice in Java to use ``instanceof`` instead of ``getClass()`` as explained here http://stackoverflow.com/a/596507/2698109
So, you could rewrite lines 65 to 74 as:
``
if (!(obj instanceof Quota))
return false;
Quota that = (Quota) obj;
return (that.bound == this.bound) && (this.upper == this.upper);
``
If you decide to keep the ``getClass()`` if-condition as is today, lines 70 to 74 can be simplified as above.
clients/src/main/java/org/apache/kafka/common/metrics/QuotaViolationException.java (line 27)
<https://reviews.apache.org/r/33049/#comment148478>
It's a very good pratice to make any field ``final`` unless necessary otherwise. Make this field final.
clients/src/main/java/org/apache/kafka/common/metrics/Sensor.java (line 138)
<https://reviews.apache.org/r/33049/#comment148488>
If time is, for example, 3.8 this will return 3. Wouldn't be better to round it to 4 using something akin ``(int) Math.round(time)``?
core/src/main/scala/kafka/server/ClientQuotaMetrics.scala (line 99)
<https://reviews.apache.org/r/33049/#comment148484>
separate ``if`` and ``(`` with a space.
core/src/main/scala/kafka/server/ClientQuotaMetrics.scala (line 174)
<https://reviews.apache.org/r/33049/#comment148482>
space between ``if`` and ``(``
core/src/main/scala/kafka/server/ClientQuotaMetrics.scala (line 184)
<https://reviews.apache.org/r/33049/#comment148483>
space between ``if`` and ``(``
core/src/main/scala/kafka/server/KafkaServer.scala (line 362)
<https://reviews.apache.org/r/33049/#comment148489>
insert a space between ``if`` and ``(``.
core/src/test/scala/unit/kafka/server/ClientQuotaMetricsTest.scala (line 68)
<https://reviews.apache.org/r/33049/#comment148487>
separate ``for`` and ``(`` with a space.
core/src/test/scala/unit/kafka/server/ThrottledRequestExpirationTest.scala (line 58)
<https://reviews.apache.org/r/33049/#comment148486>
separate ``for`` and ``(`` with a space.
core/src/test/scala/unit/kafka/server/ThrottledRequestExpirationTest.scala (line 83)
<https://reviews.apache.org/r/33049/#comment148485>
separate ``for`` and ``(`` with a space.
- Edward Ribeiro
On June 30, 2015, 12:54 a.m., Aditya Auradkar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33049/
> -----------------------------------------------------------
>
> (Updated June 30, 2015, 12:54 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/ClientQuotaMetrics.scala PRE-CREATION
> core/src/main/scala/kafka/server/KafkaApis.scala d63bc18d795a6f0e6994538ca55a9a46f7fb8ffd
> core/src/main/scala/kafka/server/KafkaConfig.scala 2d75186a110075e0c322db4b9f7a8c964a7a3e88
> core/src/main/scala/kafka/server/KafkaServer.scala b320ce9f6a12c0ee392e91beb82e8804d167f9f4
> core/src/main/scala/kafka/server/ReplicaManager.scala 59c9bc3ac3a8afc07a6f8c88c5871304db588d17
> core/src/main/scala/kafka/server/ThrottledRequest.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/ClientQuotaMetricsTest.scala PRE-CREATION
> core/src/test/scala/unit/kafka/server/KafkaConfigConfigDefTest.scala ace6321b36d809946554d205bc926c9c76a43bd6
> core/src/test/scala/unit/kafka/server/ThrottledRequestExpirationTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/33049/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Aditya Auradkar
>
>
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Aditya Auradkar <aa...@linkedin.com>.
> On Aug. 4, 2015, 12:36 a.m., Joel Koshy wrote:
> > core/src/main/scala/kafka/server/ClientQuotaMetrics.scala, line 98
> > <https://reviews.apache.org/r/33049/diff/16/?file=995406#file995406line98>
> >
> > A number of places in this patch use a : b instead of a: b. Highly stylistic, I'm beginning to not care about it, but thought we generally prefer a: b
I thought I had fixed them earlier.. I think I finally fixed them now.
- Aditya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/#review93965
-----------------------------------------------------------
On June 30, 2015, 12:54 a.m., Aditya Auradkar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33049/
> -----------------------------------------------------------
>
> (Updated June 30, 2015, 12:54 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/ClientQuotaMetrics.scala PRE-CREATION
> core/src/main/scala/kafka/server/KafkaApis.scala d63bc18d795a6f0e6994538ca55a9a46f7fb8ffd
> core/src/main/scala/kafka/server/KafkaConfig.scala 2d75186a110075e0c322db4b9f7a8c964a7a3e88
> core/src/main/scala/kafka/server/KafkaServer.scala b320ce9f6a12c0ee392e91beb82e8804d167f9f4
> core/src/main/scala/kafka/server/ReplicaManager.scala 59c9bc3ac3a8afc07a6f8c88c5871304db588d17
> core/src/main/scala/kafka/server/ThrottledRequest.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/ClientQuotaMetricsTest.scala PRE-CREATION
> core/src/test/scala/unit/kafka/server/KafkaConfigConfigDefTest.scala ace6321b36d809946554d205bc926c9c76a43bd6
> core/src/test/scala/unit/kafka/server/ThrottledRequestExpirationTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/33049/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Aditya Auradkar
>
>
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/#review93965
-----------------------------------------------------------
Looks good overall. I have a few more minor comments/suggestions. Apart from that the patch needs a rebase.
core/src/main/scala/kafka/server/ClientQuotaMetrics.scala (line 74)
<https://reviews.apache.org/r/33049/#comment148446>
Would ClientQuotaManager be a better name?
core/src/main/scala/kafka/server/ClientQuotaMetrics.scala (line 98)
<https://reviews.apache.org/r/33049/#comment148447>
A number of places in this patch use a : b instead of a: b. Highly stylistic, I'm beginning to not care about it, but thought we generally prefer a: b
core/src/main/scala/kafka/server/KafkaApis.scala (line 643)
<https://reviews.apache.org/r/33049/#comment148417>
Rather than string -> quota manager should we just do short (for requestkey) -> quota manager and avoid additional `nameForKey` lookups further above?
core/src/main/scala/kafka/server/KafkaApis.scala (line 652)
<https://reviews.apache.org/r/33049/#comment148418>
```
quotaManagers.foreach { case(apiKey, quotaManager) =>
quotaManager.shutdown()
}
```
core/src/main/scala/kafka/server/ThrottledRequest.scala (line 31)
<https://reviews.apache.org/r/33049/#comment148422>
Would ThrottledResponse be a more accurate name for this?
core/src/test/scala/integration/kafka/api/QuotasTest.scala (line 133)
<https://reviews.apache.org/r/33049/#comment148423>
Can we look up the sensor directly?
E.g., `...metrics().getSensor(RequestKeys.nameForKey(RequestKeys.FetchKey) + "ThrottleTime-" + consumerId1)`
core/src/test/scala/integration/kafka/api/QuotasTest.scala (line 140)
<https://reviews.apache.org/r/33049/#comment148425>
and with the above this would just be `assertNotNull`
core/src/test/scala/integration/kafka/api/QuotasTest.scala (line 148)
<https://reviews.apache.org/r/33049/#comment148424>
Uncomment?
core/src/test/scala/integration/kafka/api/QuotasTest.scala (line 166)
<https://reviews.apache.org/r/33049/#comment148426>
Similar comments as above.
- Joel Koshy
On June 30, 2015, 12:54 a.m., Aditya Auradkar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33049/
> -----------------------------------------------------------
>
> (Updated June 30, 2015, 12:54 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/ClientQuotaMetrics.scala PRE-CREATION
> core/src/main/scala/kafka/server/KafkaApis.scala d63bc18d795a6f0e6994538ca55a9a46f7fb8ffd
> core/src/main/scala/kafka/server/KafkaConfig.scala 2d75186a110075e0c322db4b9f7a8c964a7a3e88
> core/src/main/scala/kafka/server/KafkaServer.scala b320ce9f6a12c0ee392e91beb82e8804d167f9f4
> core/src/main/scala/kafka/server/ReplicaManager.scala 59c9bc3ac3a8afc07a6f8c88c5871304db588d17
> core/src/main/scala/kafka/server/ThrottledRequest.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/ClientQuotaMetricsTest.scala PRE-CREATION
> core/src/test/scala/unit/kafka/server/KafkaConfigConfigDefTest.scala ace6321b36d809946554d205bc926c9c76a43bd6
> core/src/test/scala/unit/kafka/server/ThrottledRequestExpirationTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/33049/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Aditya Auradkar
>
>
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Aditya Auradkar <aa...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/
-----------------------------------------------------------
(Updated June 30, 2015, 12:54 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)
-------
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/ClientQuotaMetrics.scala PRE-CREATION
core/src/main/scala/kafka/server/KafkaApis.scala d63bc18d795a6f0e6994538ca55a9a46f7fb8ffd
core/src/main/scala/kafka/server/KafkaConfig.scala 2d75186a110075e0c322db4b9f7a8c964a7a3e88
core/src/main/scala/kafka/server/KafkaServer.scala b320ce9f6a12c0ee392e91beb82e8804d167f9f4
core/src/main/scala/kafka/server/ReplicaManager.scala 59c9bc3ac3a8afc07a6f8c88c5871304db588d17
core/src/main/scala/kafka/server/ThrottledRequest.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/ClientQuotaMetricsTest.scala PRE-CREATION
core/src/test/scala/unit/kafka/server/KafkaConfigConfigDefTest.scala ace6321b36d809946554d205bc926c9c76a43bd6
core/src/test/scala/unit/kafka/server/ThrottledRequestExpirationTest.scala PRE-CREATION
Diff: https://reviews.apache.org/r/33049/diff/
Testing
-------
Thanks,
Aditya Auradkar
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Aditya Auradkar <aa...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/
-----------------------------------------------------------
(Updated June 30, 2015, 12:53 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)
-------
Signed-off-by: Aditya Auradkar <aa...@linkedin.com>
Addressing Joel's comments
Minor imports changes
Added testcase to verify that replication traffic is not throttled
Tmp commit
Fixing test failure
Minor
Addressing Joel's comments
Diffs (updated)
-----
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/ClientQuotaMetrics.scala PRE-CREATION
core/src/main/scala/kafka/server/KafkaApis.scala d63bc18d795a6f0e6994538ca55a9a46f7fb8ffd
core/src/main/scala/kafka/server/KafkaConfig.scala 2d75186a110075e0c322db4b9f7a8c964a7a3e88
core/src/main/scala/kafka/server/KafkaServer.scala b320ce9f6a12c0ee392e91beb82e8804d167f9f4
core/src/main/scala/kafka/server/ReplicaManager.scala 59c9bc3ac3a8afc07a6f8c88c5871304db588d17
core/src/main/scala/kafka/server/ThrottledRequest.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/ClientQuotaMetricsTest.scala PRE-CREATION
core/src/test/scala/unit/kafka/server/KafkaConfigConfigDefTest.scala ace6321b36d809946554d205bc926c9c76a43bd6
core/src/test/scala/unit/kafka/server/ThrottledRequestExpirationTest.scala PRE-CREATION
Diff: https://reviews.apache.org/r/33049/diff/
Testing
-------
Thanks,
Aditya Auradkar
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Aditya Auradkar <aa...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/
-----------------------------------------------------------
(Updated June 12, 2015, 5:40 p.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)
-------
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/ClientQuotaMetrics.scala PRE-CREATION
core/src/main/scala/kafka/server/KafkaApis.scala d63bc18d795a6f0e6994538ca55a9a46f7fb8ffd
core/src/main/scala/kafka/server/KafkaConfig.scala 2d75186a110075e0c322db4b9f7a8c964a7a3e88
core/src/main/scala/kafka/server/KafkaServer.scala b320ce9f6a12c0ee392e91beb82e8804d167f9f4
core/src/main/scala/kafka/server/ReplicaManager.scala 59c9bc3ac3a8afc07a6f8c88c5871304db588d17
core/src/main/scala/kafka/server/ThrottledRequest.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/ClientQuotaMetricsTest.scala PRE-CREATION
core/src/test/scala/unit/kafka/server/KafkaConfigConfigDefTest.scala ace6321b36d809946554d205bc926c9c76a43bd6
core/src/test/scala/unit/kafka/server/ThrottledRequestExpirationTest.scala PRE-CREATION
Diff: https://reviews.apache.org/r/33049/diff/
Testing
-------
Thanks,
Aditya Auradkar
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Aditya Auradkar <aa...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/
-----------------------------------------------------------
(Updated June 12, 2015, 5:39 p.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)
-------
Signed-off-by: Aditya Auradkar <aa...@linkedin.com>
Addressing Joel's comments
Minor imports changes
Added testcase to verify that replication traffic is not throttled
Tmp commit
Fixing test failure
Minor
Diffs (updated)
-----
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/ClientQuotaMetrics.scala PRE-CREATION
core/src/main/scala/kafka/server/KafkaApis.scala d63bc18d795a6f0e6994538ca55a9a46f7fb8ffd
core/src/main/scala/kafka/server/KafkaConfig.scala 2d75186a110075e0c322db4b9f7a8c964a7a3e88
core/src/main/scala/kafka/server/KafkaServer.scala b320ce9f6a12c0ee392e91beb82e8804d167f9f4
core/src/main/scala/kafka/server/ReplicaManager.scala 59c9bc3ac3a8afc07a6f8c88c5871304db588d17
core/src/main/scala/kafka/server/ThrottledRequest.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/ClientQuotaMetricsTest.scala PRE-CREATION
core/src/test/scala/unit/kafka/server/KafkaConfigConfigDefTest.scala ace6321b36d809946554d205bc926c9c76a43bd6
core/src/test/scala/unit/kafka/server/ThrottledRequestExpirationTest.scala PRE-CREATION
Diff: https://reviews.apache.org/r/33049/diff/
Testing
-------
Thanks,
Aditya Auradkar
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Aditya Auradkar <aa...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/
-----------------------------------------------------------
(Updated June 4, 2015, 11:32 p.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)
-------
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/ClientQuotaMetrics.scala PRE-CREATION
core/src/main/scala/kafka/server/KafkaApis.scala 387e387998fc3a6c9cb585dab02b5f77b0381fbf
core/src/main/scala/kafka/server/KafkaConfig.scala 9efa15ca5567b295ab412ee9eea7c03eb4cdc18b
core/src/main/scala/kafka/server/KafkaServer.scala e66710d2368334ece66f70d55f57b3f888262620
core/src/main/scala/kafka/server/ReplicaManager.scala 59c9bc3ac3a8afc07a6f8c88c5871304db588d17
core/src/main/scala/kafka/server/ThrottledRequest.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/ClientQuotaMetricsTest.scala PRE-CREATION
core/src/test/scala/unit/kafka/server/KafkaConfigConfigDefTest.scala 8014a5a6c362785539f24eb03d77278434614fe6
core/src/test/scala/unit/kafka/server/ThrottledRequestExpirationTest.scala PRE-CREATION
Diff: https://reviews.apache.org/r/33049/diff/
Testing
-------
Thanks,
Aditya Auradkar
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Aditya Auradkar <aa...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/
-----------------------------------------------------------
(Updated June 4, 2015, 11:31 p.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)
-------
Signed-off-by: Aditya Auradkar <aa...@linkedin.com>
Addressing Joel's comments
Diffs (updated)
-----
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/ClientQuotaMetrics.scala PRE-CREATION
core/src/main/scala/kafka/server/KafkaApis.scala 387e387998fc3a6c9cb585dab02b5f77b0381fbf
core/src/main/scala/kafka/server/KafkaConfig.scala 9efa15ca5567b295ab412ee9eea7c03eb4cdc18b
core/src/main/scala/kafka/server/KafkaServer.scala e66710d2368334ece66f70d55f57b3f888262620
core/src/main/scala/kafka/server/ReplicaManager.scala 59c9bc3ac3a8afc07a6f8c88c5871304db588d17
core/src/main/scala/kafka/server/ThrottledRequest.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/ClientQuotaMetricsTest.scala PRE-CREATION
core/src/test/scala/unit/kafka/server/KafkaConfigConfigDefTest.scala 8014a5a6c362785539f24eb03d77278434614fe6
core/src/test/scala/unit/kafka/server/ThrottledRequestExpirationTest.scala PRE-CREATION
Diff: https://reviews.apache.org/r/33049/diff/
Testing
-------
Thanks,
Aditya Auradkar
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Aditya Auradkar <aa...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/
-----------------------------------------------------------
(Updated June 3, 2015, 12:16 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)
-------
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.
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/ )
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/ClientQuotaMetrics.scala PRE-CREATION
core/src/main/scala/kafka/server/KafkaApis.scala 387e387998fc3a6c9cb585dab02b5f77b0381fbf
core/src/main/scala/kafka/server/KafkaConfig.scala 9efa15ca5567b295ab412ee9eea7c03eb4cdc18b
core/src/main/scala/kafka/server/KafkaServer.scala e66710d2368334ece66f70d55f57b3f888262620
core/src/main/scala/kafka/server/ReplicaManager.scala 59c9bc3ac3a8afc07a6f8c88c5871304db588d17
core/src/main/scala/kafka/server/ThrottledRequest.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/ClientQuotaMetricsTest.scala PRE-CREATION
core/src/test/scala/unit/kafka/server/KafkaConfigConfigDefTest.scala 8014a5a6c362785539f24eb03d77278434614fe6
core/src/test/scala/unit/kafka/server/ThrottledRequestExpirationTest.scala PRE-CREATION
Diff: https://reviews.apache.org/r/33049/diff/
Testing
-------
Thanks,
Aditya Auradkar
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Aditya Auradkar <aa...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/
-----------------------------------------------------------
(Updated June 3, 2015, 12:10 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)
-------
This is currently not being used anywhere in the code because I haven't yet figured out how to enforce delays i.e. purgatory vs delay queue. I'll have a better idea once I look at the new purgatory implementation. Hopefully, this smaller patch is easier to review.
Added more testcases
Some locking changes for reading/creating the sensors
WIP patch
Sample usage in ReplicaManager
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. I've not yet added integration testcases testing the consumer delays.. will update the patch once those are ready
Incorporated Jun's comments
Adding javadoc
KAFKA-2084 - Moved the callbacks to ClientQuotaMetrics
Adding more configs
Don't quota replica traffic
KAFKA-2084
Fixing test failures
Minor fix
Diffs (updated)
-----
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/ClientQuotaMetrics.scala PRE-CREATION
core/src/main/scala/kafka/server/KafkaApis.scala 387e387998fc3a6c9cb585dab02b5f77b0381fbf
core/src/main/scala/kafka/server/KafkaConfig.scala 9efa15ca5567b295ab412ee9eea7c03eb4cdc18b
core/src/main/scala/kafka/server/KafkaServer.scala e66710d2368334ece66f70d55f57b3f888262620
core/src/main/scala/kafka/server/ReplicaManager.scala 59c9bc3ac3a8afc07a6f8c88c5871304db588d17
core/src/main/scala/kafka/server/ThrottledRequest.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/ClientQuotaMetricsTest.scala PRE-CREATION
core/src/test/scala/unit/kafka/server/KafkaConfigConfigDefTest.scala 8014a5a6c362785539f24eb03d77278434614fe6
core/src/test/scala/unit/kafka/server/ThrottledRequestExpirationTest.scala PRE-CREATION
Diff: https://reviews.apache.org/r/33049/diff/
Testing
-------
Thanks,
Aditya Auradkar
Re: Review Request 33049: Patch for KAFKA-2084
Posted by Aditya Auradkar <aa...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/
-----------------------------------------------------------
(Updated June 3, 2015, 12:09 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)
-------
This is currently not being used anywhere in the code because I haven't yet figured out how to enforce delays i.e. purgatory vs delay queue. I'll have a better idea once I look at the new purgatory implementation. Hopefully, this smaller patch is easier to review.
Added more testcases
Some locking changes for reading/creating the sensors
WIP patch
Sample usage in ReplicaManager
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. I've not yet added integration testcases testing the consumer delays.. will update the patch once those are ready
Incorporated Jun's comments
Adding javadoc
KAFKA-2084 - Moved the callbacks to ClientQuotaMetrics
Adding more configs
Don't quota replica traffic
KAFKA-2084
Fixing test failures
Diffs (updated)
-----
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/ClientQuotaMetrics.scala PRE-CREATION
core/src/main/scala/kafka/server/KafkaApis.scala 387e387998fc3a6c9cb585dab02b5f77b0381fbf
core/src/main/scala/kafka/server/KafkaConfig.scala 9efa15ca5567b295ab412ee9eea7c03eb4cdc18b
core/src/main/scala/kafka/server/KafkaServer.scala e66710d2368334ece66f70d55f57b3f888262620
core/src/main/scala/kafka/server/ReplicaManager.scala 59c9bc3ac3a8afc07a6f8c88c5871304db588d17
core/src/main/scala/kafka/server/ThrottledRequest.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/ClientQuotaMetricsTest.scala PRE-CREATION
core/src/test/scala/unit/kafka/server/KafkaConfigConfigDefTest.scala 8014a5a6c362785539f24eb03d77278434614fe6
core/src/test/scala/unit/kafka/server/ThrottledRequestExpirationTest.scala PRE-CREATION
Diff: https://reviews.apache.org/r/33049/diff/
Testing
-------
Thanks,
Aditya Auradkar