apoorvmittal10 commented on code in PR #20152:
URL: https://github.com/apache/kafka/pull/20152#discussion_r2510787053
##########
clients/src/main/java/org/apache/kafka/common/metrics/MetricConfig.java:
##########
@@ -43,6 +43,19 @@ public MetricConfig() {
this.recordingLevel = Sensor.RecordingLevel.INFO;
}
+ /**
+ * Copy constructor.
+ * @param config the {@link MetricConfig} instance to copy.
+ */
+ public MetricConfig(MetricConfig config) {
+ this.quota = config.quota == null ? null : new
Quota(config.quota.bound(), config.quota.isUpperBound());
Review Comment:
Should there be a copy of Quota created? May be not.
##########
clients/src/main/java/org/apache/kafka/common/metrics/stats/Rate.java:
##########
@@ -58,11 +64,16 @@ public String unitName() {
@Override
public void record(MetricConfig config, double value, long timeMs) {
+ if (window != -1)
+ config = new MetricConfig(config).timeWindow(window, unit);
Review Comment:
Not appropriate to create a MetricConfig on every `record` execution. Rather
we can keep the `MetricConfig` as instance variable.
##########
clients/src/main/java/org/apache/kafka/common/metrics/stats/Rate.java:
##########
@@ -34,6 +34,7 @@ public class Rate implements MeasurableStat {
protected final TimeUnit unit;
protected final SampledStat stat;
+ protected final long window;
Review Comment:
// The metric config which holds custom time window configuration.
```suggestion
protected final MetricConfig overriddenMetricConfig;
```
##########
clients/src/main/java/org/apache/kafka/common/metrics/stats/Rate.java:
##########
@@ -34,6 +34,7 @@ public class Rate implements MeasurableStat {
protected final TimeUnit unit;
protected final SampledStat stat;
+ protected final long window;
Review Comment:
```suggestion
protected final long timeWindowMs;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
