Re: [DISCUSS] Change the IA for MutableSizeHistogram and MutableTimeHistogram to LImitedPrivate
+1 to "let's do both" FWIW, our stance on semver is to "strive to follow it", not "must follow it". I think when we have a concrete change we can look at, we could make the call to say whether or not we feel comfortable bringing this into maintenance release lines. The circumstances of this request certainly feel "sufficient" to me to push a new facade into a bugfix release (because it would increase later stability by getting Phoenix off private API). On 6/11/20 3:07 PM, Andrew Purtell wrote: That's unfortunate, but needs must, IMHO. A potential benefit of also marking the impls LP(COPROC) is this captures any implicit dependency on semantics and functionality of the implementation classes not directly exposed in the hbase-metrics-api facade. So, let's do both? (Facade improvement, raise to LP the impl classes) On Thu, Jun 11, 2020 at 12:00 PM Geoffrey Jacoby wrote: Couple points: 1. I like Andrew's proposed solution, and we should do it, but I'm not sure it's sufficient for Rushabh's purposes because of semver rules. Phoenix supports HBase 1.3 -1.5 (soon to add 1.6) and HBase 2.0 (soon to gain 2.1 and 2.2, with 2.3 coming shortly after its release here.) If we add the new sizeHistogram and timeHistogram methods to hbase-metrics, they'll be available in Phoenix only in HBase 1.7 and 2.4. (since 2.3 is mostly-frozen) Since Phoenix will be supporting earlier versions of both HBase branches for a good while, there will need to be a compatibility shim. And the older-version instance of the shim will probably need to access the classes directly. (Please correct me if I'm wrong, Rushabh or Andrew.) So it still might need a LimitedPrivate IA. 2. I agree with Nick that it's better to use LimitedPrivate.COPROC rather than LimitedPrivate.PHOENIX. Geoffrey On Thu, Jun 11, 2020 at 11:28 AM Josh Elser wrote: Sounds reasonable to me! On 6/11/20 1:06 PM, Andrew Purtell wrote: hbase-metrics-api is available for coprocessors already and interfaces within are already LimitedPrivate(COPROC). However, that package is mostly interface and seems geared toward consuming metrics instantiated and registered via private stuff. Or, rather, I didn't see how Phoenix could choose which of MutableSizeHistogram and MutableTimeHistogram to instantiate using those interfaces, there is only Histogram MetricRegistry#histogram(String name). So I think it is also worth some time to review the utility of hbase-metrics-api and decide if more need be done there. Would the addition of Histogram MetricRegistry#sizeHistogram(String name) Histogram MetricRegistry#timeHistogram(String name) achieve the desired objective instead? On Thu, Jun 11, 2020 at 9:16 AM Nick Dimiduk wrote: I was just about to reply with the same -- Josh is faster :) +1 on considering the full surface area of the APIs being exposed. I also wonder if exposing the metrics infrastructure is something of interest more broadly than Phoenix. Seems like any coprocessor might want to provide or monitor some metric value. On Thu, Jun 11, 2020 at 9:08 AM Josh Elser wrote: My only concern is that you can't just mark these two classes a LimitedPrivate for Phoenix -- you would also have to mark MutableRangeHistogram, MutableHistogram (and the rest of the class hierarchy) to make sure that we don't make it super confusing as to what comes from LimitedPrivate classes and what is coming from Private classes. Would it be better to just say: make ./hbase-hadoop2-compat/src/main/java/org/apache/hadoop/metrics2/lib LimitedPrivate? Do you also need the stuff in hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase to push metrics back through the HBase metrics subsystem? Sorry for the late reply. Just want to make sure we open up the audience, we open it sufficiently. On 6/8/20 1:15 PM, Rushabh Shah wrote: Hi, Currently the IA for MutableSizeHistogram and MutableTimeHistogram is private. We want to use these classes in PHOENIX project and I thought we can leverage the existing implementation from hbase histo implementation. IIUC the private IA can't be used in other projects. Proposing to make it LimitedPrivate and mark HBaseInterfaceAudience.PHOENIX. Please suggest. Related jira: https://issues.apache.org/jira/browse/HBASE-24520
Re: [DISCUSS] Change the IA for MutableSizeHistogram and MutableTimeHistogram to LImitedPrivate
That's unfortunate, but needs must, IMHO. A potential benefit of also marking the impls LP(COPROC) is this captures any implicit dependency on semantics and functionality of the implementation classes not directly exposed in the hbase-metrics-api facade. So, let's do both? (Facade improvement, raise to LP the impl classes) On Thu, Jun 11, 2020 at 12:00 PM Geoffrey Jacoby wrote: > Couple points: > > 1. I like Andrew's proposed solution, and we should do it, but I'm not sure > it's sufficient for Rushabh's purposes because of semver rules. Phoenix > supports HBase 1.3 -1.5 (soon to add 1.6) and HBase 2.0 (soon to gain 2.1 > and 2.2, with 2.3 coming shortly after its release here.) If we add the new > sizeHistogram and timeHistogram methods to hbase-metrics, they'll be > available in Phoenix only in HBase 1.7 and 2.4. (since 2.3 is > mostly-frozen) > > Since Phoenix will be supporting earlier versions of both HBase branches > for a good while, there will need to be a compatibility shim. And the > older-version instance of the shim will probably need to access the classes > directly. (Please correct me if I'm wrong, Rushabh or Andrew.) So it still > might need a LimitedPrivate IA. > > 2. I agree with Nick that it's better to use LimitedPrivate.COPROC rather > than LimitedPrivate.PHOENIX. > > Geoffrey > > > > On Thu, Jun 11, 2020 at 11:28 AM Josh Elser wrote: > > > Sounds reasonable to me! > > > > On 6/11/20 1:06 PM, Andrew Purtell wrote: > > > hbase-metrics-api is available for coprocessors already and interfaces > > > within are already LimitedPrivate(COPROC). However, that package is > > mostly > > > interface and seems geared toward consuming metrics instantiated and > > > registered via private stuff. Or, rather, I didn't see how Phoenix > could > > choose > > > which of MutableSizeHistogram and MutableTimeHistogram to instantiate > > using > > > those interfaces, there is only Histogram > MetricRegistry#histogram(String > > > name). So I think it is also worth some time to review the utility of > > > hbase-metrics-api and decide if more need be done there. Would the > > addition > > > of > > > > > > Histogram MetricRegistry#sizeHistogram(String name) > > > Histogram MetricRegistry#timeHistogram(String name) > > > > > > achieve the desired objective instead? > > > > > > > > > On Thu, Jun 11, 2020 at 9:16 AM Nick Dimiduk > > wrote: > > > > > >> I was just about to reply with the same -- Josh is faster :) +1 on > > >> considering the full surface area of the APIs being exposed. > > >> > > >> I also wonder if exposing the metrics infrastructure is something of > > >> interest more broadly than Phoenix. Seems like any coprocessor might > > want > > >> to provide or monitor some metric value. > > >> > > >> On Thu, Jun 11, 2020 at 9:08 AM Josh Elser wrote: > > >> > > >>> My only concern is that you can't just mark these two classes a > > >>> LimitedPrivate for Phoenix -- you would also have to mark > > >>> MutableRangeHistogram, MutableHistogram (and the rest of the class > > >>> hierarchy) to make sure that we don't make it super confusing as to > > what > > >>> comes from LimitedPrivate classes and what is coming from Private > > >> classes. > > >>> > > >>> Would it be better to just say: make > > >>> ./hbase-hadoop2-compat/src/main/java/org/apache/hadoop/metrics2/lib > > >>> LimitedPrivate? > > >>> > > >>> Do you also need the stuff in > > >>> hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase to push > > >>> metrics back through the HBase metrics subsystem? > > >>> > > >>> Sorry for the late reply. Just want to make sure we open up the > > >>> audience, we open it sufficiently. > > >>> > > >>> On 6/8/20 1:15 PM, Rushabh Shah wrote: > > Hi, > > Currently the IA for MutableSizeHistogram and MutableTimeHistogram > is > > private. We want to use these classes in PHOENIX project and I > thought > > >> we > > can leverage the existing implementation from hbase histo > > >> implementation. > > IIUC the private IA can't be used in other projects. Proposing to > make > > >> it > > LimitedPrivate and mark HBaseInterfaceAudience.PHOENIX. Please > > suggest. > > Related jira: https://issues.apache.org/jira/browse/HBASE-24520 > > > > >>> > > >> > > > > > > > > > -- Best regards, Andrew Words like orphans lost among the crosstalk, meaning torn from truth's decrepit hands - A23, Crosstalk
Re: [DISCUSS] Change the IA for MutableSizeHistogram and MutableTimeHistogram to LImitedPrivate
Couple points: 1. I like Andrew's proposed solution, and we should do it, but I'm not sure it's sufficient for Rushabh's purposes because of semver rules. Phoenix supports HBase 1.3 -1.5 (soon to add 1.6) and HBase 2.0 (soon to gain 2.1 and 2.2, with 2.3 coming shortly after its release here.) If we add the new sizeHistogram and timeHistogram methods to hbase-metrics, they'll be available in Phoenix only in HBase 1.7 and 2.4. (since 2.3 is mostly-frozen) Since Phoenix will be supporting earlier versions of both HBase branches for a good while, there will need to be a compatibility shim. And the older-version instance of the shim will probably need to access the classes directly. (Please correct me if I'm wrong, Rushabh or Andrew.) So it still might need a LimitedPrivate IA. 2. I agree with Nick that it's better to use LimitedPrivate.COPROC rather than LimitedPrivate.PHOENIX. Geoffrey On Thu, Jun 11, 2020 at 11:28 AM Josh Elser wrote: > Sounds reasonable to me! > > On 6/11/20 1:06 PM, Andrew Purtell wrote: > > hbase-metrics-api is available for coprocessors already and interfaces > > within are already LimitedPrivate(COPROC). However, that package is > mostly > > interface and seems geared toward consuming metrics instantiated and > > registered via private stuff. Or, rather, I didn't see how Phoenix could > choose > > which of MutableSizeHistogram and MutableTimeHistogram to instantiate > using > > those interfaces, there is only Histogram MetricRegistry#histogram(String > > name). So I think it is also worth some time to review the utility of > > hbase-metrics-api and decide if more need be done there. Would the > addition > > of > > > > Histogram MetricRegistry#sizeHistogram(String name) > > Histogram MetricRegistry#timeHistogram(String name) > > > > achieve the desired objective instead? > > > > > > On Thu, Jun 11, 2020 at 9:16 AM Nick Dimiduk > wrote: > > > >> I was just about to reply with the same -- Josh is faster :) +1 on > >> considering the full surface area of the APIs being exposed. > >> > >> I also wonder if exposing the metrics infrastructure is something of > >> interest more broadly than Phoenix. Seems like any coprocessor might > want > >> to provide or monitor some metric value. > >> > >> On Thu, Jun 11, 2020 at 9:08 AM Josh Elser wrote: > >> > >>> My only concern is that you can't just mark these two classes a > >>> LimitedPrivate for Phoenix -- you would also have to mark > >>> MutableRangeHistogram, MutableHistogram (and the rest of the class > >>> hierarchy) to make sure that we don't make it super confusing as to > what > >>> comes from LimitedPrivate classes and what is coming from Private > >> classes. > >>> > >>> Would it be better to just say: make > >>> ./hbase-hadoop2-compat/src/main/java/org/apache/hadoop/metrics2/lib > >>> LimitedPrivate? > >>> > >>> Do you also need the stuff in > >>> hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase to push > >>> metrics back through the HBase metrics subsystem? > >>> > >>> Sorry for the late reply. Just want to make sure we open up the > >>> audience, we open it sufficiently. > >>> > >>> On 6/8/20 1:15 PM, Rushabh Shah wrote: > Hi, > Currently the IA for MutableSizeHistogram and MutableTimeHistogram is > private. We want to use these classes in PHOENIX project and I thought > >> we > can leverage the existing implementation from hbase histo > >> implementation. > IIUC the private IA can't be used in other projects. Proposing to make > >> it > LimitedPrivate and mark HBaseInterfaceAudience.PHOENIX. Please > suggest. > Related jira: https://issues.apache.org/jira/browse/HBASE-24520 > > >>> > >> > > > > >
Re: [DISCUSS] Change the IA for MutableSizeHistogram and MutableTimeHistogram to LImitedPrivate
Sounds reasonable to me! On 6/11/20 1:06 PM, Andrew Purtell wrote: hbase-metrics-api is available for coprocessors already and interfaces within are already LimitedPrivate(COPROC). However, that package is mostly interface and seems geared toward consuming metrics instantiated and registered via private stuff. Or, rather, I didn't see how Phoenix could choose which of MutableSizeHistogram and MutableTimeHistogram to instantiate using those interfaces, there is only Histogram MetricRegistry#histogram(String name). So I think it is also worth some time to review the utility of hbase-metrics-api and decide if more need be done there. Would the addition of Histogram MetricRegistry#sizeHistogram(String name) Histogram MetricRegistry#timeHistogram(String name) achieve the desired objective instead? On Thu, Jun 11, 2020 at 9:16 AM Nick Dimiduk wrote: I was just about to reply with the same -- Josh is faster :) +1 on considering the full surface area of the APIs being exposed. I also wonder if exposing the metrics infrastructure is something of interest more broadly than Phoenix. Seems like any coprocessor might want to provide or monitor some metric value. On Thu, Jun 11, 2020 at 9:08 AM Josh Elser wrote: My only concern is that you can't just mark these two classes a LimitedPrivate for Phoenix -- you would also have to mark MutableRangeHistogram, MutableHistogram (and the rest of the class hierarchy) to make sure that we don't make it super confusing as to what comes from LimitedPrivate classes and what is coming from Private classes. Would it be better to just say: make ./hbase-hadoop2-compat/src/main/java/org/apache/hadoop/metrics2/lib LimitedPrivate? Do you also need the stuff in hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase to push metrics back through the HBase metrics subsystem? Sorry for the late reply. Just want to make sure we open up the audience, we open it sufficiently. On 6/8/20 1:15 PM, Rushabh Shah wrote: Hi, Currently the IA for MutableSizeHistogram and MutableTimeHistogram is private. We want to use these classes in PHOENIX project and I thought we can leverage the existing implementation from hbase histo implementation. IIUC the private IA can't be used in other projects. Proposing to make it LimitedPrivate and mark HBaseInterfaceAudience.PHOENIX. Please suggest. Related jira: https://issues.apache.org/jira/browse/HBASE-24520
Re: [DISCUSS] Change the IA for MutableSizeHistogram and MutableTimeHistogram to LImitedPrivate
hbase-metrics-api is available for coprocessors already and interfaces within are already LimitedPrivate(COPROC). However, that package is mostly interface and seems geared toward consuming metrics instantiated and registered via private stuff. Or, rather, I didn't see how Phoenix could choose which of MutableSizeHistogram and MutableTimeHistogram to instantiate using those interfaces, there is only Histogram MetricRegistry#histogram(String name). So I think it is also worth some time to review the utility of hbase-metrics-api and decide if more need be done there. Would the addition of Histogram MetricRegistry#sizeHistogram(String name) Histogram MetricRegistry#timeHistogram(String name) achieve the desired objective instead? On Thu, Jun 11, 2020 at 9:16 AM Nick Dimiduk wrote: > I was just about to reply with the same -- Josh is faster :) +1 on > considering the full surface area of the APIs being exposed. > > I also wonder if exposing the metrics infrastructure is something of > interest more broadly than Phoenix. Seems like any coprocessor might want > to provide or monitor some metric value. > > On Thu, Jun 11, 2020 at 9:08 AM Josh Elser wrote: > > > My only concern is that you can't just mark these two classes a > > LimitedPrivate for Phoenix -- you would also have to mark > > MutableRangeHistogram, MutableHistogram (and the rest of the class > > hierarchy) to make sure that we don't make it super confusing as to what > > comes from LimitedPrivate classes and what is coming from Private > classes. > > > > Would it be better to just say: make > > ./hbase-hadoop2-compat/src/main/java/org/apache/hadoop/metrics2/lib > > LimitedPrivate? > > > > Do you also need the stuff in > > hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase to push > > metrics back through the HBase metrics subsystem? > > > > Sorry for the late reply. Just want to make sure we open up the > > audience, we open it sufficiently. > > > > On 6/8/20 1:15 PM, Rushabh Shah wrote: > > > Hi, > > > Currently the IA for MutableSizeHistogram and MutableTimeHistogram is > > > private. We want to use these classes in PHOENIX project and I thought > we > > > can leverage the existing implementation from hbase histo > implementation. > > > IIUC the private IA can't be used in other projects. Proposing to make > it > > > LimitedPrivate and mark HBaseInterfaceAudience.PHOENIX. Please suggest. > > > Related jira: https://issues.apache.org/jira/browse/HBASE-24520 > > > > > > -- Best regards, Andrew Words like orphans lost among the crosstalk, meaning torn from truth's decrepit hands - A23, Crosstalk
Re: [DISCUSS] Change the IA for MutableSizeHistogram and MutableTimeHistogram to LImitedPrivate
I was just about to reply with the same -- Josh is faster :) +1 on considering the full surface area of the APIs being exposed. I also wonder if exposing the metrics infrastructure is something of interest more broadly than Phoenix. Seems like any coprocessor might want to provide or monitor some metric value. On Thu, Jun 11, 2020 at 9:08 AM Josh Elser wrote: > My only concern is that you can't just mark these two classes a > LimitedPrivate for Phoenix -- you would also have to mark > MutableRangeHistogram, MutableHistogram (and the rest of the class > hierarchy) to make sure that we don't make it super confusing as to what > comes from LimitedPrivate classes and what is coming from Private classes. > > Would it be better to just say: make > ./hbase-hadoop2-compat/src/main/java/org/apache/hadoop/metrics2/lib > LimitedPrivate? > > Do you also need the stuff in > hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase to push > metrics back through the HBase metrics subsystem? > > Sorry for the late reply. Just want to make sure we open up the > audience, we open it sufficiently. > > On 6/8/20 1:15 PM, Rushabh Shah wrote: > > Hi, > > Currently the IA for MutableSizeHistogram and MutableTimeHistogram is > > private. We want to use these classes in PHOENIX project and I thought we > > can leverage the existing implementation from hbase histo implementation. > > IIUC the private IA can't be used in other projects. Proposing to make it > > LimitedPrivate and mark HBaseInterfaceAudience.PHOENIX. Please suggest. > > Related jira: https://issues.apache.org/jira/browse/HBASE-24520 > > >
Re: [DISCUSS] Change the IA for MutableSizeHistogram and MutableTimeHistogram to LImitedPrivate
My only concern is that you can't just mark these two classes a LimitedPrivate for Phoenix -- you would also have to mark MutableRangeHistogram, MutableHistogram (and the rest of the class hierarchy) to make sure that we don't make it super confusing as to what comes from LimitedPrivate classes and what is coming from Private classes. Would it be better to just say: make ./hbase-hadoop2-compat/src/main/java/org/apache/hadoop/metrics2/lib LimitedPrivate? Do you also need the stuff in hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase to push metrics back through the HBase metrics subsystem? Sorry for the late reply. Just want to make sure we open up the audience, we open it sufficiently. On 6/8/20 1:15 PM, Rushabh Shah wrote: Hi, Currently the IA for MutableSizeHistogram and MutableTimeHistogram is private. We want to use these classes in PHOENIX project and I thought we can leverage the existing implementation from hbase histo implementation. IIUC the private IA can't be used in other projects. Proposing to make it LimitedPrivate and mark HBaseInterfaceAudience.PHOENIX. Please suggest. Related jira: https://issues.apache.org/jira/browse/HBASE-24520
[DISCUSS] Change the IA for MutableSizeHistogram and MutableTimeHistogram to LImitedPrivate
Hi, Currently the IA for MutableSizeHistogram and MutableTimeHistogram is private. We want to use these classes in PHOENIX project and I thought we can leverage the existing implementation from hbase histo implementation. IIUC the private IA can't be used in other projects. Proposing to make it LimitedPrivate and mark HBaseInterfaceAudience.PHOENIX. Please suggest. Related jira: https://issues.apache.org/jira/browse/HBASE-24520
