bbeaudreault commented on PR #5072: URL: https://github.com/apache/hbase/pull/5072#issuecomment-1453679966
@thangTang your analysis makes a lot of sense, and I 100% agree with the direction. I agree the existing setup is a bit confusing, and MetricsRegionServer is more of a tool class. I agree it would be nice to either rename it or refactor it, and that can happen in a separate issue. For this PR, I think it would be nice to move the logic back into MetricsRegionServer for now. For two reasons: 1. Reduces the diff here 2. Keeps RSRpcServices a bit cleaner... It's already a huge and messy class, so would rather not move more logic into it if not necessary. Passing `Region` into MetricRegionServer might seem a bit weird, but all of the options are weird. This way at least is a minimal diff. I think we could add a comment or todo in the code to let developers know that we hope to clean up the abstraction. I have another PR https://github.com/apache/hbase/pull/5067 which is going to add more metrics to Table and RegionServer beans. I think that PR would also end up quite a bit messier if we moved the logic into RSRpcServices, so I think that's a good reason (ease of adding new metrics) -- 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]
