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

Add HADOOP_MAPRED_HOME property to common

    Details

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

      Description

      A couple of tests in hadoop need this property to be set, but it's being checked within the tests. Since crunch and datafu also need mapred, let's carry the enforcer out to bigtop-tests-test-execution/common/pom.xml.

      1. BIGTOP-1756.patch
        4 kB
        Dasha Boudnik
      2. BIGTOP-1756.patch
        4 kB
        Dasha Boudnik
      3. BIGTOP-1756.patch
        4 kB
        Dasha Boudnik
      4. BIGTOP-1756.patch
        3 kB
        Dasha Boudnik

        Issue Links

          Activity

          Hide
          dasha.boudnik Dasha Boudnik added a comment -

          Patch tested and attached.

          • HADOOP_MAPRED_HOME property is now to be found in bigtop-tests/test-execution/common/pom.xml
          • HADOOP_MAPRED_HOME, HADOOP_HOME and HADOOP_CONF_DIR are now removed from bigtop-tests/test-execution/smokes/crunch/pom.xml, bigtop-tests/test-execution/smokes/datafu/pom.xml and bigtop-tests/test-execution/smokes/hadoop/pom.xml so there's no redundancy (all three are set in the top-level pom file)
          • Enforcers for HADOOP_MAPRED_HOME were added to these three files separately, since only these tests require this property
          Show
          dasha.boudnik Dasha Boudnik added a comment - Patch tested and attached. HADOOP_MAPRED_HOME property is now to be found in bigtop-tests/test-execution/common/pom.xml HADOOP_MAPRED_HOME, HADOOP_HOME and HADOOP_CONF_DIR are now removed from bigtop-tests/test-execution/smokes/crunch/pom.xml , bigtop-tests/test-execution/smokes/datafu/pom.xml and bigtop-tests/test-execution/smokes/hadoop/pom.xml so there's no redundancy (all three are set in the top-level pom file) Enforcers for HADOOP_MAPRED_HOME were added to these three files separately, since only these tests require this property
          Hide
          cos Konstantin Boudnik added a comment -

          Looks reasonable. +1 assuming the tests are passing with the change.

          Technically speaking

          Enforcers for HADOOP_MAPRED_HOME were added to these three files separately, since only these tests require this property

          Enforcer was only added to dadoop tests, as the crunch and datafu already had them

          Show
          cos Konstantin Boudnik added a comment - Looks reasonable. +1 assuming the tests are passing with the change. Technically speaking Enforcers for HADOOP_MAPRED_HOME were added to these three files separately, since only these tests require this property Enforcer was only added to dadoop tests, as the crunch and datafu already had them
          Hide
          dasha.boudnik Dasha Boudnik added a comment -

          Caught a mistake, fixed in this patch, everything looks to be working.

          Show
          dasha.boudnik Dasha Boudnik added a comment - Caught a mistake, fixed in this patch, everything looks to be working.
          Hide
          dasha.boudnik Dasha Boudnik added a comment -

          Committed and pushed. Thanks!

          Show
          dasha.boudnik Dasha Boudnik added a comment - Committed and pushed. Thanks!
          Hide
          cos Konstantin Boudnik added a comment -

          +1 retrospectively. In general though, if you making a different patch it might be a good idea to have it reviewed again.
          Thanks!

          Show
          cos Konstantin Boudnik added a comment - +1 retrospectively. In general though, if you making a different patch it might be a good idea to have it reviewed again. Thanks!
          Hide
          dasha.boudnik Dasha Boudnik added a comment -

          Yep, of course I had to reopen this... please review, and I'll commit. Thanks!

          Show
          dasha.boudnik Dasha Boudnik added a comment - Yep, of course I had to reopen this... please review, and I'll commit. Thanks!
          Hide
          dasha.boudnik Dasha Boudnik added a comment -

          Actually, hold on, let me check something...

          Show
          dasha.boudnik Dasha Boudnik added a comment - Actually, hold on, let me check something...
          Hide
          dasha.boudnik Dasha Boudnik added a comment - - edited

          A thought occurs... do we really want to check in bigtop-tests/test-execution/smokes/hadoop/pom.xml that HADOOP_MAPRED_HOME is set? Only the two mapreduce tests need it, and the rest won't run because of a property they don't require if we include this enforcer. Should we maybe just keep that part as is and close BIGTOP-1758?

          Show
          dasha.boudnik Dasha Boudnik added a comment - - edited A thought occurs... do we really want to check in bigtop-tests/test-execution/smokes/hadoop/pom.xml that HADOOP_MAPRED_HOME is set? Only the two mapreduce tests need it, and the rest won't run because of a property they don't require if we include this enforcer. Should we maybe just keep that part as is and close BIGTOP-1758 ?
          Hide
          cos Konstantin Boudnik added a comment -

          You mean 'check' == 'enforce'? Perhaps, not actually.

          Show
          cos Konstantin Boudnik added a comment - You mean 'check' == 'enforce'? Perhaps, not actually.
          Hide
          dasha.boudnik Dasha Boudnik added a comment -

          Yeah, enforce, my bad. So we're leaving that enforcer out, yeah?

          Show
          dasha.boudnik Dasha Boudnik added a comment - Yeah, enforce, my bad. So we're leaving that enforcer out, yeah?
          Hide
          cos Konstantin Boudnik added a comment -

          I think it should be fine. Let's have a patch and give it a day so people can review it as well. Thanks!

          Show
          cos Konstantin Boudnik added a comment - I think it should be fine. Let's have a patch and give it a day so people can review it as well. Thanks!
          Hide
          dasha.boudnik Dasha Boudnik added a comment -

          Here's the final (hopefully) version. HADOOP_MAPRED_HOME property is now in common/pom.xml and smokes/hadoop/pom.xml has the enforcer.

          Show
          dasha.boudnik Dasha Boudnik added a comment - Here's the final (hopefully) version. HADOOP_MAPRED_HOME property is now in common/pom.xml and smokes/hadoop/pom.xml has the enforcer.
          Hide
          cos Konstantin Boudnik added a comment -

          Ah, that makes sense really. So the other components will keep on going as they are right now. Thanks!

          +1

          Show
          cos Konstantin Boudnik added a comment - Ah, that makes sense really. So the other components will keep on going as they are right now. Thanks! +1
          Hide
          dasha.boudnik Dasha Boudnik added a comment -

          Committed and pushed. Thank you!

          Show
          dasha.boudnik Dasha Boudnik added a comment - Committed and pushed. Thank you!

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development