hbase-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "ryan rawson (JIRA)" <j...@apache.org>
Subject [jira] Commented: (HBASE-1818) HFile code review and refinement
Date Wed, 09 Sep 2009 23:33:57 GMT

    [ https://issues.apache.org/jira/browse/HBASE-1818?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12753327#action_12753327

ryan rawson commented on HBASE-1818:

Here are some comments from the svn commit:

        if (this.comparator.compare(this.lastKeyBuffer, this.lastKeyOffset,
-            this.lastKeyLength, key, offset, length) > 0) {
+            this.lastKeyLength, key, offset, length) >= 0) {
          throw new IOException("Added a key not lexically larger than" +
            " previous key=" + Bytes.toString(key, offset, length) +
            ", lastkey=" + Bytes.toString(this.lastKeyBuffer, this.lastKeyOffset,

This change has the _Exact opposite_ effect from the intent.  The intent (as I construct it)
is to allow duplicate keys in a HFile, yet by adding >= you are explicitly requiring the
next key to be larger, but NOT equal to the previous key.  This has to stay as is.

+      if (buf == null)
+       return null;

This is pointless, we'll NPE a few lines later anyways. Don't add checking code that doesnt
have any benefits - the caller may not be interpret a null return as 'error' in this case,
did you check that?

-      // Toss the header. May have to remove later due to performance.
-      buf.compact();
-      buf.limit(buf.limit() - METABLOCKMAGIC.length);
-      buf.rewind();

I am not super happy with this change... it doesnt affect performance, but it does reduce
code simplicity, and leaves open room for errors further on, and requires everyone to realize
that the buf has this hidden header which isnt part of the public API of HFile.

> HFile code review and refinement
> --------------------------------
>                 Key: HBASE-1818
>                 URL: https://issues.apache.org/jira/browse/HBASE-1818
>             Project: Hadoop HBase
>          Issue Type: Improvement
>          Components: io
>    Affects Versions: 0.20.0
>            Reporter: Schubert Zhang
>            Assignee: Schubert Zhang
>            Priority: Minor
>             Fix For: 0.21.0
>         Attachments: HFile-v1.patch, HFile-v2.patch
> HFile is a good mimic of Google's SSTable file format. And we want HFile to become a
common file format of hadoop in the near future.
> We will review the code of HFile and record the comments here, and then provide fixed
patch after the review.

This message is automatically generated by JIRA.
You can reply to this email to add a comment to the issue online.

View raw message