Bigtop
  1. Bigtop
  2. BIGTOP-621

Add test for distcp - intra cluster

    Details

    • Type: Test Test
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.4.0
    • Fix Version/s: 0.6.0
    • Component/s: None
    • Labels:
      None

      Description

      Wrote a test that checks whether the distcp command and its suboptions (using a source file etc.) work without error and confirms expected output is correct.

        Activity

        Sujay Rau created issue -
        Hide
        Sujay Rau added a comment -

        Added patch for my test.

        Show
        Sujay Rau added a comment - Added patch for my test.
        Sujay Rau made changes -
        Field Original Value New Value
        Attachment bigtop-621.patch [ 12531417 ]
        Sujay Rau made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Hide
        Roman Shaposhnik added a comment -

        I don't think there's any reason to introduce helper methods for debugging since the built-in shell class does full logging (provided that loglevel for it is set to trace)

        Show
        Roman Shaposhnik added a comment - I don't think there's any reason to introduce helper methods for debugging since the built-in shell class does full logging (provided that loglevel for it is set to trace)
        Hide
        Sujay Rau added a comment -

        Ok, I will remove them and re-submit.

        Show
        Sujay Rau added a comment - Ok, I will remove them and re-submit.
        Sujay Rau made changes -
        Status Patch Available [ 10002 ] Open [ 1 ]
        Sujay Rau made changes -
        Attachment bigtop-621.patch [ 12531417 ]
        Hide
        Roman Shaposhnik added a comment -

        A couple more comments:

        1. "private static final String USERNAME = sh.exec("whoami").getOut().get(0);" – nor reason to do this since System.getProperty( "user.name" ) would do just fine
        2. "+ execute("cat $HADOOP_CONF_DIR/core-site.xml");" – I would suggest parsing it with either Groovy XML slurper or better yet native Hadoop Configuration class
        3. "execute("hadoop fs -test -e testDistcpInputs");" – I would suggest randomizing the naming of test directories/files
        Show
        Roman Shaposhnik added a comment - A couple more comments: "private static final String USERNAME = sh.exec("whoami").getOut().get(0);" – nor reason to do this since System.getProperty( "user.name" ) would do just fine "+ execute("cat $HADOOP_CONF_DIR/core-site.xml");" – I would suggest parsing it with either Groovy XML slurper or better yet native Hadoop Configuration class "execute("hadoop fs -test -e testDistcpInputs");" – I would suggest randomizing the naming of test directories/files
        Hide
        Sujay Rau added a comment -

        Thanks for the feedback. Yea I just recently learned how the Configuration class works but forgot to go back and fix it. I have I believe 5 tests in total that I'm looking to submit but I'll make all these changes to all of them and post better patches later in the day hopefully.

        Show
        Sujay Rau added a comment - Thanks for the feedback. Yea I just recently learned how the Configuration class works but forgot to go back and fix it. I have I believe 5 tests in total that I'm looking to submit but I'll make all these changes to all of them and post better patches later in the day hopefully.
        Hide
        Sujay Rau added a comment -

        Updated patch by your suggestions.

        Show
        Sujay Rau added a comment - Updated patch by your suggestions.
        Sujay Rau made changes -
        Attachment bigtop-621.patch [ 12531474 ]
        Sujay Rau made changes -
        Assignee Sujay Rau [ sujay.rau ]
        Sujay Rau made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Hide
        Roman Shaposhnik added a comment -

        The patch looks good. One last question – is it really necessary for the shell to be instantiated under the root account? While reviewing the patch I haven't found anything that would require super-user privileges.

        Show
        Roman Shaposhnik added a comment - The patch looks good. One last question – is it really necessary for the shell to be instantiated under the root account? While reviewing the patch I haven't found anything that would require super-user privileges.
        Hide
        Sujay Rau added a comment -

        No you are right. Is there a username I should specify if it is just a regular account - or do I leave it blank? (I will need to quickly change this for one or two of the other patches as well)

        Show
        Sujay Rau added a comment - No you are right. Is there a username I should specify if it is just a regular account - or do I leave it blank? (I will need to quickly change this for one or two of the other patches as well)
        Hide
        Roman Shaposhnik added a comment -

        Just supply a single argument of the name of the shell and you should be fine. Oh and you're right it seems that BIGTOP-625 also suffers from the same issue. Could you, please, update the patch there as well?

        Show
        Roman Shaposhnik added a comment - Just supply a single argument of the name of the shell and you should be fine. Oh and you're right it seems that BIGTOP-625 also suffers from the same issue. Could you, please, update the patch there as well?
        Hide
        Sujay Rau added a comment -

        Fixed shell issue

        Show
        Sujay Rau added a comment - Fixed shell issue
        Sujay Rau made changes -
        Attachment bigtop-621.patch [ 12532753 ]
        Hide
        Roman Shaposhnik added a comment -

        Same comment as before – unless I'm missing something this doesn't seem to have been tested:

          @BeforeClass
          public static void setUp() {
            JarContent.unpackJarContainer(TestDistCpIntra.class, "." , null);
            // get namenode hostname from core-site.xml
            // to manually specify hostname, comment out this section and set namenode to "hdfs://...", and comment out HADOOP_HOME/CONF_DIR Block above
            Configuration conf = new Configuration();
            namenode = conf.get("fs.defaultFS");
            if (sh.getRet() != 0) {
        

        Also, while you're at it, could you please fix things like:

        sh.exec("hadoop distcp " + namenode + "/user/$USERNAME/$testDistcpInputs" + "4/$dcpfile" + "4.txt " + namenode + "/user/$USERNAME/$testDistcpInputs" + "5/$dcpfile" + "5.txt " + namenode + "/user/$USERNAME/$testDistcpOutputs");
        

        to not use '+' and substitute within a Groovy string with a dollar sign instead. It is way easier to review that way.

        Show
        Roman Shaposhnik added a comment - Same comment as before – unless I'm missing something this doesn't seem to have been tested: @BeforeClass public static void setUp() { JarContent.unpackJarContainer(TestDistCpIntra.class, "." , null); // get namenode hostname from core-site.xml // to manually specify hostname, comment out this section and set namenode to "hdfs://...", and comment out HADOOP_HOME/CONF_DIR Block above Configuration conf = new Configuration(); namenode = conf.get("fs.defaultFS"); if (sh.getRet() != 0) { Also, while you're at it, could you please fix things like: sh.exec("hadoop distcp " + namenode + "/user/$USERNAME/$testDistcpInputs" + "4/$dcpfile" + "4.txt " + namenode + "/user/$USERNAME/$testDistcpInputs" + "5/$dcpfile" + "5.txt " + namenode + "/user/$USERNAME/$testDistcpOutputs"); to not use '+' and substitute within a Groovy string with a dollar sign instead. It is way easier to review that way.
        Roman Shaposhnik made changes -
        Status Patch Available [ 10002 ] Open [ 1 ]
        Hide
        Sujay Rau added a comment -

        fixing sh.getRet and package issue

        Show
        Sujay Rau added a comment - fixing sh.getRet and package issue
        Sujay Rau made changes -
        Attachment bigtop-621.patch [ 12532958 ]
        Roman Shaposhnik made changes -
        Fix Version/s 0.5.0 [ 12321865 ]
        Fix Version/s 0.4.0 [ 12318889 ]
        Sujay Rau made changes -
        Attachment bigtop-621.patch [ 12532958 ]
        Sujay Rau made changes -
        Attachment bigtop-621.patch [ 12532753 ]
        Sujay Rau made changes -
        Attachment bigtop-621.patch [ 12531474 ]
        Hide
        Sujay Rau added a comment -

        New patch for distcp test

        Show
        Sujay Rau added a comment - New patch for distcp test
        Sujay Rau made changes -
        Attachment BIGTOP-621.patch [ 12538826 ]
        Sujay Rau made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Roman Shaposhnik made changes -
        Fix Version/s 0.6.0 [ 12323895 ]
        Fix Version/s 0.5.0 [ 12321865 ]
        Hide
        Roman Shaposhnik added a comment -

        +1 and committed!

        Show
        Roman Shaposhnik added a comment - +1 and committed!
        Roman Shaposhnik made changes -
        Status Patch Available [ 10002 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Roman Shaposhnik made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Transition Time In Source Status Execution Times Last Executer Last Execution Date
        Patch Available Patch Available Open Open
        3d 3h 48m 2 Roman Shaposhnik 21/Jun/12 22:43
        Open Open Patch Available Patch Available
        50d 22h 19m 3 Sujay Rau 01/Aug/12 20:51
        Patch Available Patch Available Resolved Resolved
        266d 23h 30m 1 Roman Shaposhnik 25/Apr/13 20:21
        Resolved Resolved Closed Closed
        57d 4h 28m 1 Roman Shaposhnik 22/Jun/13 00:49

          People

          • Assignee:
            Sujay Rau
            Reporter:
            Sujay Rau
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development