anoopsjohn commented on a change in pull request #2582:
URL: https://github.com/apache/hbase/pull/2582#discussion_r516440148
##########
File path:
hbase-common/src/main/java/org/apache/hadoop/hbase/SizeCachedKeyValue.java
##########
@@ -39,12 +39,22 @@
private short rowLen;
private int keyLen;
- public SizeCachedKeyValue(byte[] bytes, int offset, int length, long seqId) {
+ public SizeCachedKeyValue(byte[] bytes, int offset, int length, long seqId,
int keyLen) {
super(bytes, offset, length);
// We will read all these cached values at least once. Initialize now
itself so that we can
// avoid uninitialized checks with every time call
- rowLen = super.getRowLength();
- keyLen = super.getKeyLength();
+ this.rowLen = super.getRowLength();
Review comment:
Just call this() the next constructor
##########
File path:
hbase-common/src/main/java/org/apache/hadoop/hbase/SizeCachedNoTagsByteBufferKeyValue.java
##########
@@ -0,0 +1,72 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase;
+
+import java.nio.ByteBuffer;
+
+import org.apache.hadoop.hbase.util.Bytes;
+import org.apache.yetus.audience.InterfaceAudience;
+
+/**
+ * This Cell is an implementation of {@link ByteBufferExtendedCell} where the
data resides in
+ * off heap/ on heap ByteBuffer
+ */
[email protected]
+public class SizeCachedNoTagsByteBufferKeyValue extends
NoTagsByteBufferKeyValue {
+
Review comment:
nit: many empty rows.
##########
File path:
hbase-common/src/main/java/org/apache/hadoop/hbase/SizeCachedByteBufferKeyValue.java
##########
@@ -0,0 +1,80 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase;
+
+import java.nio.ByteBuffer;
+
+import org.apache.hadoop.hbase.util.Bytes;
+import org.apache.yetus.audience.InterfaceAudience;
+
+/**
+ * This Cell is an implementation of {@link ByteBufferExtendedCell} where the
data resides in
+ * off heap/ on heap ByteBuffer
+ */
[email protected]
+public class SizeCachedByteBufferKeyValue extends ByteBufferKeyValue {
+
+ private short rowLen;
+ private int keyLen;
+
+ public static final int FIXED_OVERHEAD = Bytes.SIZEOF_SHORT +
Bytes.SIZEOF_INT;
+
+ public SizeCachedByteBufferKeyValue(ByteBuffer buf, int offset, int length,
long seqId, int keyLen) {
+ super(buf, offset, length);
Review comment:
call next this() constructor
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderImpl.java
##########
@@ -554,8 +559,10 @@ protected int blockSeek(Cell key, boolean seekBefore) {
+ " path=" + reader.getPath());
}
offsetFromPos += Bytes.SIZEOF_LONG;
+ rowLen = ((blockBuffer.getByteAfterPosition(offsetFromPos) & 0xff) <<
8)
+ ^ (blockBuffer.getByteAfterPosition(offsetFromPos + 1) & 0xff);
blockBuffer.asSubByteBuffer(blockBuffer.position() + offsetFromPos,
klen, pair);
- bufBackedKeyOnlyKv.setKey(pair.getFirst(), pair.getSecond(), klen);
+ bufBackedKeyOnlyKv.setKey(pair.getFirst(), pair.getSecond(), klen,
(short)rowLen);
Review comment:
Whats the adv of decode rowLen here than in setKey()? If the rowLen was
already decoded for some other reason ya make sense to pass it than recalc
later. But here am not fully sure
##########
File path:
hbase-common/src/main/java/org/apache/hadoop/hbase/SizeCachedNoTagsByteBufferKeyValue.java
##########
@@ -0,0 +1,72 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase;
+
+import java.nio.ByteBuffer;
+
+import org.apache.hadoop.hbase.util.Bytes;
+import org.apache.yetus.audience.InterfaceAudience;
+
+/**
+ * This Cell is an implementation of {@link ByteBufferExtendedCell} where the
data resides in
+ * off heap/ on heap ByteBuffer
+ */
[email protected]
+public class SizeCachedNoTagsByteBufferKeyValue extends
NoTagsByteBufferKeyValue {
+
+
+ private short rowLen;
+ private int keyLen;
+
+ public static final int FIXED_OVERHEAD = Bytes.SIZEOF_SHORT +
Bytes.SIZEOF_INT;
+
+ public SizeCachedNoTagsByteBufferKeyValue(ByteBuffer buf, int offset, int
length, long seqId,
+ int keyLen) {
+ super(buf, offset, length);
Review comment:
Call next constructor this()
##########
File path:
hbase-common/src/main/java/org/apache/hadoop/hbase/io/encoding/RowIndexSeekerV1.java
##########
@@ -359,26 +361,30 @@ public Cell toCell() {
// TODO : reduce the varieties of KV here. Check if based on a boolean
// we can handle the 'no tags' case.
if (tagsLength > 0) {
+ // TODO : getRow len here.
Review comment:
Its ok not to get here but leave that to be decoded within
SizeCachedKeyValue. So we can remove this TODO IMO
##########
File path:
hbase-common/src/main/java/org/apache/hadoop/hbase/SizeCachedByteBufferKeyValue.java
##########
@@ -0,0 +1,80 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase;
+
+import java.nio.ByteBuffer;
+
+import org.apache.hadoop.hbase.util.Bytes;
+import org.apache.yetus.audience.InterfaceAudience;
+
+/**
+ * This Cell is an implementation of {@link ByteBufferExtendedCell} where the
data resides in
+ * off heap/ on heap ByteBuffer
+ */
[email protected]
+public class SizeCachedByteBufferKeyValue extends ByteBufferKeyValue {
+
+ private short rowLen;
+ private int keyLen;
+
+ public static final int FIXED_OVERHEAD = Bytes.SIZEOF_SHORT +
Bytes.SIZEOF_INT;
Review comment:
nit : Better we keep static fields above the instance members. Check for
all new classes
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderImpl.java
##########
@@ -790,23 +797,28 @@ public Cell getCell() {
// we can handle the 'no tags' case.
if (currTagsLen > 0) {
ret = new SizeCachedKeyValue(blockBuffer.array(),
- blockBuffer.arrayOffset() + blockBuffer.position(), cellBufSize,
seqId);
+ blockBuffer.arrayOffset() + blockBuffer.position(), cellBufSize,
seqId, currKeyLen,
+ rowLen);
} else {
ret = new SizeCachedNoTagsKeyValue(blockBuffer.array(),
- blockBuffer.arrayOffset() + blockBuffer.position(), cellBufSize,
seqId);
+ blockBuffer.arrayOffset() + blockBuffer.position(), cellBufSize,
seqId, currKeyLen,
+ rowLen);
}
} else {
ByteBuffer buf = blockBuffer.asSubByteBuffer(cellBufSize);
if (buf.isDirect()) {
- ret = currTagsLen > 0 ? new ByteBufferKeyValue(buf, buf.position(),
cellBufSize, seqId)
- : new NoTagsByteBufferKeyValue(buf, buf.position(), cellBufSize,
seqId);
+ ret = currTagsLen > 0
+ ? new SizeCachedByteBufferKeyValue(buf, buf.position(),
cellBufSize, seqId,
+ currKeyLen, rowLen)
+ : new SizeCachedNoTagsByteBufferKeyValue(buf, buf.position(),
cellBufSize, seqId,
+ currKeyLen, rowLen);
} else {
if (currTagsLen > 0) {
ret = new SizeCachedKeyValue(buf.array(), buf.arrayOffset() +
buf.position(),
- cellBufSize, seqId);
+ cellBufSize, seqId, currKeyLen, rowLen);
Review comment:
Ya here also as currKeyLen is already calculated we can pass.. But let
rowLen be decoded within SizeCachedKeyValue only. We can avoid all these
changes
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
