Ok. I will open a JIRA. Thanks,
Jerry On Wed, Dec 20, 2017 at 7:36 AM, Ted Yu <[email protected]> wrote: > Jerry: > Your proposal sounds good. > > On Tue, Dec 19, 2017 at 9:55 PM, Jerry He <[email protected]> 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 < > > [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 > > > > > > > > > > > > > > >
