apoorvmittal10 commented on code in PR #17168:
URL: https://github.com/apache/kafka/pull/17168#discussion_r1777740885
##########
server/src/main/java/org/apache/kafka/server/metrics/ClientMetricsConfigs.java:
##########
@@ -133,8 +134,8 @@ private static void validateProperties(Properties
properties) {
// Make sure that push interval is between 100ms and 1 hour.
if (properties.containsKey(PUSH_INTERVAL_MS)) {
- int pushIntervalMs =
Integer.parseInt(properties.getProperty(PUSH_INTERVAL_MS));
- if (pushIntervalMs < MIN_INTERVAL_MS || pushIntervalMs >
MAX_INTERVAL_MS) {
+ Integer pushIntervalMs = (Integer)
ConfigDef.parseType(PUSH_INTERVAL_MS, properties.getProperty(PUSH_INTERVAL_MS),
Type.INT);
Review Comment:
If I get it right, the issue we are trying to solve is regarding the correct
exception when config is not as INT or expected? If that's the case then either
of the 2 approach I think would be better:
1. Make this method non-static, instance method. And remove `properties` as
input. Then instantiate ClientMetricsConfig where they are used and call
validate. The downside of this is that validation also happens in
`ControllerConfigurationValidator` where unnecessary `ClientMetricsConfig` will
be created.
2. Enclose a try/catch in validate method and return what received in
exception but can return config exception where exception translates to
UnknownServer Exception. This is not very clean but a good way to handle.
3. Use `CONFIG.parse(properties)` in validate method to get `Map<?,?>
valueMaps = CONFIG.parse(properties);` and then iterate on map as like we
iterate on properties. That shall solve your problem.
I would have chosen option 3. Also if you detail on the prolem satetment in
PR description or jira then I can help better. But as far as I think, option 3
might solve your issue.
##########
server/src/main/java/org/apache/kafka/server/metrics/ClientMetricsConfigs.java:
##########
@@ -133,8 +134,8 @@ private static void validateProperties(Properties
properties) {
// Make sure that push interval is between 100ms and 1 hour.
if (properties.containsKey(PUSH_INTERVAL_MS)) {
- int pushIntervalMs =
Integer.parseInt(properties.getProperty(PUSH_INTERVAL_MS));
- if (pushIntervalMs < MIN_INTERVAL_MS || pushIntervalMs >
MAX_INTERVAL_MS) {
+ Integer pushIntervalMs = (Integer)
ConfigDef.parseType(PUSH_INTERVAL_MS, properties.getProperty(PUSH_INTERVAL_MS),
Type.INT);
+ if (pushIntervalMs == null || pushIntervalMs < MIN_INTERVAL_MS ||
pushIntervalMs > MAX_INTERVAL_MS) {
Review Comment:
Hmmm, what happens when we use `kafka-client-metrics.sh` tool to just update
pattern or metrics i.e. pushIntervalMs is not updated? Then will pushIntervalMs
is not null?
--
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]
