Ok. I will open a JIRA.
Thanks,
Jerry
On Wed, Dec 20, 2017 at 7:36 AM, Ted Yu <yuzhih...@gmail.com> wrote:
> 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
> > > > >
> > > >
> > >
> >
>
