mcvsubbu commented on a change in pull request #4392: Allow customized metrics
prefix in pinot controller/broker/server
URL: https://github.com/apache/incubator-pinot/pull/4392#discussion_r304125347
##########
File path:
pinot-common/src/main/java/org/apache/pinot/common/utils/CommonConstants.java
##########
@@ -293,6 +294,7 @@ public ServerType getServerType() {
public static final String METADATA_EVENT_OBSERVER_PREFIX =
"metadata.event.notifier";
// Config keys
+ public static final String METRICS_PREFIX_CONFIG = "metrics.prefix.config";
Review comment:
Can you rename this to "metricsPrefix" instead of "metrics.prefix.config"?
It is unfortunate that minion went on a different route for configuring
metricsRegistryRegistrationListener (basically, minion picks up metrics related
configs without any prefix string for the configuration keys of metrics related
items. The other components have "pinot.controller.metrics.foo",
"pinot.broker.metrics.foo", etc.). minion chooses to pick the listeners from
the top-level config. Ideally, I would prefer deprecating the old config for
minion (with empty prefix) in favor of the new one (with a prefix
"pinot.,minion.metrics", like the others) for one release and then removing the
old one altogether. Of course, this will mean changing minion to parse the new
configs if they are there, and then falling back to the old one, and then
applying any defaults.
@Jackie-Jiang can you chime in as to why minion took this route? Any
thoughts?
----------------------------------------------------------------
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:
[email protected]
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
