[DISCUSS] Punt the Coprocessor TableWrapper and CoprocessorHConnection?

[Stack <st...@duboce.net>]

tl;dr I believe the original intent for TableWrapper and
CoprocessorHConnection can now be gotten elsewhere so we should purge these
classes.

In base CoprocessorEnvironment, there are methods to return a Table
instance. There are none to return an Admin. In our code base, the only
user is the AccessController. Phoenix uses it twice in its Indexer
implementation.

When you call CE#getTable, the default implementation in
BaseEnvironemnt calls HTableWrapper.createWrapper which takes a
BaseEnvironment List in which we keep all Table instances. On shutdown of
the coprocessor, the list is iterated and all tables are closed.

Going via TableWrapper, the class comment says:

* A wrapper for HTable. Can be used to restrict privilege.
*
* Currently it just helps to track tables opened by a Coprocessor and
* facilitate close of them if it is aborted.
*
* We also disallow row locking.
*
* There is nothing now that will stop a coprocessor from using HTable
* objects directly instead of this API, but in the future we intend to
* analyze coprocessor implementations as they are loaded and reject those
* which attempt to use objects and methods outside the Environment
* sandbox.


TableWrapper by my reading delegates all calls to a Table instance with no
interception (there is not rowlocking to override anymore). On open, we do
ensure the Table is up on a CoprocessorHConnection which does the following:

* Connection to an HTable from within a Coprocessor. We can do some
nice tricks since we know we
* are on a regionserver, for instance skipping the full
serialization/deserialization of objects
* when talking to the server.


The above 'trick' is now commonplace in servers as the Master and
RegionServer always make Connections that will short-circuit if an
opportunity.

As I read TableWrapper and CoprocessorHConnection, they were written at
another time when Table construction was heavyweight and Table#close was
not expected of clients and before the introduction of the general
Server-side short-circuit Connection facility.

Unless objection, I think we should purge them.

Writing here in case I'm missing some key facility they provide.

Thanks,
St.Ack

Re: [DISCUSS] Punt the Coprocessor TableWrapper and CoprocessorHConnection?

[Stack <st...@duboce.net>]

HBASE-19043
S

On Wed, Oct 18, 2017 at 3:29 PM, Andrew Purtell <ap...@apache.org> wrote:

> > I remember Andrew saying same thing about CoprocessorEnvironment#
> getTable
> functions during the review of HBASE-17732, that we should get rid of them
> and let CP do their own table resource management like all other things.
>
> Yes
>
> > The above 'trick' is now commonplace in servers as the Master and
> RegionServer
> always make Connections that will short-circuit if an opportunity
>
> No need for CoprocessorHConnection, then.
>
>
>
> On Wed, Oct 18, 2017 at 3:12 PM, Apekshit Sharma <ap...@cloudera.com>
> wrote:
>
> > +1
> > I remember Andrew saying same thing about CoprocessorEnvironment#
> getTable
> > functions during the review of HBASE-17732, that we should get rid of
> them
> > and let CP do their own table resource management like all other things.
> >
> > On Wed, Oct 18, 2017 at 2:45 PM, Stack <st...@duboce.net> wrote:
> >
> > > tl;dr I believe the original intent for TableWrapper and
> > > CoprocessorHConnection can now be gotten elsewhere so we should purge
> > these
> > > classes.
> > >
> > > In base CoprocessorEnvironment, there are methods to return a Table
> > > instance. There are none to return an Admin. In our code base, the only
> > > user is the AccessController. Phoenix uses it twice in its Indexer
> > > implementation.
> > >
> > > When you call CE#getTable, the default implementation in
> > > BaseEnvironemnt calls HTableWrapper.createWrapper which takes a
> > > BaseEnvironment List in which we keep all Table instances. On shutdown
> of
> > > the coprocessor, the list is iterated and all tables are closed.
> > >
> > > Going via TableWrapper, the class comment says:
> > >
> > > * A wrapper for HTable. Can be used to restrict privilege.
> > > *
> > > * Currently it just helps to track tables opened by a Coprocessor and
> > > * facilitate close of them if it is aborted.
> > > *
> > > * We also disallow row locking.
> > > *
> > > * There is nothing now that will stop a coprocessor from using HTable
> > > * objects directly instead of this API, but in the future we intend to
> > > * analyze coprocessor implementations as they are loaded and reject
> those
> > > * which attempt to use objects and methods outside the Environment
> > > * sandbox.
> > >
> > >
> > > TableWrapper by my reading delegates all calls to a Table instance with
> > no
> > > interception (there is not rowlocking to override anymore). On open, we
> > do
> > > ensure the Table is up on a CoprocessorHConnection which does the
> > > following:
> > >
> > > * Connection to an HTable from within a Coprocessor. We can do some
> > > nice tricks since we know we
> > > * are on a regionserver, for instance skipping the full
> > > serialization/deserialization of objects
> > > * when talking to the server.
> > >
> > >
> > > The above 'trick' is now commonplace in servers as the Master and
> > > RegionServer always make Connections that will short-circuit if an
> > > opportunity.
> > >
> > > As I read TableWrapper and CoprocessorHConnection, they were written at
> > > another time when Table construction was heavyweight and Table#close
> was
> > > not expected of clients and before the introduction of the general
> > > Server-side short-circuit Connection facility.
> > >
> > > Unless objection, I think we should purge them.
> > >
> > > Writing here in case I'm missing some key facility they provide.
> > >
> > > Thanks,
> > > St.Ack
> > >
> >
> >
> >
> > --
> >
> > -- Appy
> >
>
>
>
> --
> Best regards,
> Andrew
>
> Words like orphans lost among the crosstalk, meaning torn from truth's
> decrepit hands
>    - A23, Crosstalk
>

Re: [DISCUSS] Punt the Coprocessor TableWrapper and CoprocessorHConnection?

[Andrew Purtell <ap...@apache.org>]

> I remember Andrew saying same thing about CoprocessorEnvironment#getTable
functions during the review of HBASE-17732, that we should get rid of them
and let CP do their own table resource management like all other things.

Yes

> The above 'trick' is now commonplace in servers as the Master and RegionServer
always make Connections that will short-circuit if an opportunity

No need for CoprocessorHConnection, then.



On Wed, Oct 18, 2017 at 3:12 PM, Apekshit Sharma <ap...@cloudera.com> wrote:

> +1
> I remember Andrew saying same thing about CoprocessorEnvironment#getTable
> functions during the review of HBASE-17732, that we should get rid of them
> and let CP do their own table resource management like all other things.
>
> On Wed, Oct 18, 2017 at 2:45 PM, Stack <st...@duboce.net> wrote:
>
> > tl;dr I believe the original intent for TableWrapper and
> > CoprocessorHConnection can now be gotten elsewhere so we should purge
> these
> > classes.
> >
> > In base CoprocessorEnvironment, there are methods to return a Table
> > instance. There are none to return an Admin. In our code base, the only
> > user is the AccessController. Phoenix uses it twice in its Indexer
> > implementation.
> >
> > When you call CE#getTable, the default implementation in
> > BaseEnvironemnt calls HTableWrapper.createWrapper which takes a
> > BaseEnvironment List in which we keep all Table instances. On shutdown of
> > the coprocessor, the list is iterated and all tables are closed.
> >
> > Going via TableWrapper, the class comment says:
> >
> > * A wrapper for HTable. Can be used to restrict privilege.
> > *
> > * Currently it just helps to track tables opened by a Coprocessor and
> > * facilitate close of them if it is aborted.
> > *
> > * We also disallow row locking.
> > *
> > * There is nothing now that will stop a coprocessor from using HTable
> > * objects directly instead of this API, but in the future we intend to
> > * analyze coprocessor implementations as they are loaded and reject those
> > * which attempt to use objects and methods outside the Environment
> > * sandbox.
> >
> >
> > TableWrapper by my reading delegates all calls to a Table instance with
> no
> > interception (there is not rowlocking to override anymore). On open, we
> do
> > ensure the Table is up on a CoprocessorHConnection which does the
> > following:
> >
> > * Connection to an HTable from within a Coprocessor. We can do some
> > nice tricks since we know we
> > * are on a regionserver, for instance skipping the full
> > serialization/deserialization of objects
> > * when talking to the server.
> >
> >
> > The above 'trick' is now commonplace in servers as the Master and
> > RegionServer always make Connections that will short-circuit if an
> > opportunity.
> >
> > As I read TableWrapper and CoprocessorHConnection, they were written at
> > another time when Table construction was heavyweight and Table#close was
> > not expected of clients and before the introduction of the general
> > Server-side short-circuit Connection facility.
> >
> > Unless objection, I think we should purge them.
> >
> > Writing here in case I'm missing some key facility they provide.
> >
> > Thanks,
> > St.Ack
> >
>
>
>
> --
>
> -- Appy
>



-- 
Best regards,
Andrew

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

Re: [DISCUSS] Punt the Coprocessor TableWrapper and CoprocessorHConnection?

[Apekshit Sharma <ap...@cloudera.com>]

+1
I remember Andrew saying same thing about CoprocessorEnvironment#getTable
functions during the review of HBASE-17732, that we should get rid of them
and let CP do their own table resource management like all other things.

On Wed, Oct 18, 2017 at 2:45 PM, Stack <st...@duboce.net> wrote:

> tl;dr I believe the original intent for TableWrapper and
> CoprocessorHConnection can now be gotten elsewhere so we should purge these
> classes.
>
> In base CoprocessorEnvironment, there are methods to return a Table
> instance. There are none to return an Admin. In our code base, the only
> user is the AccessController. Phoenix uses it twice in its Indexer
> implementation.
>
> When you call CE#getTable, the default implementation in
> BaseEnvironemnt calls HTableWrapper.createWrapper which takes a
> BaseEnvironment List in which we keep all Table instances. On shutdown of
> the coprocessor, the list is iterated and all tables are closed.
>
> Going via TableWrapper, the class comment says:
>
> * A wrapper for HTable. Can be used to restrict privilege.
> *
> * Currently it just helps to track tables opened by a Coprocessor and
> * facilitate close of them if it is aborted.
> *
> * We also disallow row locking.
> *
> * There is nothing now that will stop a coprocessor from using HTable
> * objects directly instead of this API, but in the future we intend to
> * analyze coprocessor implementations as they are loaded and reject those
> * which attempt to use objects and methods outside the Environment
> * sandbox.
>
>
> TableWrapper by my reading delegates all calls to a Table instance with no
> interception (there is not rowlocking to override anymore). On open, we do
> ensure the Table is up on a CoprocessorHConnection which does the
> following:
>
> * Connection to an HTable from within a Coprocessor. We can do some
> nice tricks since we know we
> * are on a regionserver, for instance skipping the full
> serialization/deserialization of objects
> * when talking to the server.
>
>
> The above 'trick' is now commonplace in servers as the Master and
> RegionServer always make Connections that will short-circuit if an
> opportunity.
>
> As I read TableWrapper and CoprocessorHConnection, they were written at
> another time when Table construction was heavyweight and Table#close was
> not expected of clients and before the introduction of the general
> Server-side short-circuit Connection facility.
>
> Unless objection, I think we should purge them.
>
> Writing here in case I'm missing some key facility they provide.
>
> Thanks,
> St.Ack
>



-- 

-- Appy