You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hbase.apache.org by Jerry He <je...@gmail.com> on 2017/12/20 03:02:04 UTC
Cleanup and remove the code path where is no hbase.client.rpc.codec
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
Re: Cleanup and remove the code path where is no hbase.client.rpc.codec
Posted by Jerry He <je...@gmail.com>.
On Thu, Dec 21, 2017 at 12:03 AM, Stack <st...@duboce.net> wrote:
> On Tue, Dec 19, 2017 at 7:02 PM, Jerry He <je...@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.
Thanks for the background information. The intention for non native client
is a good intention.
> 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.
Make sense.
>
> > 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.
>
>
Make sense. The intention is good. It may be hard for us to maintain
though.
It will be a burden we make for the java native client to carry if we
continue to use it to cover the pure pb case.
> > 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?
>
Originally the thinking is to clean up any redundant pure pb code path in
the java native client but keep the server side logic.
But looks like we still need it in the client to cover and test for these
two scenarios:
1. Old versions of java native clients with pure pb going to the new
server.
2. As you explained, non native clients with pure pb.
So the thinking now is no cleaning, but up the coverage and tests if
anything is missing? (I am sure there are).
Any suggestions? I can dig and think more to see what can be done.
Thanks,
Jerry
> Thanks Jerry for bringing this up.
> S
>
>
> > Any concerns?
> >
> > Thanks.
> >
> > Jerry
> >
>
Re: Cleanup and remove the code path where is no hbase.client.rpc.codec
Posted by Stack <st...@duboce.net>.
On Tue, Dec 19, 2017 at 7:02 PM, Jerry He <je...@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
>
Re: Cleanup and remove the code path where is no hbase.client.rpc.codec
Posted by Jerry He <je...@gmail.com>.
Ok. I will open a JIRA.
Thanks,
Jerry
On Wed, Dec 20, 2017 at 7:36 AM, Ted Yu <yu...@gmail.com> wrote:
> Jerry:
> Your proposal sounds good.
>
> On Tue, Dec 19, 2017 at 9:55 PM, Jerry He <je...@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.vasudevan@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) <pa...@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 <je...@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
> > > > >
> > > >
> > >
> >
>
Re: Cleanup and remove the code path where is no hbase.client.rpc.codec
Posted by Ted Yu <yu...@gmail.com>.
Jerry:
Your proposal sounds good.
On Tue, Dec 19, 2017 at 9:55 PM, Jerry He <je...@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.vasudevan@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) <pa...@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 <je...@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
> > > >
> > >
> >
>
Re: Cleanup and remove the code path where is no hbase.client.rpc.codec
Posted by Jerry He <je...@gmail.com>.
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.vasudevan@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) <pa...@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 <je...@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
> > >
> >
>
Re: Cleanup and remove the code path where is no hbase.client.rpc.codec
Posted by ramkrishna vasudevan <ra...@gmail.com>.
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) <pa...@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 <je...@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
> >
>
Re: Cleanup and remove the code path where is no hbase.client.rpc.codec
Posted by "张铎 (Duo Zhang)" <pa...@gmail.com>.
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 <je...@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
>