[
https://issues.apache.org/jira/browse/HBASE-11874?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14125255#comment-14125255
]
Anoop Sam John edited comment on HBASE-11874 at 9/8/14 6:51 AM:
----------------------------------------------------------------
Thanks for the look Stack. Yes requesting more close look as the changes
involve lots of Math. :)
bq.Is estimatedLengthOf used for allocating buffers copying Cells or KVs or
just an 'estimate'?
In this patch I have used this method in PrefixTreeCodec where this length will
be finally getting written as the total unencoded data size into the HFile
meta. Changing that to use KeyValueUtil#length() now. This method is not an
estimate but the length if serialized as in KeyValue. (So KVUtil is a better
place (?))
As part of HBASE-11805, I have used this estimateOfLength() in few places. Most
are like metric. One is used in Replication area to pass as size for
SizedCellScanner. Didnt get whether/how we use this size. I hope the estimate
is fine there.
bq.'writeFlatKey' means write it out as we used serialize a KV?
Yes. This is the way we will be writing keys to HFiles. I thought whether this
should put in KeyValueUtil as this takes Cell but writes as a KeyValue key.
Then finally decided to keep in CellUtil only but with decriptive javadoc.
Coundn't get names :) Suggestions pls..
bq.This one is a bit hard to grok: writeFlatKeyWithoutRowKey It needs to be
public?
Wanted one public API which can write all other key parts other than RK. This
is used from one more place and didnt want code duplication there. That is why
marked public. But I think not much of a code duplication. So will take away
this method. Yes the method might not worth a public in a public exposed Util
class
writeRowKeyPart What is the common part. Are we writing out the Cell as we
would a KV?
Yes this writes the non common part of the rk. (rk alone.) Again as KV way
only. <2 bytes rk len><rk bytes> Out of these bytes how many are common with
previous cell is what the commonPrefix says.
writeRowKeyLeavingCommon -> This is what it is doing.
bq.Are we doing more copies in this patch than previous?
Not too much expensive than the current way I would say. Tried to minimize it.
Non DBE case you can see
{code}
- fileInfo.append(FileInfo.LASTKEY, Arrays.copyOfRange(lastKeyBuffer,
- lastKeyOffset, lastKeyOffset + lastKeyLength), false);
+ byte[] lastKey = new byte[lastKeyLength];
+ KeyValueUtil.appendKeyTo(lastCell, lastKey, 0);
+ fileInfo.append(FileInfo.LASTKEY, lastKey, false);
{code}
The byte[] copying was already there. That was plain array copy. Now (In a
KeyValue case) appendKeyTo() will have to read the lengths from byte[] so some
extra decoding stuff.
Same with DBE case
Here for timestamp common checks, we will end up in creating ts byte[] of 8
bytes as we get long from Cell.getTimestamp().
This was one one of the reason why I tried with the new Interface way where
there can be exactly 0 overhead when KeyValue case (and also cases where Cell
imp is having a contigous byte[] key). But in this also not much of overhead
so should be fine IMO.
One cleaning I would like to do is , we use some of the Cell based APIs from
KeyValueUtil. These were existing APIs. Like keyLength() This is the key
length for the Cell if we serialzie key as in KeyValue. So KeyValueUtil is a
better place(?). Similar case for new APIs also (?) What do you say Stack?
was (Author: anoop.hbase):
Thaks for the look Stack. Yes requesting more close look as the changes involve
lots of Math. :)
bq.Is estimatedLengthOf used for allocating buffers copying Cells or KVs or
just an 'estimate'?
In this patch I have used this method in PrefixTreeCodec where this length will
be finally getting written as the total unencoded data size into the HFile
meta. Changing that to use KeyValueUtil#length() now. This method is not an
estimate but the length if serialized as in KeyValue. (So KVUtil is a better
place (?))
As part of HBASE-11805, I have used this estimateOfLength() in few places. Most
are like metric. One is used in Replication area to pass as size for
SizedCellScanner. Didnt get whether/how we use this size. I hope the estimate
is fine there.
bq.'writeFlatKey' means write it out as we used serialize a KV?
Yes. This is the way we will be writing keys to HFiles. I thought whether this
should put in KeyValueUtil as this takes Cell but writes as a KeyValue key.
Then finally decided to keep in CellUtil only but with decriptive javadoc.
Coundn't get names :)
bq.This one is a bit hard to grok: writeFlatKeyWithoutRowKey It needs to be
public?
Wanted one public API which can write all other key parts other than RK. This
is used from one more place and didnt want code duplication there. That is why
marked public. But I think not much of a code duplication. So will take away
this method. Yes the method might not worth a public in a public exposed Util
class
writeRowKeyPart What is the common part. Are we writing out the Cell as we
would a KV?
Yes this writes the non common part of the rk. (rk alone.) Again as KV way
only. <2 bytes rk len><rk bytes> Out of these bytes how many are common with
previous cell is what the commonPrefix says.
writeRowKeyLeavingCommon -> This is what it is doing.
bq.Are we doing more copies in this patch than previous?
Not too much expensive than the current way I would say. Tried to minimize it.
Non DBE case you can see
{code}
- fileInfo.append(FileInfo.LASTKEY, Arrays.copyOfRange(lastKeyBuffer,
- lastKeyOffset, lastKeyOffset + lastKeyLength), false);
+ byte[] lastKey = new byte[lastKeyLength];
+ KeyValueUtil.appendKeyTo(lastCell, lastKey, 0);
+ fileInfo.append(FileInfo.LASTKEY, lastKey, false);
{code}
The byte[] copying was already there. That was plain array copy. Now (In a
KeyValue case) appendKeyTo() will have to read the lengths from byte[] so some
extra decoding stuff.
Same with DBE case
Here for timestamp common checks, we will end up in creating ts byte[] of 8
bytes as we get long from Cell.getTimestamp().
This was one one of the reason why I tried with the new Interface way where
there can be exactly 0 overhead when KeyValue case (and also cases where Cell
imp is having a contigous byte[] key). But in this also not much of overhead
so should be fine IMO.
One cleaning I would like to do is , we use some of the Cell based APIs from
KeyValueUtil. These were existing APIs. Like keyLength() This is the key
length for the Cell if we serialzie key as in KeyValue. So KeyValueUtil is a
better place(?). Similar case for new APIs also (?) What do you say Stack?
> Support Cell to be passed to StoreFile.Writer rather than KeyValue
> ------------------------------------------------------------------
>
> Key: HBASE-11874
> URL: https://issues.apache.org/jira/browse/HBASE-11874
> Project: HBase
> Issue Type: Sub-task
> Reporter: Anoop Sam John
> Assignee: Anoop Sam John
> Fix For: 0.99.0, 2.0.0
>
> Attachments: HBASE-11874.patch, HBASE-11874_V2.patch,
> HBASE-11874_V3.patch, HBASE-11874_V3.patch, HBASE-11874_V4.patch
>
>
> This is the in write path and touches StoreFile.Writer, HFileWriter ,
> HFileDataBlockEncoder and different DataBlockEncoder impl.
> We will have to avoid KV#getBuffer() KV#getKeyOffset/Length() calls.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
