NihalJain commented on PR #5323: URL: https://github.com/apache/hbase/pull/5323#issuecomment-1680396632
> After checking the code, I think we should change the implementation for DefaultMetricsSystemInitializer? > > It should be called when we create HMaster or HRegionServer, not in BaseSourceImpl? > > Have you read the commit history about why we just add a call in the Constructor of BaseSourceImpl? It looks strange to initialize JvmMetrics with a metrics name instead of the actual process name. > > Thanks. Hi @Apache9 thanks for your input. I had put up an RCA at https://issues.apache.org/jira/browse/HBASE-27966?focusedCommentId=17741087&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17741087. One of the proposed solution was to do following (let's call this approach 1) : > Explicitly initialize JvmMetrics when Master/RS starts up as one of the first step. But based on discussion went ahead with a simpler fix, i.e. approach 2, which was as follow: > Use lazy initialization to delay static field initialization so that HFile.checkHFileVersion(this.conf); does not invoke the metric io initialization and hence I did not try to see the details of how to fix using approach 1. Could we work on approach 1 with another ticket after taking this as a stop-gap? Or do you want me to switch to approach 1 and revert the code changes done here? Please let me know how we should move ahead. I will work on following review comment: > Could use RestrictedApi to restrict the place where we can call this method. It will cause a compilation error if called from other places in the error prone check. based on what we decide here -- 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]
