[GitHub] [incubator-pinot] sajjad-moradi commented on a change in pull request #6037: Add list of allowed tables for emitting table level metrics

[GitBox <gi...@apache.org>]

sajjad-moradi commented on a change in pull request #6037:
URL: https://github.com/apache/incubator-pinot/pull/6037#discussion_r493019744



##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/CommonConstants.java
##########
@@ -279,7 +281,10 @@
     }
 
     public static final String DEFAULT_METRICS_PREFIX = "pinot.server.";
-    public static final boolean DEFAULT_METRICS_GLOBAL_ENABLED = false;
+    public static final String CONFIG_OF_ENABLE_TABLE_LEVEL_METRICS = "pinot.server.enableTableLevelMetrics";

Review comment:
       Other than keeping similar config in one place, there's also this todo in server conf: `TODO: Replace with constants in CommonConstants`.
   Having said that the main reason to move to CommonConstant is that in the existing code, the default value for enabling table level metrics is defined in two places: ServerConf and CommonConstants. ServerConf lives in pinot-broker module and CommonConstant in pinot-common. Both are needed as ServerConf is used in ServerInstance defined in pinot-broker module and CommonConstant is used in ServerMetrics which is defined in pinot-common module.
   Having it only defined in CommonConstant removes this duplicity. 




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org