Mailing-List: contact dev-help@hbase.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@hbase.apache.org
MIME-Version: 1.0
In-Reply-To: <CADcMMgEu7B_cPpKx6AZLX96YTdvkoEeJsU657hmj9Uvbpe1e3A@mail.gmail.com>
References: <CADcMMgEu7B_cPpKx6AZLX96YTdvkoEeJsU657hmj9Uvbpe1e3A@mail.gmail.com>
From: Andrew Purtell <apurtell@apache.org>
Date: Wed, 4 Oct 2017 15:51:18 -0700
Message-ID: <CA+RK=_Bjzp6bfDMFbBnCCVK8Q_seM824noKSOcemhugu5CEnqA@mail.gmail.com>
Subject: Re: Looking for input on an alpha-4 thorny item
To: "dev@hbase.apache.org" <dev@hbase.apache.org>
Content-Type: multipart/alternative; boundary="089e082c5538887e2c055ac074b2"
archived-at: Wed, 04 Oct 2017 22:52:03 -0000

--089e082c5538887e2c055ac074b2
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

I think it is fine to rebrand these interfaces as for coprocessors and tag
them LP(COPROC):

    Region (use HRegion in internals)

    Store (use HStore in internals)

    MasterServices (use HMaster in internals)

    RegionServerServices (use HRegionServer in internals)

and pare them down. This is not a complete list, just a handful of examples=
.

Some specific points of feedback I have had:

In Region, it would be good for CPs to be able to schedule async flushes
and compactions, poll or wait for completion of a specific request, or wait
for all pending flushes and compactions to complete. There is a Phoenix use
case for this in indexing.

Security coprocessors use MasterServices to create their system tables.
Maybe this can be replaced by using the normal admin API for same.

When removing access to internal services, consider if there are client API
equivalents that the CP can use, and if embedded calls to such client APIs
from the coprocessor context would be a good idea. CP invocation of an
internal service is simply an in-process method call. That's good and bad,
right? The bad part, direct access, is the thing we want to restrict, the
motivation for this work (in part). But the good thing is it avoids a lot
of the fat client logic unnecessary for all-in-process service invocation,
which might even not work correctly. Removing everything is as drastic as
allowing CPs access to everything. It could be fine to drastically pare
down, but please consider it.

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.
=E2=80=8B=E2=80=8B

On Wed, Oct 4, 2017 at 3:15 PM, Stack <stack@duboce.net> wrote:
=E2=80=8B=E2=80=8B

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 Coprocesso=
r
> API", mostly undoing access accidentally granted Coprocessors. I'm talkin=
g
> 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 HMast=
er
> instance (This usually required our putting up a cluster too). If
> MasterServices had but a few methods, a Mock would be easy, making testin=
g
> 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 neede=
d
> 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 le=
ss
> facility available (for parts, there are alternatives; Andy review sugges=
ts
> 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 Du=
o
> and Anoop in the RegionServer Coprocessor refactor salt mine. They hit a
> similar tangle. There was the RegionServerServices <=3D> MasterServices c=
ase
> 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 gaine=
d
> 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 t=
he
> 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? Dit=
to
> on MasterServices?
>
> Thanks,
> S
>


--=20
Best regards,
Andrew

Words like orphans lost among the crosstalk, meaning torn from truth's
decrepit hands
   - A23, Crosstalk

--089e082c5538887e2c055ac074b2--