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 < [email protected]> 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) <[email protected]> > 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 <[email protected]>: > > > > > 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 > > > > > >
