Mailing-List: contact hdfs-dev-help@hadoop.apache.org; run by ezmlm
Precedence: bulk
Reply-To: hdfs-dev@hadoop.apache.org
Received-SPF: neutral (nike.apache.org: local policy)
MIME-Version: 1.0
In-Reply-To: 
 <CADY20s6jGU8QFzxs8nSKbPEZP0JQe8c6TSAt5vDdhfkYCvHE_w@mail.gmail.com>
References: 
 <CADY20s58z8BenS8p8dUkR=4c_B0uc-Oat0HrmMQ9vYQ1ivm=Bw@mail.gmail.com>
	<CA+4052k4Le2Q2GDo=GNzmuhCndBoubJzL2gDABOwqj=UkSgFHg@mail.gmail.com>
	<CADY20s6jGU8QFzxs8nSKbPEZP0JQe8c6TSAt5vDdhfkYCvHE_w@mail.gmail.com>
Date: Thu, 9 Feb 2012 16:32:35 -0800
Message-ID: 
 <CADdVdVF7NkCv-v6tJ27hmdWtsnZLSFzcQfn1NgHkvZ8eJxNKFA@mail.gmail.com>
Subject: Re: Review request: trunk->HDFS-1623 merge
From: Suresh Srinivas <suresh@hortonworks.com>
To: hdfs-dev@hadoop.apache.org
Content-Type: multipart/alternative; boundary=20cf3079b88ce752c304b8914361

--20cf3079b88ce752c304b8914361
Content-Type: text/plain; charset=ISO-8859-1

I looked at the merge. It looks good. +1.

On Wed, Feb 8, 2012 at 9:08 PM, Todd Lipcon <todd@cloudera.com> wrote:

> The branch developed some new conflicts due to recent changes in trunk
> affecting the RPC between the DN and the NN (the "StorageReport"
> stuff). I've done a new merge to address these conflicts here:
>
> https://github.com/toddlipcon/hadoop-common/tree/ha-merge-20120208
>
> I've also addressed Aaron's comments in the thread above.
> I ran the unit tests on the branch and they passed.
>
> Thanks
> -Todd
>
> On Fri, Feb 3, 2012 at 4:44 PM, Aaron T. Myers <atm@cloudera.com> wrote:
> > 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
> >>
>
>
>
> --
> Todd Lipcon
> Software Engineer, Cloudera
>

--20cf3079b88ce752c304b8914361--