Re: [DISCUSS] Merge hbase-metrics:oahh.metrics.impl into hbase-metrics-api

[Misty Stanley-Jones]

On Thu, Oct 26, 2017 at 1:52 PM Josh Elser  wrote:

> That's the million-dollar question, now isn't it! :)
>
> Something to hbase-metrics-api/README.md? A chapter in the book?
>

In the Javadoc makes the most sense to me. I don’t think people would check
the book. Apply, where did you try and fail to find the info?


> Happy to try to translate some of this info into something more "permanent"
>
> On 10/26/17 12:59 AM, Misty Stanley-Jones 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  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  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
> 

Re: [DISCUSS] Merge hbase-metrics:oahh.metrics.impl into hbase-metrics-api

[Josh Elser]

On 10/26/17 1:55 PM, Apekshit Sharma wrote:

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?


I think the current separation of default implementation and interfaces 
as separate modules is the ideal solution.



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.


Well, the concern would be failing intuitively if the classpath is 
missing any implementation at all (unlikely, but possible).



- Or move MetricRegistriesImpl in the same module as 'default'
implementation. (until we have multiple implementation)
Ahh, yeah, that's not a poor idea. Took me a moment to realize there was 
both MetricRegistryImpl and MetricRegistriesImpl. I don't yet grok why 
MetricRegistries is important -- I assume the ref-counting being done 
there is important.

Re: [DISCUSS] Merge hbase-metrics:oahh.metrics.impl into hbase-metrics-api

[Josh Elser]

That's the million-dollar question, now isn't it! :)

Something to hbase-metrics-api/README.md? A chapter in the book?

Happy to try to translate some of this info into something more "permanent"

On 10/26/17 12:59 AM, Misty Stanley-Jones 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  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  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

[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

Re: [DISCUSS] Merge hbase-metrics:oahh.metrics.impl into hbase-metrics-api

[Apekshit Sharma]

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 
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  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  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 

Re: [DISCUSS] Merge hbase-metrics:oahh.metrics.impl into hbase-metrics-api

[Misty Stanley-Jones]

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  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  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
> >>>  >>> 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" 

Re: [DISCUSS] Merge hbase-metrics:oahh.metrics.impl into hbase-metrics-api

[Josh Elser]

:) 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  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

[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

Re: [DISCUSS] Merge hbase-metrics:oahh.metrics.impl into hbase-metrics-api

[Elliott Clark]

That issue renamed a bunch of what used to be there only for
hbase-hadoop-compat. HBASE-6405 is the start of the chain.

Currently though I don't see any reason for them to be in separate modules
unless we think hadoop will make another breaking change to the metrics
classes.


On Tue, Oct 24, 2017 at 7:38 PM, Apekshit Sharma  wrote:

> Hey Elliott, good to see you around :)
>
> Btw, this is new stuff added in HBASE-9774 which was committed to only 2.0.
> Made it to 1.4 too later (HBASE-18060).
>
> -- Appy
>
> On Tue, Oct 24, 2017 at 4:16 PM, Elliott Clark  wrote:
>
> > The weird module dance with reflection came from needing to extend inner
> > hadoop classes on multiple different versions. This was back when HBase
> had
> > to support hadoop < 1, hadoop 1.x, and hadoop 2.x. When we dropped older
> > hadoop versions then things got easier.
> >
> > On Tue, Oct 24, 2017 at 3: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
> > >  > > c01bc3aed2/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/eee3b0180ead73c09b33f9583bfee9
> > > c01bc3aed2/hbase-metrics-api/src/main/java/org/apache/
> > > hadoop/hbase/metrics/MetricRegistriesLoader.java#L39
> > >
> > > -- Appy
> > >
> >
>
>
>
> --
>
> -- Appy
>

Re: [DISCUSS] Merge hbase-metrics:oahh.metrics.impl into hbase-metrics-api

[Apekshit Sharma]

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  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
>> > 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

Re: [DISCUSS] Merge hbase-metrics:oahh.metrics.impl into hbase-metrics-api

[Josh Elser]

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

[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/eee3b0180ead73c09b33f9583bfee9c01bc3aed2/hbase-metrics-api/src/main/java/org/apache/hadoop/hbase/metrics/MetricRegistriesLoader.java#L39

-- Appy

Re: [DISCUSS] Merge hbase-metrics:oahh.metrics.impl into hbase-metrics-api

[Apekshit Sharma]

Hey Elliott, good to see you around :)

Btw, this is new stuff added in HBASE-9774 which was committed to only 2.0.
Made it to 1.4 too later (HBASE-18060).

-- Appy

On Tue, Oct 24, 2017 at 4:16 PM, Elliott Clark  wrote:

> The weird module dance with reflection came from needing to extend inner
> hadoop classes on multiple different versions. This was back when HBase had
> to support hadoop < 1, hadoop 1.x, and hadoop 2.x. When we dropped older
> hadoop versions then things got easier.
>
> On Tue, Oct 24, 2017 at 3: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
> >  > c01bc3aed2/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/eee3b0180ead73c09b33f9583bfee9
> > c01bc3aed2/hbase-metrics-api/src/main/java/org/apache/
> > hadoop/hbase/metrics/MetricRegistriesLoader.java#L39
> >
> > -- Appy
> >
>



-- 

-- Appy

Re: [DISCUSS] Merge hbase-metrics:oahh.metrics.impl into hbase-metrics-api

[Elliott Clark]

The weird module dance with reflection came from needing to extend inner
hadoop classes on multiple different versions. This was back when HBase had
to support hadoop < 1, hadoop 1.x, and hadoop 2.x. When we dropped older
hadoop versions then things got easier.

On Tue, Oct 24, 2017 at 3: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
>  c01bc3aed2/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/eee3b0180ead73c09b33f9583bfee9
> c01bc3aed2/hbase-metrics-api/src/main/java/org/apache/
> hadoop/hbase/metrics/MetricRegistriesLoader.java#L39
>
> -- Appy
>