Re: Cleanup and remove the code path where is no hbase.client.rpc.codec

[Jerry He <je...@gmail.com>]

On Thu, Dec 21, 2017 at 12:03 AM, Stack <st...@duboce.net> wrote:

> On Tue, Dec 19, 2017 at 7:02 PM, Jerry He <je...@gmail.com> wrote:
>
> > RPC_CODEC_CONF_KEY 'hbase.client.rpc.codec' is a property we use on the
> > client side to determine the RPC codec.
> >
> > It currently has a strange logic. Whereas the default is KeyValueCodec,
> we
> > allow  a user to specify an empty string "" as the a way to indicate
> there
> > is no codec class and we should not use any.
> >
> >   Codec getCodec() {
> >     // For NO CODEC, "hbase.client.rpc.codec" must be configured with
> empty
> > string AND
> >     // "hbase.client.default.rpc.codec" also -- because default is to do
> > cell block encoding.
> >     String className = conf.get(HConstants.RPC_CODEC_CONF_KEY,
> > getDefaultCodec(this.conf));
> >     if (className == null || className.length() == 0) {
> >       return null;
> >     }
> >     try {
> >       return (Codec) Class.forName(className).newInstance();
> >     } catch (Exception e) {
> >       throw new RuntimeException("Failed getting codec " + className, e);
> >     }
> >   }
> >
> > I don't know the original reason for having this.
> >
>
> IIRC, when we moved to pb's first, KeyValues were all pb'd. This was our
> default serialization.
>
> It was too slow -- duh -- so we had to figure something else. We came up w/
> notion of pb being used to describe the content of the RPC but that the
> actual cells would follow-behind the pb in a 'cellblock' (We used to show a
> picture of a motorcycle with a sidecar as an illustration trying to convey
> that the follow-behind appendage was like a 'sidecar' that went with the
> RPC message). We went out of our way to ensure we allowed shipping both
> forms of message -- with sidecar and without with KVs PB encoded. The
> latter would be useful for not native clients, at least while trying to get
> off the ground.


 Thanks for the background information. The intention for non native client
is a good intention.


> The above code came in with:
>
> tree 6c91d2f4ee7faadea35b238418fcd6b5051e37f5
> parent 823656bf8372e55b5b4a81e72921cb78b0be96d7
> author stack <st...@apache.org> Mon Dec 8 15:23:38 2014 -0800
> committer stack <st...@apache.org> Mon Dec 8 15:23:38 2014 -0800
>
> HBASE-12597 Add RpcClient interface and enable changing of RpcClient
> implementation (Jurriaan Mous)
>
> ... where Jurriaan started in on a client-side refactor whose intent was
> being able to slot in an async client.
>
> Before that was HBASE-10322 which added RPC_CODEC_CONF_KEY. Previous to
> this again, IIRC, Anoop I believe noticed that we werent' cellblocking by
> default and fixed it.
>
> We've been cellblocking by default ever since.
>
>
> > The consequence of this 'no codec' is that we will pb all RPC payload and
> > not using cell blocks.
> >
> >
> Exactly.
>
> I used to think it critical we support this mode for the python messers or
> whoever who wanted to put together a quick client; they wouldn't have to
> rig a non-native cellblock decoder. Rare if ever has anyone made use of
> this facility it seems.


Make sense.


>
> > In the test cases, after these many releases, there is no test that
> > excercises this special case.
> > The code path we test are mostly with a valid or default
> > 'hbase.client.rpc.codec'.
> > The other code path is probably sitting there rotten.
> >
> > For example,
> >
> > In MultiServerCallable:
> >
> >           if (this.cellBlock) {
> >             // Build a multi request absent its Cell payload. Send data
> in
> > cellblocks.
> >             regionActionBuilder =
> > RequestConverter.buildNoDataRegionAction(regionName,
> > rms, cells,
> >               regionActionBuilder, actionBuilder, mutationBuilder);
> >           } else {
> >             regionActionBuilder =
> > RequestConverter.buildRegionAction(regionName,
> > rms);   ==> Will not be exercised in test..
> >           }
> >
> > Proposal:
> >
> > We remove this 'no hbase.rpc.codec' case and all dependent logic. There
> is
> > a default and user can overwrite the default, but have to provide a valid
> > non-empty value.
> >
>
> Only objection is that it makes it harder writing non-native client. With
> all pb encoded, you could do a first-cut easy enough w/o having to do
> non-java decoder for our cryptic cellblock packing mechanism.
>
>
Make sense.  The intention is good.  It may be hard for us to maintain
though.
It will be a burden we make for the java native client to carry if we
continue to use it to cover the pure pb case.


> > Then we can clean up the code where we choose between pb or no pb.  We
> will
> > always do cell block in these cases.
> >
> > There are cases where we currently only do pb, like some of the
> individual
> > ops (append, increment, mutateRow, etc). We can revisit to see if they
> can
> > be non-pb'ed.
> >
> > The proposed change only cleans up the client side (PRC client).
> > I want to keep the server side handling of pb and no-pb both for now, so
> > that the server can accommodate a 'no hbase.rpc.codec' connection request
> > for now for backward compatibility.
> >
> >
> This is an arg for upping coverage for the pure-pb case and for not
> removing our client's ability to ask for this encoding?
>

Originally the thinking is to clean up any redundant pure pb code path in
the java native client but keep the server side logic.
But looks like we still need it in the client to cover and test for these
two scenarios:
1.  Old versions of java native clients with pure pb going to the new
server.
2.  As you explained, non native clients with pure pb.

So the thinking now is no cleaning, but up the coverage and tests if
anything is missing?  (I am sure there are).
Any suggestions?  I can dig and think more to see what can be done.

Thanks,

Jerry


> Thanks Jerry for bringing this up.
> S
>
>
> > Any concerns?
> >
> > Thanks.
> >
> > Jerry
> >
>