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 >
