Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 0.10.0
    • Fix Version/s: 0.11, 0.10.1
    • Component/s: None
    • Labels:
      None

      Description

      Fix unit tests that fail when compiling with jdk7 as they depend on the order of tests run - TestGFCross, TestJsonLoaderStorage, TestLogToPhyCompiler, TestMRCompiler, TestMergeJoinOuter and TestNewPlanLogToPhyTranslationVisitor. jdk7 returns methods in a different order during reflection.

      1. PIG-2908-trunk-1.patch
        51 kB
        Rohini Palaniswamy
      2. PIG-2908-branch10-1.patch
        50 kB
        Rohini Palaniswamy
      3. PIG-2908-trunk.patch
        45 kB
        Rohini Palaniswamy
      4. PIG-2908-branch10.patch
        47 kB
        Rohini Palaniswamy

        Issue Links

          Activity

          Hide
          Dmitriy V. Ryaboy added a comment -

          Done

          Show
          Dmitriy V. Ryaboy added a comment - Done
          Hide
          Rohini Palaniswamy added a comment -

          Yes. OrderedJunitRunner

          Show
          Rohini Palaniswamy added a comment - Yes. OrderedJunitRunner
          Hide
          Dmitriy V. Ryaboy added a comment -

          test/org/apache/pig/test/junit ?

          Show
          Dmitriy V. Ryaboy added a comment - test/org/apache/pig/test/junit ?
          Hide
          Rohini Palaniswamy added a comment -

          You had missed the svn add step. Can you commit the missing new test file?

          Show
          Rohini Palaniswamy added a comment - You had missed the svn add step. Can you commit the missing new test file?
          Hide
          Rohini Palaniswamy added a comment -

          Thanks Dmitriy for committing this one.

          Show
          Rohini Palaniswamy added a comment - Thanks Dmitriy for committing this one.
          Hide
          Dmitriy V. Ryaboy added a comment -

          Thanks for seeing this important but non-flashy ticket to completion, Rohini!

          Show
          Dmitriy V. Ryaboy added a comment - Thanks for seeing this important but non-flashy ticket to completion, Rohini!
          Hide
          Dmitriy V. Ryaboy added a comment -

          Committed to 0.11, 0.10, and trunk branches.

          Show
          Dmitriy V. Ryaboy added a comment - Committed to 0.11, 0.10, and trunk branches.
          Hide
          Rohini Palaniswamy added a comment -

          Dmitriy,
          We have a mandate to upgrade to jdk7 by end of Nov and we are going to have 0.10.1 around for a while before 0.11 gets stabilized and we internally release 0.11. That's why need it on 0.10 too.

          Show
          Rohini Palaniswamy added a comment - Dmitriy, We have a mandate to upgrade to jdk7 by end of Nov and we are going to have 0.10.1 around for a while before 0.11 gets stabilized and we internally release 0.11. That's why need it on 0.10 too.
          Hide
          Dmitriy V. Ryaboy added a comment -

          +1 for trunk and 0.11

          Why do we need to apply this to 0.10?

          Show
          Dmitriy V. Ryaboy added a comment - +1 for trunk and 0.11 Why do we need to apply this to 0.10?
          Hide
          Rohini Palaniswamy added a comment -
          Show
          Rohini Palaniswamy added a comment - Updated https://reviews.apache.org/r/7176/
          Hide
          Rohini Palaniswamy added a comment -

          Cancelling patch. Have few more test failures to fix after running on RHEL6+JDK7 combination. Will update the patch with fix for new test failures soon.

          Show
          Rohini Palaniswamy added a comment - Cancelling patch. Have few more test failures to fix after running on RHEL6+JDK7 combination. Will update the patch with fix for new test failures soon.
          Hide
          Rohini Palaniswamy added a comment -

          Review:
          https://reviews.apache.org/r/7176/

          Steps to Run Tests:

          for test in

          {TestLogToPhyCompiler,TestMRCompiler,TestMapSideCogroup,TestMergeJoinOuter,TestGFCross,TestJsonLoaderStorage,TestNewPlanLogToPhyTranslationVisitor}; do ant clean test Dtestcase=$test -Dtest.output=true -logfile PIG-2908$test-H20.log; done
          cd contrib/piggybank/java
          for test in {TestAvroStorage,TestCSVExcelStorage}; do ant clean test Dtestcase=$test -Dtest.output=true -logfile PIG-2908$test-H20.log; done
          cd -
          for test in {TestLogToPhyCompiler,TestMRCompiler,TestMapSideCogroup,TestMergeJoinOuter,TestGFCross,TestJsonLoaderStorage,TestNewPlanLogToPhyTranslationVisitor}

          ; do ant clean test Dhadoopversion=23 -Dtestcase=$test -Dtest.output=true -logfile PIG-2908$test-H23.log; done
          grep BUILD PIG-2908*
          cd contrib/piggybank/java
          for test in

          {TestAvroStorage,TestCSVExcelStorage}

          ; do ant clean test Dhadoopversion=23 -Dtestcase=$test -Dtest.output=true -logfile PIG-2908$test-H23.log; done
          grep BUILD PIG-2908*

          For trunk, tests depending on MiniCluster will fail in H23. That requires PIG-2791 to be checked in.

          Checkin Steps:
          There is a new java class and svn add needs to be done.
          svn add test/org/apache/pig/test/junit

          There is a overlap between this patch and PIG-2405 for one test - TestNewPlanLogToPhyTranslationVisitor class. Fang has solved it by changing it to a LinkedHashMap in MapReduceOper.java and OperatorPlan.java. I have just modified tests to sort the entrySet results and then assert. If LinkedHashMap approach is preferred, I can remove TestNewPlanLogToPhyTranslationVisitor from this patch. But in general, would prefer not changing code for tests to work.

          Show
          Rohini Palaniswamy added a comment - Review: https://reviews.apache.org/r/7176/ Steps to Run Tests: for test in {TestLogToPhyCompiler,TestMRCompiler,TestMapSideCogroup,TestMergeJoinOuter,TestGFCross,TestJsonLoaderStorage,TestNewPlanLogToPhyTranslationVisitor}; do ant clean test Dtestcase=$test -Dtest.output=true -logfile PIG-2908 $test-H20.log; done cd contrib/piggybank/java for test in {TestAvroStorage,TestCSVExcelStorage}; do ant clean test Dtestcase=$test -Dtest.output=true -logfile PIG-2908 $test-H20.log; done cd - for test in {TestLogToPhyCompiler,TestMRCompiler,TestMapSideCogroup,TestMergeJoinOuter,TestGFCross,TestJsonLoaderStorage,TestNewPlanLogToPhyTranslationVisitor} ; do ant clean test Dhadoopversion=23 -Dtestcase=$test -Dtest.output=true -logfile PIG-2908 $test-H23.log; done grep BUILD PIG-2908 * cd contrib/piggybank/java for test in {TestAvroStorage,TestCSVExcelStorage} ; do ant clean test Dhadoopversion=23 -Dtestcase=$test -Dtest.output=true -logfile PIG-2908 $test-H23.log; done grep BUILD PIG-2908 * For trunk, tests depending on MiniCluster will fail in H23. That requires PIG-2791 to be checked in. Checkin Steps: There is a new java class and svn add needs to be done. svn add test/org/apache/pig/test/junit There is a overlap between this patch and PIG-2405 for one test - TestNewPlanLogToPhyTranslationVisitor class. Fang has solved it by changing it to a LinkedHashMap in MapReduceOper.java and OperatorPlan.java. I have just modified tests to sort the entrySet results and then assert. If LinkedHashMap approach is preferred, I can remove TestNewPlanLogToPhyTranslationVisitor from this patch. But in general, would prefer not changing code for tests to work.
          Hide
          Rohini Palaniswamy added a comment -

          That would be ideal. TestGFCross was easy to fix to run independent of the order. TestJsonLoaderStorage - first test does store, the second and third do load from data stored in first test. So I am merging the tests into one as junit does not have dependsOnMethods option like in TestNG. TestLogToPhyCompiler, TestMRCompiler depend on golden files and the order of tests is important. TestMergeJoinOuter and TestNewPlanLogToPhyTranslationVisitor, I think they could be fixed to be independent. The plans are different when the order of tests is different. I need to spend some more time to track that one down and fix them to succeed independent of the order.

          Show
          Rohini Palaniswamy added a comment - That would be ideal. TestGFCross was easy to fix to run independent of the order. TestJsonLoaderStorage - first test does store, the second and third do load from data stored in first test. So I am merging the tests into one as junit does not have dependsOnMethods option like in TestNG. TestLogToPhyCompiler, TestMRCompiler depend on golden files and the order of tests is important. TestMergeJoinOuter and TestNewPlanLogToPhyTranslationVisitor, I think they could be fixed to be independent. The plans are different when the order of tests is different. I need to spend some more time to track that one down and fix them to succeed independent of the order.
          Hide
          Dmitriy V. Ryaboy added a comment -

          I haven't looked at the tests in question, but it sounds like the right solution is to make them succeed independent of the order in which tests are executed?

          Show
          Dmitriy V. Ryaboy added a comment - I haven't looked at the tests in question, but it sounds like the right solution is to make them succeed independent of the order in which tests are executed?
          Hide
          Rohini Palaniswamy added a comment -

          Have a patch for the tests mentioned above. Fixed tests where it was easy and for others wrote a OrderedJUnitTestRunner. But found that with new runs, more test failures came up because the order of methods seem to be random with jdk7 with different runs. Will do a few more runs next week and collect all the failures before putting up the patch. e2e tests all pass with jdk7 32 bit. So except for unit test failures pig is good with jdk7.

          Show
          Rohini Palaniswamy added a comment - Have a patch for the tests mentioned above. Fixed tests where it was easy and for others wrote a OrderedJUnitTestRunner. But found that with new runs, more test failures came up because the order of methods seem to be random with jdk7 with different runs. Will do a few more runs next week and collect all the failures before putting up the patch. e2e tests all pass with jdk7 32 bit. So except for unit test failures pig is good with jdk7.
          Hide
          Dmitriy V. Ryaboy added a comment -

          Rohini, are you planning to work on this ticket, or just raising this as an issue?
          JDK7 compatibility is pretty important, it'd be great if you knocked this one out.

          Show
          Dmitriy V. Ryaboy added a comment - Rohini, are you planning to work on this ticket, or just raising this as an issue? JDK7 compatibility is pretty important, it'd be great if you knocked this one out.

            People

            • Assignee:
              Rohini Palaniswamy
              Reporter:
              Rohini Palaniswamy
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development