So the proposal is to avoid the empty config and have a better defined
config if we need No pb way? Ya I agree to it if this empty way seems odd
to config.
Any non - java client what will be the value for this config?
Regards
Ram
On Wed, Dec 20, 2017 at 8:40 AM, 张铎(Duo Zhang) <palomino...@gmail.com>
wrote:
> See AbstractTestIPC, there is a testNoCodec. But I agree that we should
> have a default fallback codec always.
>
> 2017-12-20 11:02 GMT+08:00 Jerry He <jerry...@gmail.com>:
>
> > 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
> >
>
