Hadoop Map/Reduce
  1. Hadoop Map/Reduce
  2. MAPREDUCE-1113

mumak compiles aspects even if skip.contrib is true

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: build, contrib/mumak
    • Labels:
      None

      Description

      The compile-aspects task in mumak's build.xml runs regardless of the skip.contrib property. Momentarily uploading a patch to fix this.

        Activity

        Hide
        Todd Lipcon added a comment -

        This patch simply uses a Condition task to skip aspect building when skip.contrib is set.

        Test plan: build without -Dskip.contrib=1, mumak aspects should compile. Build with -Dskip.contrib=1 and you should not see this task run.

        Show
        Todd Lipcon added a comment - This patch simply uses a Condition task to skip aspect building when skip.contrib is set. Test plan: build without -Dskip.contrib=1, mumak aspects should compile. Build with -Dskip.contrib=1 and you should not see this task run.
        Hide
        Chris Douglas added a comment -

        This is a duplicate of MAPREDUCE-1038, but the patch looks OK. I'm not fluent in this ant task, but it might make sense to also limit the uptodate glob to the classes actually instrumented for Mumak.

        Show
        Chris Douglas added a comment - This is a duplicate of MAPREDUCE-1038 , but the patch looks OK. I'm not fluent in this ant task, but it might make sense to also limit the uptodate glob to the classes actually instrumented for Mumak.
        Hide
        Todd Lipcon added a comment -

        This isn't quite a duplicate of MAPREDUCE-1038. That one was about reinstrumenting even in the case when the build didn't change. This patch is about not compiling when skip.contrib is true. The uptodate task itself isn't something I wrote - I just left that be since I think that's the domain of 1038.

        Show
        Todd Lipcon added a comment - This isn't quite a duplicate of MAPREDUCE-1038 . That one was about reinstrumenting even in the case when the build didn't change. This patch is about not compiling when skip.contrib is true. The uptodate task itself isn't something I wrote - I just left that be since I think that's the domain of 1038.
        Hide
        Chris Douglas added a comment -

        Fair enough. +1 on the current patch, assuming Hudson comes back clean

        Show
        Chris Douglas added a comment - Fair enough. +1 on the current patch, assuming Hudson comes back clean
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12422180/mapreduce-1113.txt
        against trunk revision 825083.

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 findbugs. The patch does not introduce any new Findbugs warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed core unit tests.

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/76/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/76/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/76/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/76/console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12422180/mapreduce-1113.txt against trunk revision 825083. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/76/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/76/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/76/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/76/console This message is automatically generated.
        Hide
        Vinod Kumar Vavilapalli added a comment -

        Actually, there are other targets in contrib that still don't respect skip.contrib. `ant binary -Dskip.contrib` fails in fact!

        The following targets need unless="skip.contrib":

        • data_join: jar, jar-examples
        • mumak: package, check.aspects
        • streaming: jar, test, test-unix
        • vaidya: init, package

        Care do this in this issue? Or another one?

        Show
        Vinod Kumar Vavilapalli added a comment - Actually, there are other targets in contrib that still don't respect skip.contrib. `ant binary -Dskip.contrib` fails in fact! The following targets need unless="skip.contrib": data_join: jar, jar-examples mumak: package, check.aspects streaming: jar, test, test-unix vaidya: init, package Care do this in this issue? Or another one?
        Hide
        Todd Lipcon added a comment -

        Sure, I'll upload a new patch this afternoon or evening. Thanks Vinod.

        Show
        Todd Lipcon added a comment - Sure, I'll upload a new patch this afternoon or evening. Thanks Vinod.
        Hide
        Todd Lipcon added a comment -

        Just had another thought... if skip.contrib is set, why do we subant into the contrib directories at all? Seems an unless="skip.contrib" in the toplevel build file would be a lot less of a headache, and speeds up the build significantly when working on non-contrib stuff.

        Show
        Todd Lipcon added a comment - Just had another thought... if skip.contrib is set, why do we subant into the contrib directories at all? Seems an unless="skip.contrib" in the toplevel build file would be a lot less of a headache, and speeds up the build significantly when working on non-contrib stuff.
        Hide
        Vinod Kumar Vavilapalli added a comment -

        Just had another thought... if skip.contrib is set, why do we subant into the contrib directories at all?

        +1 for doing it in and only in the main build file.

        Show
        Vinod Kumar Vavilapalli added a comment - Just had another thought... if skip.contrib is set, why do we subant into the contrib directories at all? +1 for doing it in and only in the main build file.
        Hide
        Todd Lipcon added a comment -

        Is it a requirement that "ant binary" work with skip.contrib set? There would need to be a fair amount of build refactoring there to get it to work, I fear. One thing that would make it a lot easier would be to utilize some tasks from ant-contrib (http://ant-contrib.sourceforge.net/ant-contrib/manual/tasks/). Does anyone have objections to using the contrib ant tasks to clean up the build a bit?

        Show
        Todd Lipcon added a comment - Is it a requirement that "ant binary" work with skip.contrib set? There would need to be a fair amount of build refactoring there to get it to work, I fear. One thing that would make it a lot easier would be to utilize some tasks from ant-contrib ( http://ant-contrib.sourceforge.net/ant-contrib/manual/tasks/ ). Does anyone have objections to using the contrib ant tasks to clean up the build a bit?
        Hide
        Chris Douglas added a comment -

        I wouldn't oppose adding the ant-contrib dep in principle and binary tarballs without contrib would be great, but this is quickly expanding outside its original scope. If this is going into 0.21, then a light touch would be strongly preferred. Perhaps a separate issue for refactoring the build would be appropriate? Either that, or this can be appropriated/renamed for that purpose while MAPREDUCE-1038 can track the excessive Mumak aspect generation.

        Show
        Chris Douglas added a comment - I wouldn't oppose adding the ant-contrib dep in principle and binary tarballs without contrib would be great, but this is quickly expanding outside its original scope. If this is going into 0.21, then a light touch would be strongly preferred. Perhaps a separate issue for refactoring the build would be appropriate? Either that, or this can be appropriated/renamed for that purpose while MAPREDUCE-1038 can track the excessive Mumak aspect generation.
        Hide
        Chris Douglas added a comment -

        Cancelling patch under discussion.

        If the issue has changed to fix the broader, "skip.contrib doesn't" problem, then the title/description should be updated.

        Show
        Chris Douglas added a comment - Cancelling patch under discussion. If the issue has changed to fix the broader, "skip.contrib doesn't" problem, then the title/description should be updated.
        Hide
        Vinod Kumar Vavilapalli added a comment -

        Todd, can we decide on something here?

        I am assuming you've originally opened this issue to reduce the overall build time and not just in the mumak project. In which case, I am leaning towards fixing the skip.contrib problem here itself (changing the title of this issue as Chris suggests). It's really getting frustrating now to wait for nearly 5 minutes for a simple test run, something's got to be done with our build.

        Show
        Vinod Kumar Vavilapalli added a comment - Todd, can we decide on something here? I am assuming you've originally opened this issue to reduce the overall build time and not just in the mumak project. In which case, I am leaning towards fixing the skip.contrib problem here itself (changing the title of this issue as Chris suggests). It's really getting frustrating now to wait for nearly 5 minutes for a simple test run, something's got to be done with our build.
        Hide
        Todd Lipcon added a comment -

        Hey Vinod.

        Yes, you're correct - I don't make use of mumak, so the issue was just about build time.

        I don't have much time in the next week or two to work on this, so if you'd like to grab it from me, please feel free. Otherwise I'll try to pick it back up as soon as I can. Thanks!

        (FYI, I've found that "ant run-test-mapred" is a much faster way to iterate on tests than the full "ant test")

        Show
        Todd Lipcon added a comment - Hey Vinod. Yes, you're correct - I don't make use of mumak, so the issue was just about build time. I don't have much time in the next week or two to work on this, so if you'd like to grab it from me, please feel free. Otherwise I'll try to pick it back up as soon as I can. Thanks! (FYI, I've found that "ant run-test-mapred" is a much faster way to iterate on tests than the full "ant test")

          People

          • Assignee:
            Todd Lipcon
            Reporter:
            Todd Lipcon
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:

              Development