On Tue, Dec 19, 2017 at 7:02 PM, Jerry He <jerry...@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.
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.
> 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.
> 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?
Thanks Jerry for bringing this up.
S
> Any concerns?
>
> Thanks.
>
> Jerry
>
