hadoop-hdfs-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Todd Lipcon <t...@cloudera.com>
Subject Re: Review request: trunk->HDFS-1623 merge
Date Thu, 09 Feb 2012 21:42:14 GMT
Updated the merge against current trunk and HA branch. Here's the github link:

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.


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);
     case OP_CLOSE: {

On Thu, Feb 9, 2012 at 1:20 PM, Todd Lipcon <todd@cloudera.com> wrote:
> On Thu, Feb 9, 2012 at 12:54 PM, Konstantin Shvachko
> <shv.hadoop@gmail.com> 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 <todd@cloudera.com> 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
>>> <shv.hadoop@gmail.com> 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 <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>
>>>>>> 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.
>>>>>> makes sense in 0.22/0.23/0.24, but in the HA branch the OP_ADD and
>>>>>> cases are completely separate case blocks. I actually find this whole
>>>>>> comment a little confusing, since it numbers the cases we have to
>>>>>> but those numbers aren't referenced anywhere else.
>>>>>> 2. You mentioned in your message that you don't handle the (invalid)
>>>>>> of OP_ADD on a new file containing updated blocks, but it looks like
>>>>>> 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
>>>>>> 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
>>>>>> 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
>>>>>> 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>
>>>>>>> I've got a merge pending of trunk into HDFS-1623 -- it was a
>>>>>>> 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
>>>>>>> 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
>>>>>>> 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,
>>>>>>> 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
>>>>>>> they passed.
>>>>>>> -Todd
>>>>>>> --
>>>>>>> Todd Lipcon
>>>>>>> Software Engineer, Cloudera
>>>>> --
>>>>> Todd Lipcon
>>>>> Software Engineer, Cloudera
>>> --
>>> Todd Lipcon
>>> Software Engineer, Cloudera
> --
> Todd Lipcon
> Software Engineer, Cloudera

Todd Lipcon
Software Engineer, Cloudera

View raw message