hadoop-hdfs-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Konstantin Shvachko <shv.had...@gmail.com>
Subject Re: Review request: trunk->HDFS-1623 merge
Date Thu, 09 Feb 2012 18:54:53 GMT
I was wondering
1. What was the test plan that has been executed for testing this
implementation of HA? Besides unit tests.
2. Have you done any benchmarks, comparing current cluster performance
against the branch. Would be good to have numbers for both cases with
HA off and HA on.

I'll post these questions to the jira as well.

Thanks,
--Konstantin

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

Mime
View raw message