Todd Lipcon
2012-02-03, 22:10
Aaron T. Myers
2012-02-04, 00:44
Suresh Srinivas
2012-02-04, 02:45
Aaron T. Myers
2012-02-04, 03:02
Todd Lipcon
2012-02-09, 05:08
Konstantin Shvachko
2012-02-09, 18:54
Todd Lipcon
2012-02-09, 18:57
Konstantin Shvachko
2012-02-09, 20:54
Todd Lipcon
2012-02-09, 21:20
Todd Lipcon
2012-02-09, 21:42
Suresh Srinivas
2012-02-10, 00:32
Todd Lipcon
2012-02-10, 01:50
-
Review request: trunk->HDFS-1623 mergeTodd Lipcon 2012-02-03, 22:10
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
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
-
Re: Review request: trunk->HDFS-1623 mergeAaron T. Myers 2012-02-04, 00:44
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 <[EMAIL PROTECTED]> 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
>
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 <[EMAIL PROTECTED]> 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
>
-
Re: Review request: trunk->HDFS-1623 mergeSuresh Srinivas 2012-02-04, 02:45
Todd, can you please hold off for the merge till Tuesday, until some of the
other folks working on HA could catch up with some of the recent changes.
On Fri, Feb 3, 2012 at 2:10 PM, Todd Lipcon <[EMAIL PROTECTED]> 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
>
other folks working on HA could catch up with some of the recent changes.
On Fri, Feb 3, 2012 at 2:10 PM, Todd Lipcon <[EMAIL PROTECTED]> 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
>
-
Re: Review request: trunk->HDFS-1623 mergeAaron T. Myers 2012-02-04, 03:02
On Fri, Feb 3, 2012 at 6:45 PM, Suresh Srinivas <[EMAIL PROTECTED]>wrote:
> Todd, can you please hold off for the merge till Tuesday, until some of the
> other folks working on HA could catch up with some of the recent changes.
>
Note that this merge doesn't have anything to do with any *recent* changes
to the HA branch. The conflicts with HDFS-2718 are from HDFS-2602, which
was committed back in December.
--
Aaron T. Myers
Software Engineer, Cloudera
> Todd, can you please hold off for the merge till Tuesday, until some of the
> other folks working on HA could catch up with some of the recent changes.
>
Note that this merge doesn't have anything to do with any *recent* changes
to the HA branch. The conflicts with HDFS-2718 are from HDFS-2602, which
was committed back in December.
--
Aaron T. Myers
Software Engineer, Cloudera
-
Re: Review request: trunk->HDFS-1623 mergeTodd Lipcon 2012-02-09, 05:08
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 <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> 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
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 <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> 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
-
Re: Review request: trunk->HDFS-1623 mergeKonstantin Shvachko 2012-02-09, 18:54
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 <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> 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
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 <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> 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
-
Re: Review request: trunk->HDFS-1623 mergeTodd Lipcon 2012-02-09, 18:57
Hi Konstantin,
To be clear, this review request is a merge from the trunk branch into
the HA branch (NOT a merge INTO trunk) - we've been doing these pretty
much daily since the project began, so that we track trunk closely.
The idea is so that we don't have unexpected integration issues when
we do the merge the _other_ way when it's ready.
When we propose the merge *into* trunk we'll definitely address your
questions below. We are indeed already running cluster tests at
100-node scale with failovers and both MR and HBase workloads, though
have not done a formal performance comparison at this point.
-Todd
On Thu, Feb 9, 2012 at 10:54 AM, Konstantin Shvachko
<[EMAIL PROTECTED]> wrote:
> 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 <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> 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
Todd Lipcon
Software Engineer, Cloudera
To be clear, this review request is a merge from the trunk branch into
the HA branch (NOT a merge INTO trunk) - we've been doing these pretty
much daily since the project began, so that we track trunk closely.
The idea is so that we don't have unexpected integration issues when
we do the merge the _other_ way when it's ready.
When we propose the merge *into* trunk we'll definitely address your
questions below. We are indeed already running cluster tests at
100-node scale with failovers and both MR and HBase workloads, though
have not done a formal performance comparison at this point.
-Todd
On Thu, Feb 9, 2012 at 10:54 AM, Konstantin Shvachko
<[EMAIL PROTECTED]> wrote:
> 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 <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> 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
Todd Lipcon
Software Engineer, Cloudera
-
Re: Review request: trunk->HDFS-1623 mergeKonstantin Shvachko 2012-02-09, 20:54
Ok I misunderstood the current direction of the merge.
On the review request:
> 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 intentionally did not remove handling of new files with blocks (non
empty). The reason is potential issues with backward compatibility. If
any HDFS version in the past produced such transactions the new
version must be able to read them. I thought it is easier to retain
the support for this type of transactions rather than checking all
past versions.
I would not recommend restricting OP_ADD in the way that it requires
new files to be empty.
Thanks,
--Konstantin
On Thu, Feb 9, 2012 at 10:57 AM, Todd Lipcon <[EMAIL PROTECTED]> wrote:
> Hi Konstantin,
>
> To be clear, this review request is a merge from the trunk branch into
> the HA branch (NOT a merge INTO trunk) - we've been doing these pretty
> much daily since the project began, so that we track trunk closely.
> The idea is so that we don't have unexpected integration issues when
> we do the merge the _other_ way when it's ready.
>
> When we propose the merge *into* trunk we'll definitely address your
> questions below. We are indeed already running cluster tests at
> 100-node scale with failovers and both MR and HBase workloads, though
> have not done a formal performance comparison at this point.
>
> -Todd
>
> On Thu, Feb 9, 2012 at 10:54 AM, Konstantin Shvachko
> <[EMAIL PROTECTED]> wrote:
>> 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 <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> 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 the review request:
> 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 intentionally did not remove handling of new files with blocks (non
empty). The reason is potential issues with backward compatibility. If
any HDFS version in the past produced such transactions the new
version must be able to read them. I thought it is easier to retain
the support for this type of transactions rather than checking all
past versions.
I would not recommend restricting OP_ADD in the way that it requires
new files to be empty.
Thanks,
--Konstantin
On Thu, Feb 9, 2012 at 10:57 AM, Todd Lipcon <[EMAIL PROTECTED]> wrote:
> Hi Konstantin,
>
> To be clear, this review request is a merge from the trunk branch into
> the HA branch (NOT a merge INTO trunk) - we've been doing these pretty
> much daily since the project began, so that we track trunk closely.
> The idea is so that we don't have unexpected integration issues when
> we do the merge the _other_ way when it's ready.
>
> When we propose the merge *into* trunk we'll definitely address your
> questions below. We are indeed already running cluster tests at
> 100-node scale with failovers and both MR and HBase workloads, though
> have not done a formal performance comparison at this point.
>
> -Todd
>
> On Thu, Feb 9, 2012 at 10:54 AM, Konstantin Shvachko
> <[EMAIL PROTECTED]> wrote:
>> 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 <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> 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
-
Re: Review request: trunk->HDFS-1623 mergeTodd Lipcon 2012-02-09, 21:20
On Thu, Feb 9, 2012 at 12:54 PM, Konstantin Shvachko
<[EMAIL PROTECTED]> wrote:
> Ok I misunderstood the current direction of the merge.
>
> On the review request:
>
>> 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 intentionally did not remove handling of new files with blocks (non
> empty). The reason is potential issues with backward compatibility. If
> any HDFS version in the past produced such transactions the new
> version must be able to read them. I thought it is easier to retain
> the support for this type of transactions rather than checking all
> past versions.
> I would not recommend restricting OP_ADD in the way that it requires
> new files to be empty.
Good point. I checked back in old versions and it appears that we had
this behavior in 0.16 and below (removed in HADOOP-2345) in 0.17.
I'll update the merge to continue to support this old behavior, and
leave a note that it could be simplified by bumping our minimum
upgrade version to 0.17 or 0.18 (which seems entirely reasonable given
they're ~4 years old).
Will report back when a new patch is up for review.
-Todd
>
> Thanks,
> --Konstantin
>
> On Thu, Feb 9, 2012 at 10:57 AM, Todd Lipcon <[EMAIL PROTECTED]> wrote:
>> Hi Konstantin,
>>
>> To be clear, this review request is a merge from the trunk branch into
>> the HA branch (NOT a merge INTO trunk) - we've been doing these pretty
>> much daily since the project began, so that we track trunk closely.
>> The idea is so that we don't have unexpected integration issues when
>> we do the merge the _other_ way when it's ready.
>>
>> When we propose the merge *into* trunk we'll definitely address your
>> questions below. We are indeed already running cluster tests at
>> 100-node scale with failovers and both MR and HBase workloads, though
>> have not done a formal performance comparison at this point.
>>
>> -Todd
>>
>> On Thu, Feb 9, 2012 at 10:54 AM, Konstantin Shvachko
>> <[EMAIL PROTECTED]> wrote:
>>> 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 <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> 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
Todd Lipcon
Software Engineer, Cloudera
<[EMAIL PROTECTED]> wrote:
> Ok I misunderstood the current direction of the merge.
>
> On the review request:
>
>> 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 intentionally did not remove handling of new files with blocks (non
> empty). The reason is potential issues with backward compatibility. If
> any HDFS version in the past produced such transactions the new
> version must be able to read them. I thought it is easier to retain
> the support for this type of transactions rather than checking all
> past versions.
> I would not recommend restricting OP_ADD in the way that it requires
> new files to be empty.
Good point. I checked back in old versions and it appears that we had
this behavior in 0.16 and below (removed in HADOOP-2345) in 0.17.
I'll update the merge to continue to support this old behavior, and
leave a note that it could be simplified by bumping our minimum
upgrade version to 0.17 or 0.18 (which seems entirely reasonable given
they're ~4 years old).
Will report back when a new patch is up for review.
-Todd
>
> Thanks,
> --Konstantin
>
> On Thu, Feb 9, 2012 at 10:57 AM, Todd Lipcon <[EMAIL PROTECTED]> wrote:
>> Hi Konstantin,
>>
>> To be clear, this review request is a merge from the trunk branch into
>> the HA branch (NOT a merge INTO trunk) - we've been doing these pretty
>> much daily since the project began, so that we track trunk closely.
>> The idea is so that we don't have unexpected integration issues when
>> we do the merge the _other_ way when it's ready.
>>
>> When we propose the merge *into* trunk we'll definitely address your
>> questions below. We are indeed already running cluster tests at
>> 100-node scale with failovers and both MR and HBase workloads, though
>> have not done a formal performance comparison at this point.
>>
>> -Todd
>>
>> On Thu, Feb 9, 2012 at 10:54 AM, Konstantin Shvachko
>> <[EMAIL PROTECTED]> wrote:
>>> 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 <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> 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
Todd Lipcon
Software Engineer, Cloudera
-
Re: Review request: trunk->HDFS-1623 mergeTodd Lipcon 2012-02-09, 21:42
Updated the merge against current trunk and HA branch. Here's the github link:
https://github.com/toddlipcon/hadoop-common/commits/ha-merge-20120209
And the relevant diff reproduced below.
If this looks mostly good, please +1 - we can continue to make
improvements on the branch, but redoing the merges as both branches
move underneath takes a lot of time.
-Todd
diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdf
index 96840f6..bf1ec99 100644
--- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/serve
+++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/serve
@@ -197,9 +197,12 @@ public class FSEditLogLoader {
permissions = addCloseOp.permissions;
}
long blockSize = addCloseOp.blockSize;
-
- // TODO: we should add a sanity check that there are no blocks
- // in this op, and simplify the code below!
+
+ // Versions of HDFS prior to 0.17 may log an OP_ADD transaction
+ // which includes blocks in it. When we update the minimum
+ // upgrade version to something more recent than 0.17, we can
+ // simplify this code by asserting that OP_ADD transactions
+ // don't have any blocks.
// Older versions of HDFS does not store the block size in inode.
// If the file has more than one block, use the size of the
@@ -237,13 +240,9 @@ public class FSEditLogLoader {
}
}
// Fall-through for case 2.
- // TODO: this is below the if/else above in order to handle the
- // case where OP_ADD is used to add a new file, and the new
- // file has blocks in the first opcode. However, this case
- // doesn't happen in practice. Will address in a separate
- // JIRA so as to avoid changing too much code in a merge commit.
+ // Regardless of whether it's a new file or an updated file,
+ // update the block list.
updateBlocks(fsDir, addCloseOp, newFile);
-
break;
}
case OP_CLOSE: {
On Thu, Feb 9, 2012 at 1:20 PM, Todd Lipcon <[EMAIL PROTECTED]> wrote:
> On Thu, Feb 9, 2012 at 12:54 PM, Konstantin Shvachko
> <[EMAIL PROTECTED]> wrote:
>> Ok I misunderstood the current direction of the merge.
>>
>> On the review request:
>>
>>> 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 intentionally did not remove handling of new files with blocks (non
>> empty). The reason is potential issues with backward compatibility. If
>> any HDFS version in the past produced such transactions the new
>> version must be able to read them. I thought it is easier to retain
>> the support for this type of transactions rather than checking all
>> past versions.
>> I would not recommend restricting OP_ADD in the way that it requires
>> new files to be empty.
>
> Good point. I checked back in old versions and it appears that we had
> this behavior in 0.16 and below (removed in HADOOP-2345) in 0.17.
>
> I'll update the merge to continue to support this old behavior, and
> leave a note that it could be simplified by bumping our minimum
> upgrade version to 0.17 or 0.18 (which seems entirely reasonable given
> they're ~4 years old).
>
> Will report back when a new patch is up for review.
>
> -Todd
>
>>
>> Thanks,
>> --Konstantin
>>
>> On Thu, Feb 9, 2012 at 10:57 AM, Todd Lipcon <[EMAIL PROTECTED]> wrote:
>>> Hi Konstantin,
>>>
>>> To be clear, this review request is a merge from the trunk branch into
>>> the HA branch (NOT a merge INTO trunk) - we've been doing these pretty
>>> much daily since the project began, so that we track trunk closely.
>>> The idea is so that we don't have unexpected integration issues when
>>> we do the merge the _other_ way when it's ready.
>>>
>>> When we propose the merge *into* trunk we'll definitely address your
>>> questions below. We are indeed already running cluster tests at
Todd Lipcon
Software Engineer, Cloudera
https://github.com/toddlipcon/hadoop-common/commits/ha-merge-20120209
And the relevant diff reproduced below.
If this looks mostly good, please +1 - we can continue to make
improvements on the branch, but redoing the merges as both branches
move underneath takes a lot of time.
-Todd
diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdf
index 96840f6..bf1ec99 100644
--- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/serve
+++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/serve
@@ -197,9 +197,12 @@ public class FSEditLogLoader {
permissions = addCloseOp.permissions;
}
long blockSize = addCloseOp.blockSize;
-
- // TODO: we should add a sanity check that there are no blocks
- // in this op, and simplify the code below!
+
+ // Versions of HDFS prior to 0.17 may log an OP_ADD transaction
+ // which includes blocks in it. When we update the minimum
+ // upgrade version to something more recent than 0.17, we can
+ // simplify this code by asserting that OP_ADD transactions
+ // don't have any blocks.
// Older versions of HDFS does not store the block size in inode.
// If the file has more than one block, use the size of the
@@ -237,13 +240,9 @@ public class FSEditLogLoader {
}
}
// Fall-through for case 2.
- // TODO: this is below the if/else above in order to handle the
- // case where OP_ADD is used to add a new file, and the new
- // file has blocks in the first opcode. However, this case
- // doesn't happen in practice. Will address in a separate
- // JIRA so as to avoid changing too much code in a merge commit.
+ // Regardless of whether it's a new file or an updated file,
+ // update the block list.
updateBlocks(fsDir, addCloseOp, newFile);
-
break;
}
case OP_CLOSE: {
On Thu, Feb 9, 2012 at 1:20 PM, Todd Lipcon <[EMAIL PROTECTED]> wrote:
> On Thu, Feb 9, 2012 at 12:54 PM, Konstantin Shvachko
> <[EMAIL PROTECTED]> wrote:
>> Ok I misunderstood the current direction of the merge.
>>
>> On the review request:
>>
>>> 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 intentionally did not remove handling of new files with blocks (non
>> empty). The reason is potential issues with backward compatibility. If
>> any HDFS version in the past produced such transactions the new
>> version must be able to read them. I thought it is easier to retain
>> the support for this type of transactions rather than checking all
>> past versions.
>> I would not recommend restricting OP_ADD in the way that it requires
>> new files to be empty.
>
> Good point. I checked back in old versions and it appears that we had
> this behavior in 0.16 and below (removed in HADOOP-2345) in 0.17.
>
> I'll update the merge to continue to support this old behavior, and
> leave a note that it could be simplified by bumping our minimum
> upgrade version to 0.17 or 0.18 (which seems entirely reasonable given
> they're ~4 years old).
>
> Will report back when a new patch is up for review.
>
> -Todd
>
>>
>> Thanks,
>> --Konstantin
>>
>> On Thu, Feb 9, 2012 at 10:57 AM, Todd Lipcon <[EMAIL PROTECTED]> wrote:
>>> Hi Konstantin,
>>>
>>> To be clear, this review request is a merge from the trunk branch into
>>> the HA branch (NOT a merge INTO trunk) - we've been doing these pretty
>>> much daily since the project began, so that we track trunk closely.
>>> The idea is so that we don't have unexpected integration issues when
>>> we do the merge the _other_ way when it's ready.
>>>
>>> When we propose the merge *into* trunk we'll definitely address your
>>> questions below. We are indeed already running cluster tests at
Todd Lipcon
Software Engineer, Cloudera
-
Re: Review request: trunk->HDFS-1623 mergeSuresh Srinivas 2012-02-10, 00:32
I looked at the merge. It looks good. +1.
On Wed, Feb 8, 2012 at 9:08 PM, Todd Lipcon <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> 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
>
On Wed, Feb 8, 2012 at 9:08 PM, Todd Lipcon <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> 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
>
-
Re: Review request: trunk->HDFS-1623 mergeTodd Lipcon 2012-02-10, 01:50
Thanks Suresh. I committed the merge to the branch.
-Todd
On Thu, Feb 9, 2012 at 4:32 PM, Suresh Srinivas <[EMAIL PROTECTED]> wrote:
> I looked at the merge. It looks good. +1.
>
> On Wed, Feb 8, 2012 at 9:08 PM, Todd Lipcon <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> 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
>>
--
Todd Lipcon
Software Engineer, Cloudera
-Todd
On Thu, Feb 9, 2012 at 4:32 PM, Suresh Srinivas <[EMAIL PROTECTED]> wrote:
> I looked at the merge. It looks good. +1.
>
> On Wed, Feb 8, 2012 at 9:08 PM, Todd Lipcon <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> 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
>>
--
Todd Lipcon
Software Engineer, Cloudera
