Jerry:
Your proposal sounds good.
On Tue, Dec 19, 2017 at 9:55 PM, Jerry He <jerry...@gmail.com> wrote:
> AbstractTestIPC is a good test. And it will continue to work after this
> proposed change, because we still want the server to be able to handle no
> codec (and pb) case, for backward compatibility.
> The proposal is to remove the support of no codec from the RpcClient at the
> moment.
> There will always be a default codec, and whoever is using the existence of
> codec to decide pb or no pb will always go pb now.
>
> Thanks.
>
> Jerry
>
>
> On Tue, Dec 19, 2017 at 9:18 PM, ramkrishna vasudevan <
> ramkrishna.s.vasude...@gmail.com> wrote:
>
> > 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
> > > >
> > >
> >
>
