Uploaded image for project: 'Bigtop'
  1. Bigtop
  2. BIGTOP-1560

Add a test case for performing block corruption recovery

    Details

    • Type: Test
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 0.8.0
    • Fix Version/s: 1.0.0
    • Component/s: tests
    • Labels:
      None

      Description

      Found this issue in internal testing. Roughly:

      create file in HDFS
      find block for the file
      corrupt a block
      trigger recovery by trying to read the file
      check recovery was successful

      1. BIGTOP-1560.patch
        9 kB
        Dasha Boudnik
      2. BIGTOP-1560.patch
        9 kB
        Dasha Boudnik
      3. BIGTOP-1560.patch
        9 kB
        Dasha Boudnik
      4. BIGTOP-1560.patch
        9 kB
        Dasha Boudnik
      5. BIGTOP-1560.patch
        9 kB
        Dasha Boudnik

        Issue Links

          Activity

          Hide
          dasha.boudnik Dasha Boudnik added a comment -

          Patch attached

          Show
          dasha.boudnik Dasha Boudnik added a comment - Patch attached
          Hide
          cos Konstantin Boudnik added a comment -

          Patch looks good in a quick glance. A couple of suggestions:

          • + assert (numberOfDataNodes >= 3) : "Blocks cannot recover when there is only one datanode."
            I would suggest to use Assume.assumeTrue() to simply skip the test with a message is the condition isn't met
          • considering that you are doing a number of operations under hdfs user, it would make sense to either skip or fail the test if current user is different. So, perhaps you can USERNAME is set as desired.

          Thanks for doing this - pretty complex test, BTW!

          Show
          cos Konstantin Boudnik added a comment - Patch looks good in a quick glance. A couple of suggestions: + assert (numberOfDataNodes >= 3) : "Blocks cannot recover when there is only one datanode." I would suggest to use Assume.assumeTrue() to simply skip the test with a message is the condition isn't met considering that you are doing a number of operations under hdfs user, it would make sense to either skip or fail the test if current user is different. So, perhaps you can USERNAME is set as desired. Thanks for doing this - pretty complex test, BTW!
          Hide
          jayunit100 jay vyas added a comment - - edited

          This is an interesting test !

          • move the This test chekcs blocks recorvery after a block... comment to the top of the class, and, and define the minimum criteria for running and passing the tests (block replication must be > 1 i assume, maybe some other criteria)
          • Can you consolidate the grep for the ip address into a separate method with a good method name - i always get firghtened when i see random regexes and it distracts from the logic .
          • comment above for loop j < 3
          • just an idea if your familiar w/ vagrant, then would love to see an call to this from the gradle based smoke-tests, then you could confirm that this works on a virtual 3 node hdfs cluster from your laptop ... maybe at a later data i can do this.

          Otherwise, LGTM and very interesting patch and ready to commit, provided you've tested this and confirmed it works perfectly ?

          Show
          jayunit100 jay vyas added a comment - - edited This is an interesting test ! move the This test chekcs blocks recorvery after a block... comment to the top of the class, and, and define the minimum criteria for running and passing the tests (block replication must be > 1 i assume, maybe some other criteria) Can you consolidate the grep for the ip address into a separate method with a good method name - i always get firghtened when i see random regexes and it distracts from the logic . comment above for loop j < 3 just an idea if your familiar w/ vagrant, then would love to see an call to this from the gradle based smoke-tests , then you could confirm that this works on a virtual 3 node hdfs cluster from your laptop ... maybe at a later data i can do this. Otherwise, LGTM and very interesting patch and ready to commit, provided you've tested this and confirmed it works perfectly ?
          Hide
          cos Konstantin Boudnik added a comment -

          Also, it seems that the need to do ssh across DNs as user hdfs will require our deployment recipes to setup keys for password-less logins. I will create a subtask here and have it ready in a bit.

          Show
          cos Konstantin Boudnik added a comment - Also, it seems that the need to do ssh across DNs as user hdfs will require our deployment recipes to setup keys for password-less logins. I will create a subtask here and have it ready in a bit.
          Hide
          cos Konstantin Boudnik added a comment -

          Ok, BIGTOP-1563 is pretty much done. Please make sure to add the following parameters to your ssh invocation -o StrictHostKeyChecking=no -i ~/.ssh/id_hdfsuser
          The former will accept new hosts without confirmation, and the latter will use a special key produced by the puppet recipes.

          Show
          cos Konstantin Boudnik added a comment - Ok, BIGTOP-1563 is pretty much done. Please make sure to add the following parameters to your ssh invocation -o StrictHostKeyChecking=no -i ~/.ssh/id_hdfsuser The former will accept new hosts without confirmation, and the latter will use a special key produced by the puppet recipes.
          Hide
          dasha.boudnik Dasha Boudnik added a comment -

          Hi jay vyas,

          Thanks for the excellent suggestions (I especially admire the one for the IP address grep – after looking at it for a while I stopped noticing how terrifying it looks ).

          I added to the comment that the test needs to be performed on a cluster with at least three datanodes, for the block to be able to recover. Is there actually a need to indicate that block replication needs to be > 1? Correct me if I'm wrong, but I thought that's always the case...

          That's all I can remember as far as requirements go; everything else has been taken care of within the test, I think... (Konstantin Boudnik, thanks for the puppet recipe – let me know if I've missed anything in regard to your comment!)

          And jay vyas – not familiar with vagrant

          Show
          dasha.boudnik Dasha Boudnik added a comment - Hi jay vyas , Thanks for the excellent suggestions (I especially admire the one for the IP address grep – after looking at it for a while I stopped noticing how terrifying it looks ). I added to the comment that the test needs to be performed on a cluster with at least three datanodes, for the block to be able to recover. Is there actually a need to indicate that block replication needs to be > 1? Correct me if I'm wrong, but I thought that's always the case... That's all I can remember as far as requirements go; everything else has been taken care of within the test, I think... ( Konstantin Boudnik , thanks for the puppet recipe – let me know if I've missed anything in regard to your comment!) And jay vyas – not familiar with vagrant
          Hide
          jayunit100 jay vyas added a comment - - edited

          Thanks for looking into it... http://hadoop.apache.org/docs/r2.3.0/hadoop-project-dist/hadoop-hdfs/hdfs-default.xml I think min replication is actually 1... Even though certainly any sane hdfs deploy will have 3 or more? Either way I can do a final review and commit this today I'm pretty sure it's ready to go!

          Show
          jayunit100 jay vyas added a comment - - edited Thanks for looking into it... http://hadoop.apache.org/docs/r2.3.0/hadoop-project-dist/hadoop-hdfs/hdfs-default.xml I think min replication is actually 1... Even though certainly any sane hdfs deploy will have 3 or more? Either way I can do a final review and commit this today I'm pretty sure it's ready to go!
          Hide
          dasha.boudnik Dasha Boudnik added a comment -

          Jay,

          Looking at your link, the default rep number = 3, so I guess the reasoning was the same as yours I caught a mistake in the most recent patch I uploaded, so here's one with the mistake fixed (escape character issue in the grepIP method).

          Thanks!

          Show
          dasha.boudnik Dasha Boudnik added a comment - Jay, Looking at your link, the default rep number = 3, so I guess the reasoning was the same as yours I caught a mistake in the most recent patch I uploaded, so here's one with the mistake fixed (escape character issue in the grepIP method). Thanks!
          Hide
          cos Konstantin Boudnik added a comment -

          Thanks for the update, Dasha Boudnik. two small comments:

          • grepIP isn't a method, but a constant
          • one last thing was about replacing assert with assume (as I've mentioned in here. Assume will simply skip the test with an appropriate message on a single node clusters instead of failing it. Do you think it'd make sense to do it this way?

          Other than then I think the patch is ready to get committed. I will try it today on a cluster spun with new hdfs keys to make sure it all works as expected. If you can make the assume change above I should be able to commit it given that the cluster test will pass.
          Thanks

          Show
          cos Konstantin Boudnik added a comment - Thanks for the update, Dasha Boudnik . two small comments: grepIP isn't a method, but a constant one last thing was about replacing assert with assume (as I've mentioned in here . Assume will simply skip the test with an appropriate message on a single node clusters instead of failing it. Do you think it'd make sense to do it this way? Other than then I think the patch is ready to get committed. I will try it today on a cluster spun with new hdfs keys to make sure it all works as expected. If you can make the assume change above I should be able to commit it given that the cluster test will pass. Thanks
          Hide
          jayunit100 jay vyas added a comment - - edited

          not a big deal, feel free to put it in as is... but, to clarify {{ dfs.namenode.replication.min default=1 Minimal block replication. }} <-- Dasha Boudnik FYI, not a huge deal, but just want to clarify what i meant - the minimum replication is 1, not 3.... so that means that if someone configures HDFS for minimal replication, this test will fail..... Dont worry, its not a big deal but just want to clarify that not all folks running this test will necessarily use the exact bigtop hdfs configuration of 3 , but probably that is generally going to be the case, so its ok

          im okay with it once cos' suggestions above go in, assuming you have tested it and it works !

          Show
          jayunit100 jay vyas added a comment - - edited not a big deal, feel free to put it in as is... but, to clarify {{ dfs.namenode.replication.min default=1 Minimal block replication. }} <-- Dasha Boudnik FYI, not a huge deal, but just want to clarify what i meant - the minimum replication is 1, not 3.... so that means that if someone configures HDFS for minimal replication, this test will fail..... Dont worry, its not a big deal but just want to clarify that not all folks running this test will necessarily use the exact bigtop hdfs configuration of 3 , but probably that is generally going to be the case, so its ok im okay with it once cos' suggestions above go in, assuming you have tested it and it works !
          Hide
          dasha.boudnik Dasha Boudnik added a comment -

          Konstantin Boudnik, sorry, missed it! Ok, so I:

          • put in assume instead of assert
          • took out the constant USERNAME altogether and changed USER_DIR to "/user/hdfs", since in the same comment you've linked to you mentioned that the test will only work under user hdfs. USERNAME isn't used anywhere except when defining USER_DIR, so I think that makes sense?
          • added the user hdfs requirement into the comment above the class
          • Does it maybe also make sense to add an assume for the user? Or is it better to allow the test to fail with the appropriate stdout message if its run under the wrong user?

          jay vyas, didn't occur to me that there may be cases where the configs are changed (oops), thanks again! I added the requirement to the top comment. But (again, I may be very wrong here) won't the test still work if block rep = 2? Or will that mean that the system won't know which of the two blocks to use – the one that was corrupted or the one that was left untouched? If you're triggering specifically the corrupted one for recovery, wouldn't HDFS be able to restore it from the second replica alone?

          Patch attached with mentioned changes, let me know if we should add anything else or change what I've done. Thanks to the both of you!

          Show
          dasha.boudnik Dasha Boudnik added a comment - Konstantin Boudnik , sorry, missed it! Ok, so I: put in assume instead of assert took out the constant USERNAME altogether and changed USER_DIR to "/user/hdfs", since in the same comment you've linked to you mentioned that the test will only work under user hdfs. USERNAME isn't used anywhere except when defining USER_DIR, so I think that makes sense? added the user hdfs requirement into the comment above the class Does it maybe also make sense to add an assume for the user? Or is it better to allow the test to fail with the appropriate stdout message if its run under the wrong user? jay vyas , didn't occur to me that there may be cases where the configs are changed (oops), thanks again! I added the requirement to the top comment. But (again, I may be very wrong here) won't the test still work if block rep = 2? Or will that mean that the system won't know which of the two blocks to use – the one that was corrupted or the one that was left untouched? If you're triggering specifically the corrupted one for recovery, wouldn't HDFS be able to restore it from the second replica alone? Patch attached with mentioned changes, let me know if we should add anything else or change what I've done. Thanks to the both of you!
          Hide
          cos Konstantin Boudnik added a comment -

          Does it maybe also make sense to add an assume for the user?

          I think it's a great idea: let it skip instead of fail. Please do!

          The test should be working fine with the repl == 2 The system will know perfectly well which block is corrupted and which isn't: that's why there are checksums for each block, etc. So, you're right.

          Show
          cos Konstantin Boudnik added a comment - Does it maybe also make sense to add an assume for the user? I think it's a great idea: let it skip instead of fail. Please do! The test should be working fine with the repl == 2 The system will know perfectly well which block is corrupted and which isn't: that's why there are checksums for each block, etc. So, you're right.
          Hide
          dasha.boudnik Dasha Boudnik added a comment -

          Awesome! Added the assume for USERNAME

          Show
          dasha.boudnik Dasha Boudnik added a comment - Awesome! Added the assume for USERNAME
          Hide
          cos Konstantin Boudnik added a comment -

          +1 patch looks good. Thanks! I will commit it in a bit.

          Show
          cos Konstantin Boudnik added a comment - +1 patch looks good. Thanks! I will commit it in a bit.
          Hide
          cos Konstantin Boudnik added a comment - - edited

          Committed and pushed as
          fce1618..e7646c6 HEAD -> master

          Thank you Dasha!

          Show
          cos Konstantin Boudnik added a comment - - edited Committed and pushed as fce1618..e7646c6 HEAD -> master Thank you Dasha!

            People

            • Assignee:
              dasha.boudnik Dasha Boudnik
              Reporter:
              dasha.boudnik Dasha Boudnik
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development