> I agree that what we publish as metrics out over our jmx interface is public. What is interesting though is that all Metrics implementation classes are IA.private. And what we push out to operators is read-only.
Yep the problem as I see it is those metrics that are published via Hadoop metrics or scraped from JMX go to production monitoring systems and are consumed to produce reports, alerts, predictive analytics on performance trends, etc. If a coprocessor bypasses core code that updates a metric and replaces that core functionality, then if the metrics are not updated by the coprocessor, the accumulated counts in those external systems are no longer representative of what is happening in the server. On the other hand I see your point on metrics interface annotations. I think it is one reason out of many why bypass seems not a good idea. The effects are going to be very version dependent and unstable, and considerations like whether or not metrics should be updated expect the implementer to study what they are replacing in detail and understand all the ramifications, which given the Phoenix example is hit or miss. On Mon, Oct 9, 2017 at 11:24 AM, Stack <st...@duboce.net> wrote: > On Fri, Oct 6, 2017 at 9:10 AM, Stack <st...@duboce.net> wrote: > > > .... > > > > > >> Some changes have been proposed that removes access to metrics (e.g. > >> RegionMetrics, MasterMetrics). Right now coprocessors can bypass core > >> function and replace it. Until and unless we remove the bypass semantic > >> (under discussion) we should continue to allow CPs access to metrics > >> objects so they can update metrics as expected by admins and users when > >> replacing functionality (via bypass). Metrics are a public facing API. I > >> agree this is kind of dodgy. I believe we should remove the bypass > >> semantic. Once that is done, coprocessors can only mix in additional > >> functionality. No more cause to touch core metrics. They can export > their > >> own metrics if so desired. > >> > >> > > > > Where is the by-pass conversation happening? On quick review, I am unable > to find it. I'm interested given the above. > > Above you say metrics are 'public'. I agree that what we publish as metrics > out over our jmx interface is public. What is interesting though is that > all Metrics implementation classes are IA.private. And what we push out to > operators is read-only. > > So, yeah, interested in by-pass discussion. > > Thanks, > St.Ack > > > > > > > > > > >> On Wed, Oct 4, 2017 at 3:15 PM, Stack <st...@duboce.net> wrote: > >> > >> > >> A bunch of us are making good progress on the next alpha release, > >> > hbase-2.0.0-alpha-4. The theme for this release is "Fixing the > >> Coprocessor > >> > API", mostly undoing access accidentally granted Coprocessors. I'm > >> talking > >> > out loud about a particularly awkward item here rather than in a > >> comment up > >> > in JIRA so the airing sees a broader audience. Interested in any > >> opinions > >> > or input you might have. > >> > > >> > TL;DR MasterService/RegionServerService and Region, etc., Interfaces > >> were > >> > overloaded serving two, disparate roles; a load of refactoring has to > be > >> > done to undo the damage. Suggestions for how to avoid making same > >> mistake > >> > in future? > >> > > >> > I'm working on "HBASE-12260 MasterServices - remove from coprocessor > API > >> > (Discuss)". MasterServices started out as a subset of the Master > >> > functionality. The idea back then was that certain Services and > Managers > >> > could make do w/ less-than-full-access to the HMaster process. If so, > we > >> > could test the Service and Manager without having to standup a full > >> HMaster > >> > instance (This usually required our putting up a cluster too). If > >> > MasterServices had but a few methods, a Mock would be easy, making > >> testing > >> > easier still. We did the same thing on the RegionServer side where we > >> had > >> > RegionServerServices. > >> > > >> > MasterServices (and RegionServerServices) were also exposed to > >> > Coprocessors. The idea was that CPs could ask-for particular Master > >> > functions via MasterService. In this role MasterServices was meant to > >> > constrain what CPs had access to. > >> > > >> > MasterServices therefore had two consumers; one internal, the other > not. > >> > > >> > With time, MasterServices got fat as internal Services and Managers > >> needed > >> > more of HMaster. Everytime we added to MasterServices, CPs got access. > >> > > >> > On survey as part of the recent HBASE-12260 work, it turns out that > the > >> > bulk of the methods in MasterServices are actually annotated > >> > InterfaceAudience.Private; i.e. for internal use only, not for > >> > Coprocessors. A brutal purge of Private audience objects, makes for a > >> > MasterServices we can pass Coprocessors but Coprocessors now have much > >> less > >> > facility available (for parts, there are alternatives; Andy review > >> suggests > >> > that CPs are severely crimped if this patch goes in). But > MasterServices > >> > can no longer be used for its original purpose, passing Services and > >> > Managers a subset of HMaster. The latter brings on a substantial > >> refactor. > >> > > >> > Another example of the double-role problem outlined above was found by > >> Duo > >> > and Anoop in the RegionServer Coprocessor refactor salt mine. They > hit a > >> > similar tangle. There was the RegionServerServices <=> MasterServices > >> case > >> > but also the exposure of HRegion internals. In this latter, Region was > >> > introduced by Andy EXPLICITLY as a subset of HRegion facility FOR > >> > Coprocessors. Subsequently, we all confused his original intent and > went > >> > ahead and thought of Region (as opposed to HRegion) as an Interface > for > >> > HRegion and plumbed it throughout the code base in place of explicit > >> > HRegion references. As Region picked up functionality, Coprocessors > >> gained > >> > more access. > >> > > >> > The refactoring pattern that has emerged out of the RegionServer-side > >> > refactoring (which is ahead of the Master-work), is that we move to > use > >> the > >> > HRegion implementation everywhere internally, undoing use of Region > >> > Interface; Region Interface picks up a "FOR COPROCESSORS ONLY" stamp. > >> I'm > >> > following suit on the Master side moving to use HMaster in place of > >> > MasterServices in all launched Services and Managers. > >> > > >> > How do we avoid this mistake in future? Should we rename Region as > >> > CoprocessorRegion so it more plain that its consumer is Coprocessors? > >> Ditto > >> > on MasterServices? > >> > > >> > Thanks, > >> > S > >> > > >> > >> > >> > >> -- > >> Best regards, > >> Andrew > >> > >> Words like orphans lost among the crosstalk, meaning torn from truth's > >> decrepit hands > >> - A23, Crosstalk > >> > > > > > -- Best regards, Andrew Words like orphans lost among the crosstalk, meaning torn from truth's decrepit hands - A23, Crosstalk
