bq. .....integration with metrics-library1 to support the metrics platform for their $business1. $business2 might want to use metrics-library2.... Makes sense Josh.
bq. One final concern, bundling our "default" DropwizardMetrics 3-based implementation could cause headaches for users who want to build their own metrics-api implementation. Right. Do you have something in mind to avoid this? Thanks Josh! So the only remaining thing is, we should either - remove this ( https://github.com/apache/hbase/blob/master/hbase-metrics-api/src/main/java/org/apache/hadoop/hbase/metrics/MetricRegistriesLoader.java#L67) which seems unnecessary because Service Provider will pick up "MetricRegistriesImpl" if it's there. - Or move MetricRegistriesImpl in the same module as 'default' implementation. (until we have multiple implementation). -- Appy On Wed, Oct 25, 2017 at 9:59 PM, Misty Stanley-Jones <mi...@apache.org> wrote: > If we keep it as is, how and where can we capture this knowledge to save > future contributors from having to ask again? > > On Wed, Oct 25, 2017 at 9:22 AM Josh Elser <els...@apache.org> wrote: > > > :) no worries, Appy. These are good questions. > > > > It really comes down to the question: would HBase ever provide multiple > > metrics implementations for users? > > > > As we know metrics in HBase now, there isn't any reason to support > > multiple implementations of metrics. We have one implementation which is > > optimal for people to use. > > > > One plausible use-case I can see being built is a metrics implementation > > that natively uses something _other than_ Hadoop metrics2. Something you > > may not have seen yet is the translation-layer from Dropwizard Metrics > > to Metrics2 via HBaseMetrics2HadoopMetricsAdapter.java. > > > > I could see a world where folks would want to build an integration with > > metrics-library1 to support the metrics platform for their $business1. > > $business2 might want to use metrics-library2 that can automatically > > push to something. As we know all too well, there may be conflicting > > dependencies between metrics-library1 and metrics-library2 to prevent > > them from successfully functioning on the classpath at the same time. > > Additionally, $business1 may have a hard requirement to not deploy > > metrics-library2 (and vice versa). One final concern, bundling our > > "default" DropwizardMetrics 3-based implementation could cause headaches > > for users who want to build their own metrics-api implementation. > > > > Granted, all of the above is hypothetical, but I think leaving this > > flexibility in place is worth the separation of logic. > > > > On 10/25/17 3:35 AM, Apekshit Sharma wrote: > > > Hi Josh > > > > > > You're right that naming doesn't make it clear, but Enis added a really > > > great description in README of hbase-metrics-api to explain what's in > > each > > > module. However it's not obvious why are we splitting things into > > separate > > > module, as opposed to say, separating implementation in a separate > "sub" > > > package. > > > > > > I don't know Avatica, but as you clearly state, one of the requirements > > was > > > being able to plug in different implementations. Using service provider > > > clearly makes sense in that case, so does keeping the implementation > in a > > > separate jar to avoid loading it if not needed. > > > But in case of HBase, even if we have multiple implementations > > > (hypothetically, because it doesn't seem probable), since the only user > > > will be HBase and all modules will always be in classpath, it doesn't > > > really get us anything. Right? > > > I dug into this code first time today and just learnt about service > > > providers, so i may be off, in which case please correct me. Thank you! > > > > > > On Tue, Oct 24, 2017 at 8:25 PM, Josh Elser <els...@apache.org> wrote: > > > > > >> Hey Appy, > > >> > > >> I think Enis essentially copied what I was originally trying to do. Up > > in > > >> Avatica[1], I made a similar API Maven module which just had > interfaces. > > >> The implementation was them split up into a different module to avoid > > the > > >> implementation details of the API (specifically, using Dropwizard3) > from > > >> "infecting" anyone who just wanted the API. Implementations of the > > metrics > > >> API could be picked up via ServiceLoader. > > >> > > >> So, here in HBase, our "hbase-metrics" module is really an > > implementation > > >> of hbase-metrics-api for dropwizard-metrics 3.2.1 (the naming just > loses > > >> that detail). > > >> > > >> In Avatica, I wanted to make sure that people could use a metrics > > library > > >> implementation of their choosing (e.g. tied to whatever kind of > > container > > >> or app-server you're deploying Avatica). In HBase, that isn't such a > > >> concern -- we publish via Metrics2 and that's it. > > >> > > >> That said, I could see a place where this separation enables some > extra > > >> functionality with less headache, but I, admittedly, don't have a > > concrete > > >> use-case behind it. > > >> > > >> - Josh > > >> > > >> [1] https://github.com/apache/calcite-avatica > > >> > > >> On 10/24/17 6:09 PM, Apekshit Sharma wrote: > > >> > > >>> Hi > > >>> > > >>> Am confused why do we have this weird circular dependency between the > > two > > >>> modules: hbase-metrics-api & hbase-metrics. And since maven doesn't > > allow > > >>> it explicitly, this was tucked and hidden by using reflection? > > >>> > > >>> MetricRegistriesImpl[hbase-metrics] implements > > >>> MetricRegistries[hbase-metrics-api]. > > >>> MetricRegistriesLoader[hbase-metrics-api] depends on this > > implementation, > > >>> MetricRegistriesImpl, because it instantiates it via reflection > > >>> <https://github.com/apache/hbase/blob/eee3b0180ead73c09b33f9 > > >>> 583bfee9c01bc3aed2/hbase-metrics-api/src/main/java/org/ > > >>> apache/hadoop/hbase/metrics/MetricRegistriesLoader.java#L39> > > >>> [1]. > > >>> > > >>> Can we just move all interfaces and default implementation together > > into > > >>> single module. But i like the idea of keeping implementations in > > separate > > >>> package with "impl" explicitly in package name. > > >>> > > >>> [1] > > >>> https://github.com/apache/hbase/blob/eee3b0180ead73c09b33f95 > > >>> 83bfee9c01bc3aed2/hbase-metrics-api/src/main/java/org/ > > >>> apache/hadoop/hbase/metrics/MetricRegistriesLoader.java#L39 > > >>> > > >>> -- Appy > > >>> > > >>> > > > > > > > > > -- -- Appy
