HFile Code Review

[Schubert Zhang <zs...@gmail.com>]

Hi stack,

I am reviewing the code of HFile. And I will post my comments here.

Comments:

1. a minor mistake in HFile.Writer.writeFileInfo(FSDataOutputStream o)

     int avgValueLen = this.entryCount == 0? 0:
        (int)(this.keylength/this.entryCount);

    Here this.keylength should be this.valuelength.

2.  I found it is very uncomfortable to check or convert data types of
Integer and Long.
     I think HFile will be a common file format in the future, so it is
better to avoid limitations.

    My suggestions:

    private int totalBytes = 0;  // long is better, in fact the parameter in
Trailer is long.
    private int entryCount = 0; // long is better
    private int blocksize; //maybe long is better?

    it we change type of these variables, some parameters in some methods
should also be changed.

Schubert

Re: HFile Code Review

[stack <st...@duboce.net>]

Excellent.  Thanks for the review Schubert.  I'd suggest you collect all
your comments into one JIRA (Below all looks good except for perhaps the
last two suggestions).
Thanks,
St.Ack



On Sun, Aug 30, 2009 at 3:33 AM, Schubert Zhang <zs...@gmail.com> wrote:

> Hi stack,
>
> I am reviewing the code of HFile. And I will post my comments here.
>
> Comments:
>
> 1. a minor mistake in HFile.Writer.writeFileInfo(FSDataOutputStream o)
>
>     int avgValueLen = this.entryCount == 0? 0:
>        (int)(this.keylength/this.entryCount);
>
>    Here this.keylength should be this.valuelength.
>
> 2.  I found it is very uncomfortable to check or convert data types of
> Integer and Long.
>     I think HFile will be a common file format in the future, so it is
> better to avoid limitations.
>
>    My suggestions:
>
>    private int totalBytes = 0;  // long is better, in fact the parameter in
> Trailer is long.
>    private int entryCount = 0; // long is better
>    private int blocksize; //maybe long is better?
>
>    it we change type of these variables, some parameters in some methods
> should also be changed.
>
> Schubert
>