hbase-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Anoop Sam John (JIRA)" <j...@apache.org>
Subject [jira] [Comment Edited] (HBASE-11874) Support Cell to be passed to StoreFile.Writer rather than KeyValue
Date Mon, 08 Sep 2014 06:50:29 GMT

    [ 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:50 AM:
----------------------------------------------------------------

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?


was (Author: anoop.hbase):
That 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)

Mime
View raw message