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

Mumak's compile-aspects target weaves aspects even though there are no changes to the Mumak's sources

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.21.0
    • Fix Version/s: 0.21.0
    • Component/s: build
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      This is particularly time consuming and is the bottle neck even for a simple ant build. In the case where no files have been updated in Mumak, there is no reason to recompile sources along with the aspects. compile-aspects should skip this step in these cases.

      1. M1038-1.patch
        2 kB
        Chris Douglas
      2. MAPREDUCE-1038.patch
        2 kB
        Aaron Kimball

        Activity

        Hide
        Hudson added a comment -

        Integrated in Hadoop-Mapreduce-trunk #133 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Mapreduce-trunk/133/)

        Show
        Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #133 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Mapreduce-trunk/133/ )
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Mapreduce-trunk-Commit #105 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Mapreduce-trunk-Commit/105/)
        . Weave Mumak aspects only if related files have changed.
        Contributed by Aaron Kimball

        Show
        Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk-Commit #105 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Mapreduce-trunk-Commit/105/ ) . Weave Mumak aspects only if related files have changed. Contributed by Aaron Kimball
        Hide
        Chris Douglas added a comment -

        +1

        I committed this. Thanks, Aaron!

        Show
        Chris Douglas added a comment - +1 I committed this. Thanks, Aaron!
        Hide
        Aaron Kimball added a comment -

        That seems reasonable. +1.

        Show
        Aaron Kimball added a comment - That seems reasonable. +1.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12422636/M1038-1.patch
        against trunk revision 826767.

        +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 failed contrib unit tests.

        Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/82/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/82/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/82/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/82/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/12422636/M1038-1.patch against trunk revision 826767. +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 failed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/82/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/82/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/82/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/82/console This message is automatically generated.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12422631/MAPREDUCE-1038.patch
        against trunk revision 826767.

        +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 failed contrib unit tests.

        Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/189/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/189/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/189/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/189/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/12422631/MAPREDUCE-1038.patch against trunk revision 826767. +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 failed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/189/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/189/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/189/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/189/console This message is automatically generated.
        Hide
        Hong Tang added a comment -

        +1 on Chris's patch. It only pays the cost of aspect weaving only once. If the dev is only interested in work on core mapred source package, he should probably use "compile-core" instead of "compile".

        Show
        Hong Tang added a comment - +1 on Chris's patch. It only pays the cost of aspect weaving only once. If the dev is only interested in work on core mapred source package, he should probably use "compile-core" instead of "compile".
        Hide
        Aaron Kimball added a comment -

        The most common case for individual developers will not involve using the failure-injection framework or mumak. I feel like shifting the burden on developers to somehow manually disable this very time-consuming process that doesn't help their workflow is the wrong solution here. If anything, I feel like the burden should be on Hudson to run the 'clean' first, by modifying test-patch.sh.

        Show
        Aaron Kimball added a comment - The most common case for individual developers will not involve using the failure-injection framework or mumak. I feel like shifting the burden on developers to somehow manually disable this very time-consuming process that doesn't help their workflow is the wrong solution here. If anything, I feel like the burden should be on Hudson to run the 'clean' first, by modifying test-patch.sh.
        Hide
        Chris Douglas added a comment -
        ant test -Dtestcase=TestConfiguration # Observe that weaving does not occur.
        touch src/java/org/apache/hadoop/mapred/JobClient.java # "change" a MapReduce file
        ant test -Dtestcase=TestConfiguration # Observe that weaving does not occur.
        

        I don't think Hudson runs a clean before testing the patched build. This means that the Mumak tests would be run against the trunk version, rather than the patched version.

        The aspects should be re-weaved if the core classes are modified.

        Show
        Chris Douglas added a comment - ant test -Dtestcase=TestConfiguration # Observe that weaving does not occur. touch src/java/org/apache/hadoop/mapred/JobClient.java # "change" a MapReduce file ant test -Dtestcase=TestConfiguration # Observe that weaving does not occur. I don't think Hudson runs a clean before testing the patched build. This means that the Mumak tests would be run against the trunk version, rather than the patched version. The aspects should be re-weaved if the core classes are modified.
        Hide
        Aaron Kimball added a comment -

        Sorry about the comment-spam; JIRA responded 503 Service Unavailable so I resent the upload. If anyone has delete access, please feel free to delete duplicate comments. I'll remove the extra copies of the patch.

        Show
        Aaron Kimball added a comment - Sorry about the comment-spam; JIRA responded 503 Service Unavailable so I resent the upload. If anyone has delete access, please feel free to delete duplicate comments. I'll remove the extra copies of the patch.
        Hide
        Aaron Kimball added a comment -

        Submitting patch to address the issue.

        Interestingly, Mumak's build.xml already had an uptodate task in it for the weave-aspects. The task was somewhat useless, however, as it:

        • includes all the main MapReduce src/java in its changeset
        • and compared the source file mtimes vs. that of the mapreduce-${version}-mumak.jar file, which isn't generated by the compile target.

        I changed this to ignore the MapReduce source for uptodate-ness, (so you now have to explicitly ant clean if you want aspects rewoven after you change MapReduce but not mumak), and also to use a stamp file generated with the touch target.

        No new unit tests because this is a buildfile tweak. I manually tested it in the following fashion:

        cd $MAPREDUCE_HOME
        ant clean # cleans state
        ant test -Dtestcase=TestConfiguration # compile MapReduce, run a test. Note that weaving occurs.
        ant test -Dtestcase=TestConfiguration # Observe that weaving does not occur.
        touch src/java/org/apache/hadoop/mapred/JobClient.java # "change" a MapReduce file
        ant test -Dtestcase=TestConfiguration # Observe that weaving does not occur.
        touch src/contrib/mumak/src/java/org/apache/hadoop/mapred/SimulatorClock.java # "change" Mumak src
        ant test -Dtestcase=TestConfiguration # Weaving occurs.
        touch src/contrib/mumak/src/java/org/apache/hadoop/mapred/EagerTaskInitializationListenerAspects.aj # change mumak aspects
        ant test -Dtestcase=TestConfiguration # Weaving occurs.
        ant test -Dtestcase=TestConfiguration # No weaving required.
        
        Show
        Aaron Kimball added a comment - Submitting patch to address the issue. Interestingly, Mumak's build.xml already had an uptodate task in it for the weave-aspects. The task was somewhat useless, however, as it: includes all the main MapReduce src/java in its changeset and compared the source file mtimes vs. that of the mapreduce-${version}-mumak.jar file, which isn't generated by the compile target. I changed this to ignore the MapReduce source for uptodate-ness, (so you now have to explicitly ant clean if you want aspects rewoven after you change MapReduce but not mumak), and also to use a stamp file generated with the touch target. No new unit tests because this is a buildfile tweak. I manually tested it in the following fashion: cd $MAPREDUCE_HOME ant clean # cleans state ant test -Dtestcase=TestConfiguration # compile MapReduce, run a test. Note that weaving occurs. ant test -Dtestcase=TestConfiguration # Observe that weaving does not occur. touch src/java/org/apache/hadoop/mapred/JobClient.java # "change" a MapReduce file ant test -Dtestcase=TestConfiguration # Observe that weaving does not occur. touch src/contrib/mumak/src/java/org/apache/hadoop/mapred/SimulatorClock.java # "change" Mumak src ant test -Dtestcase=TestConfiguration # Weaving occurs. touch src/contrib/mumak/src/java/org/apache/hadoop/mapred/EagerTaskInitializationListenerAspects.aj # change mumak aspects ant test -Dtestcase=TestConfiguration # Weaving occurs. ant test -Dtestcase=TestConfiguration # No weaving required.
        Hide
        Aaron Kimball added a comment -

        Submitting patch to address the issue.

        Interestingly, Mumak's build.xml already had an uptodate task in it for the weave-aspects. The task was somewhat useless, however, as it:

        • includes all the main MapReduce src/java in its changeset
        • and compared the source file mtimes vs. that of the mapreduce-${version}-mumak.jar file, which isn't generated by the compile target.

        I changed this to ignore the MapReduce source for uptodate-ness, (so you now have to explicitly ant clean if you want aspects rewoven after you change MapReduce but not mumak), and also to use a stamp file generated with the touch target.

        No new unit tests because this is a buildfile tweak. I manually tested it in the following fashion:

        cd $MAPREDUCE_HOME
        ant clean # cleans state
        ant test -Dtestcase=TestConfiguration # compile MapReduce, run a test. Note that weaving occurs.
        ant test -Dtestcase=TestConfiguration # Observe that weaving does not occur.
        touch src/java/org/apache/hadoop/mapred/JobClient.java # "change" a MapReduce file
        ant test -Dtestcase=TestConfiguration # Observe that weaving does not occur.
        touch src/contrib/mumak/src/java/org/apache/hadoop/mapred/SimulatorClock.java # "change" Mumak src
        ant test -Dtestcase=TestConfiguration # Weaving occurs.
        touch src/contrib/mumak/src/java/org/apache/hadoop/mapred/EagerTaskInitializationListenerAspects.aj # change mumak aspects
        ant test -Dtestcase=TestConfiguration # Weaving occurs.
        ant test -Dtestcase=TestConfiguration # No weaving required.
        
        Show
        Aaron Kimball added a comment - Submitting patch to address the issue. Interestingly, Mumak's build.xml already had an uptodate task in it for the weave-aspects. The task was somewhat useless, however, as it: includes all the main MapReduce src/java in its changeset and compared the source file mtimes vs. that of the mapreduce-${version}-mumak.jar file, which isn't generated by the compile target. I changed this to ignore the MapReduce source for uptodate-ness, (so you now have to explicitly ant clean if you want aspects rewoven after you change MapReduce but not mumak), and also to use a stamp file generated with the touch target. No new unit tests because this is a buildfile tweak. I manually tested it in the following fashion: cd $MAPREDUCE_HOME ant clean # cleans state ant test -Dtestcase=TestConfiguration # compile MapReduce, run a test. Note that weaving occurs. ant test -Dtestcase=TestConfiguration # Observe that weaving does not occur. touch src/java/org/apache/hadoop/mapred/JobClient.java # "change" a MapReduce file ant test -Dtestcase=TestConfiguration # Observe that weaving does not occur. touch src/contrib/mumak/src/java/org/apache/hadoop/mapred/SimulatorClock.java # "change" Mumak src ant test -Dtestcase=TestConfiguration # Weaving occurs. touch src/contrib/mumak/src/java/org/apache/hadoop/mapred/EagerTaskInitializationListenerAspects.aj # change mumak aspects ant test -Dtestcase=TestConfiguration # Weaving occurs. ant test -Dtestcase=TestConfiguration # No weaving required.
        Hide
        Aaron Kimball added a comment -

        Submitting patch to address the issue.

        Interestingly, Mumak's build.xml already had an uptodate task in it for the weave-aspects. The task was somewhat useless, however, as it:

        • includes all the main MapReduce src/java in its changeset
        • and compared the source file mtimes vs. that of the mapreduce-${version}-mumak.jar file, which isn't generated by the compile target.

        I changed this to ignore the MapReduce source for uptodate-ness, (so you now have to explicitly ant clean if you want aspects rewoven after you change MapReduce but not mumak), and also to use a stamp file generated with the touch target.

        No new unit tests because this is a buildfile tweak. I manually tested it in the following fashion:

        cd $MAPREDUCE_HOME
        ant clean # cleans state
        ant test -Dtestcase=TestConfiguration # compile MapReduce, run a test. Note that weaving occurs.
        ant test -Dtestcase=TestConfiguration # Observe that weaving does not occur.
        touch src/java/org/apache/hadoop/mapred/JobClient.java # "change" a MapReduce file
        ant test -Dtestcase=TestConfiguration # Observe that weaving does not occur.
        touch src/contrib/mumak/src/java/org/apache/hadoop/mapred/SimulatorClock.java # "change" Mumak src
        ant test -Dtestcase=TestConfiguration # Weaving occurs.
        touch src/contrib/mumak/src/java/org/apache/hadoop/mapred/EagerTaskInitializationListenerAspects.aj # change mumak aspects
        ant test -Dtestcase=TestConfiguration # Weaving occurs.
        ant test -Dtestcase=TestConfiguration # No weaving required.
        
        Show
        Aaron Kimball added a comment - Submitting patch to address the issue. Interestingly, Mumak's build.xml already had an uptodate task in it for the weave-aspects. The task was somewhat useless, however, as it: includes all the main MapReduce src/java in its changeset and compared the source file mtimes vs. that of the mapreduce-${version}-mumak.jar file, which isn't generated by the compile target. I changed this to ignore the MapReduce source for uptodate-ness, (so you now have to explicitly ant clean if you want aspects rewoven after you change MapReduce but not mumak), and also to use a stamp file generated with the touch target. No new unit tests because this is a buildfile tweak. I manually tested it in the following fashion: cd $MAPREDUCE_HOME ant clean # cleans state ant test -Dtestcase=TestConfiguration # compile MapReduce, run a test. Note that weaving occurs. ant test -Dtestcase=TestConfiguration # Observe that weaving does not occur. touch src/java/org/apache/hadoop/mapred/JobClient.java # "change" a MapReduce file ant test -Dtestcase=TestConfiguration # Observe that weaving does not occur. touch src/contrib/mumak/src/java/org/apache/hadoop/mapred/SimulatorClock.java # "change" Mumak src ant test -Dtestcase=TestConfiguration # Weaving occurs. touch src/contrib/mumak/src/java/org/apache/hadoop/mapred/EagerTaskInitializationListenerAspects.aj # change mumak aspects ant test -Dtestcase=TestConfiguration # Weaving occurs. ant test -Dtestcase=TestConfiguration # No weaving required.
        Hide
        Konstantin Boudnik added a comment -

        This is the way iajc compiler works: fresh recompilation on every occasion. There's the reason why clean compile makes a lot of sense: aspects might affect unupdated java classes and the other way around. You can 'fix' it by using incremental compiler for development purposes and it will be much faster. In ant script you can ride on <uptodate> task

        For more information check antTasks-iajc-uptodate

        Show
        Konstantin Boudnik added a comment - This is the way iajc compiler works: fresh recompilation on every occasion. There's the reason why clean compile makes a lot of sense: aspects might affect unupdated java classes and the other way around. You can 'fix' it by using incremental compiler for development purposes and it will be much faster. In ant script you can ride on <uptodate> task For more information check antTasks-iajc-uptodate

          People

          • Assignee:
            Aaron Kimball
            Reporter:
            Vinod Kumar Vavilapalli
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development