[GitHub] [hbase] bbeaudreault commented on a diff in pull request #5016: HBASE-27621 Always use findEntry to fill the Dictionary when reading …

["bbeaudreault (via GitHub)" <gi...@apache.org>]

bbeaudreault commented on code in PR #5016:
URL: https://github.com/apache/hbase/pull/5016#discussion_r1100233880


##########
hbase-common/src/main/java/org/apache/hadoop/hbase/io/TagCompressionContext.java:
##########
@@ -107,15 +107,15 @@ public void uncompressTags(InputStream src, byte[] dest, int offset, int length)
     throws IOException {
     int endOffset = offset + length;
     while (offset < endOffset) {
-      byte status = (byte) src.read();
+      byte status = StreamUtils.readByte(src);
       if (status == Dictionary.NOT_IN_DICTIONARY) {
         int tagLen = StreamUtils.readRawVarint32(src);
         offset = Bytes.putAsShort(dest, offset, tagLen);
         IOUtils.readFully(src, dest, offset, tagLen);
-        tagDict.addEntry(dest, offset, tagLen);
+        tagDict.findEntry(dest, offset, tagLen);

Review Comment:
   Do you think this change could be expensive? In the normal case, the entry will not exist in the dict. But now we're adding an extra map lookup for every call. Granted o(1), but involves cpu for hashcode, allocating lookup key, etc.
   
   I wonder if we could trigger findEntry only if context has been reset? Otherwise use addEntry for first pass?
   
   May not be a big issue, just checking



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org