Apache9 commented on code in PR #4691:
URL: https://github.com/apache/hbase/pull/4691#discussion_r953647493
##########
hbase-http/src/main/java/org/apache/hadoop/hbase/http/HttpServer.java:
##########
@@ -784,6 +787,22 @@ protected void addDefaultServlets(ContextHandlerCollection
contexts, Configurati
LOG.info("ASYNC_PROFILER_HOME environment variable and
async.profiler.home system property "
+ "not specified. Disabling /prof endpoint.");
}
+
+ /* register metrics servlets */
+ String[] enabledServlets = conf.getStrings(METRIC_SERVLETS_CONF_KEY,
METRICS_SERVLETS_DEFAULT);
+ for (String enabledServlet : enabledServlets) {
+ try {
+ ServletConfig servletConfig = METRIC_SERVLETS.get(enabledServlet);
+ if (servletConfig != null) {
+ Class<?> clz = Class.forName(servletConfig.getClazz());
+ addPrivilegedServlet(servletConfig.getName(),
servletConfig.getPathSpec(),
+ clz.asSubclass(HttpServlet.class));
+ }
+ } catch (Exception e) {
+ /* shouldn't be fatal, so warn the user about it */
Review Comment:
For me, I think the current implementation is enough. We will warn if a
registered endpoint fails to load. And the default configuration is to load all
metrics endpoints, if users configured it to none, they must have a reason, so
I do not think here we need to warn it. And I also do not think we should abort
the process if none metrics can be loaded, as it does not effect the normal
read/write.
##########
hbase-http/src/main/java/org/apache/hadoop/hbase/http/HttpServer.java:
##########
@@ -784,6 +787,22 @@ protected void addDefaultServlets(ContextHandlerCollection
contexts, Configurati
LOG.info("ASYNC_PROFILER_HOME environment variable and
async.profiler.home system property "
+ "not specified. Disabling /prof endpoint.");
}
+
+ /* register metrics servlets */
+ String[] enabledServlets = conf.getStrings(METRIC_SERVLETS_CONF_KEY,
METRICS_SERVLETS_DEFAULT);
+ for (String enabledServlet : enabledServlets) {
+ try {
+ ServletConfig servletConfig = METRIC_SERVLETS.get(enabledServlet);
+ if (servletConfig != null) {
+ Class<?> clz = Class.forName(servletConfig.getClazz());
+ addPrivilegedServlet(servletConfig.getName(),
servletConfig.getPathSpec(),
+ clz.asSubclass(HttpServlet.class));
+ }
+ } catch (Exception e) {
+ /* shouldn't be fatal, so warn the user about it */
Review Comment:
For me, I think the current implementation is enough. We will warn if a
registered endpoint fails to load. And the default configuration is to load all
metrics endpoints, if users configured it to none, they must have a reason, so
I do not think here we need to warn it. And I also do not think we should abort
the process if none metrics can be loaded, as it does not affect the normal
read/write.
--
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: issues-unsubscr...@hbase.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
