hadoop-hdfs-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Aaron T. Myers" <...@cloudera.com>
Subject Re: Review request: trunk->HDFS-1623 merge
Date Sat, 04 Feb 2012 00:44:02 GMT
Hey Todd,

The merge largely looks good. I agree with the general approach you took. A
few small comments:

1. There's a comment in the OP_ADD case blockabout handling OP_CLOSE. This
makes sense in 0.22/0.23/0.24, but in the HA branch the OP_ADD and OP_CLOSE
cases are completely separate case blocks. I actually find this whole
comment a little confusing, since it numbers the cases we have to handle,
but those numbers aren't referenced anywhere else.

2. You mentioned in your message that you don't handle the (invalid) case
of OP_ADD on a new file containing updated blocks, but it looks like the
code actually does, though the code also mentions that we should add a
sanity check that this is actually can't occur. Seems like we should clean
up this inconsistency. I agree that adding asserting this case doesn't
occur is the right way to go.

3. If we go with my suggestion in (2), we can also move the call to
FSEditLogLoader#updateBlocks to only the case of OP_ADD for an existing
file, and then get rid of the "INodeFile newFile = oldFile" assignment,
which I found kind of confusing at first. (Though I do see why it's correct
as-implemented.) If you don't go with my suggestion in (2), please add a
comment explaining the assignment.

Otherwise looks good. Merge away.

Aaron T. Myers
Software Engineer, Cloudera

On Fri, Feb 3, 2012 at 2:10 PM, Todd Lipcon <todd@cloudera.com> wrote:

> I've got a merge pending of trunk into HDFS-1623 -- it was a bit
> complicated so wanted to ask for another set of eyes:
> https://github.com/toddlipcon/hadoop-common/tree/ha-merge-20120203
> (using github since it's hard to review a merge patch via JIRA)
> The interesting bit of the merge was to deal with conflicts with
> HDFS-2718. To summarize the changes I had to make:
> - in the HDFS-1623 branch, we don't deal with the case where OP_ADD
> contains blocks on a new file -- this is a case that doesn't happen on
> real clusters, but currently happens with synthetic logs generated
> from the CreateEditLogs tool. I added a TODO to add a sanity check
> here and will address as a follow-up. Given the difference between
> trunk and branch, there were a couple of small changes that propagated
> into unprotectedAddFile
> - In the HDFS-1623 branch we had already implemented the
> "updateBlocks" call inside FSEditLogLoader. I used that existing
> implementation rather than adding the new one in FSDirectory, since
> this function had some other changes related to HA in the branch
> version.
> I'll wait for a +1 before committing. I ran all of the unit tests and
> they passed.
> -Todd
> --
> Todd Lipcon
> Software Engineer, Cloudera

  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message