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.
The consequence of this 'no codec' is that we will pb all RPC payload and
not using cell blocks.

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.
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.

Any concerns?

Thanks.

Jerry

Reply via email to