HBase
  1. HBase
  2. HBASE-6234

Move simple KeyValue tests to hbase-common module

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.95.2
    • Fix Version/s: 0.95.0
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      TestKeyValue, LoadTestKVGenerator, and TestLoadTestKVGenerator should move up to hbase-common. This brings MD5Hash up as a dependency as well.

      To play well with Maven as discussed in HBASE-6162, I moved LoadTestKVGenerator from the src/test folder to src/main folder so that tests in other modules can see it. A couple other files' import statements were affected by this.

      1. HBASE-6234-v1.patch
        61 kB
        Matt Corgan
      2. HBASE-6234-v2.patch
        67 kB
        Matt Corgan

        Activity

        Hide
        stack added a comment -

        Marking closed.

        Show
        stack added a comment - Marking closed.
        Hide
        Nicolas Liochon added a comment -

        the single line itself

        It main purpose was that it's executed within the JVM on hadoop-qa. That's how we found that our ulimit were not taking into account on hadoop-qa. I now use it to see that the test has actually started & finished in the logs. There's no impact on test coverage if we don't have it. I would be lazy here I think: don't spend time to remove it when it works, don't spend time to make it work when it doesn't. We fix/remove it when (if) we need it.

        hbase-common all run in the one JVM

        It would work short term imho. If we could, it would be better not to have this as a prerequiste, we can imagine having a lot of stuff there, with maybe long running tests or tests playing with the environment (typical stuff: using defaultEdgeEnvironment to manipulate the time needs to be done in isolation as it's a static). In the same area, if we share code around thread management, with extreme/error cases (too many threads & so on), it's better to do it on another jvm. I guess there are other example

        Leakage tools

        Yes, really the main advantage is that it runs on hadoop qa as well.

        Show
        Nicolas Liochon added a comment - the single line itself It main purpose was that it's executed within the JVM on hadoop-qa. That's how we found that our ulimit were not taking into account on hadoop-qa. I now use it to see that the test has actually started & finished in the logs. There's no impact on test coverage if we don't have it. I would be lazy here I think: don't spend time to remove it when it works, don't spend time to make it work when it doesn't. We fix/remove it when (if) we need it. hbase-common all run in the one JVM It would work short term imho. If we could, it would be better not to have this as a prerequiste, we can imagine having a lot of stuff there, with maybe long running tests or tests playing with the environment (typical stuff: using defaultEdgeEnvironment to manipulate the time needs to be done in isolation as it's a static). In the same area, if we share code around thread management, with extreme/error cases (too many threads & so on), it's better to do it on another jvm. I guess there are other example Leakage tools Yes, really the main advantage is that it runs on hadoop qa as well.
        Hide
        Matt Corgan added a comment -

        btw - i am all for having that kind of test monitoring code thoroughly used in all the tests. I justified dropping it in this case because it was the only thing standing in the way of an otherwise clean module refactoring... i think it's much safer to keep the refactorings coming steadily in small patches than to try to yank apart the whole code base at once. Seemed like a good place to snip some dependencies without major collateral damage. We should already be thinking of how to clean up the damage (as Jesse already is in HBASE-6702).

        Along the lines of what stack says above, the tests in hbase-common should generally (maybe without exception?) not use external resources like the minicluster or the filesystem. If we find ourselves wanting to put minicluster tests in hbase-common, i would encourage us to further split the hbase-common module so that we retain some central module that holds the crown-jewels of hbase (KeyValue, Bytes, comparators, etc).

        Show
        Matt Corgan added a comment - btw - i am all for having that kind of test monitoring code thoroughly used in all the tests. I justified dropping it in this case because it was the only thing standing in the way of an otherwise clean module refactoring... i think it's much safer to keep the refactorings coming steadily in small patches than to try to yank apart the whole code base at once. Seemed like a good place to snip some dependencies without major collateral damage. We should already be thinking of how to clean up the damage (as Jesse already is in HBASE-6702 ). Along the lines of what stack says above, the tests in hbase-common should generally (maybe without exception?) not use external resources like the minicluster or the filesystem. If we find ourselves wanting to put minicluster tests in hbase-common, i would encourage us to further split the hbase-common module so that we retain some central module that holds the crown-jewels of hbase (KeyValue, Bytes, comparators, etc).
        Hide
        stack added a comment -

        Nicolas Liochon What if hbase-common did not include these lines but hbase-server and all its dependencies did. We could still do thread and file descriptor counts right? But it'd just be for the tests that include these lines? So, we would be losing all-test coverage? Could we make it so all tests in hbase-common all run in the one JVM and require that they are all 'small' as per your categorization? Would this help contain the damage hbase-common unit tests could do? i.e. if in their own jvm, thread leakage or file descriptor counts would impinge little on other tests run... and if a problem, because all in one jvm, should be easy enough debugging leakage (you have tools already that could be used here that are other than the lines added to unit test files?).

        Show
        stack added a comment - Nicolas Liochon What if hbase-common did not include these lines but hbase-server and all its dependencies did. We could still do thread and file descriptor counts right? But it'd just be for the tests that include these lines? So, we would be losing all-test coverage? Could we make it so all tests in hbase-common all run in the one JVM and require that they are all 'small' as per your categorization? Would this help contain the damage hbase-common unit tests could do? i.e. if in their own jvm, thread leakage or file descriptor counts would impinge little on other tests run... and if a problem, because all in one jvm, should be easy enough debugging leakage (you have tools already that could be used here that are other than the lines added to unit test files?).
        Hide
        Jesse Yates added a comment -

        File HBASE-6702 for this issue.

        Show
        Jesse Yates added a comment - File HBASE-6702 for this issue.
        Hide
        Nicolas Liochon added a comment -

        I would not have solved the hadoop qa issues (HBASE-4965) without it. This said, I haven't used much since (but sometimes I do), so we can as well consider dropping it. Both options (improve / remove) are fine for me if "keep as it is" is not possible.

        Show
        Nicolas Liochon added a comment - I would not have solved the hadoop qa issues ( HBASE-4965 ) without it. This said, I haven't used much since (but sometimes I do), so we can as well consider dropping it. Both options (improve / remove) are fine for me if "keep as it is" is not possible.
        Hide
        Jesse Yates added a comment -

        I had the same issue moving bytes across. Its not elegant and we should consider doing a ticket where we break out the resource checker into the simple stuff (threads, pools, etc) and then a more complete one that digs into the hconnections, etc. that we use in hbase-server and its dependencies.

        Show
        Jesse Yates added a comment - I had the same issue moving bytes across. Its not elegant and we should consider doing a ticket where we break out the resource checker into the simple stuff (threads, pools, etc) and then a more complete one that digs into the hconnections, etc. that we use in hbase-server and its dependencies.
        Hide
        Nicolas Liochon added a comment -

        Do you think it's ok to drop it for now NK so we can pull up the other stuff? I don't really have an opinion since i don't know what it does (any better than a guess). I will submit the patch without it since i already made it.

        +1 to drop it if it's not trivial to keep it.

        Show
        Nicolas Liochon added a comment - Do you think it's ok to drop it for now NK so we can pull up the other stuff? I don't really have an opinion since i don't know what it does (any better than a guess). I will submit the patch without it since i already made it. +1 to drop it if it's not trivial to keep it.
        Hide
        stack added a comment -

        Committed to trunk. Thanks for the patch Matt.

        Show
        stack added a comment - Committed to trunk. Thanks for the patch Matt.
        Hide
        stack added a comment -

        Two tests hung:

        pynchon:trunk stack$ ./dev-support/findHangingTest.sh https://builds.apache.org/job/PreCommit-HBASE-Build/2743//console
          % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                         Dload  Upload   Total   Spent    Left  Speed
        100  159k    0  159k    0     0   166k      0 --:--:-- --:--:-- --:--:--  232k
        Hanging test: Running org.apache.hadoop.hbase.zookeeper.TestZooKeeperNodeTracker
        Hanging test: Running org.apache.hadoop.hbase.regionserver.
        

        I tried them locally and passed. So committing...

        Show
        stack added a comment - Two tests hung: pynchon:trunk stack$ ./dev-support/findHangingTest.sh https: //builds.apache.org/job/PreCommit-HBASE-Build/2743//console % Total % Received % Xferd Average Speed Time Time Time Current Dload Upload Total Spent Left Speed 100 159k 0 159k 0 0 166k 0 --:--:-- --:--:-- --:--:-- 232k Hanging test: Running org.apache.hadoop.hbase.zookeeper.TestZooKeeperNodeTracker Hanging test: Running org.apache.hadoop.hbase.regionserver. I tried them locally and passed. So committing...
        Hide
        Matt Corgan added a comment -

        Scary output as usual, but i don't think it explicitly failed any tests. Jenkins says 12 skipped.

        Show
        Matt Corgan added a comment - Scary output as usual, but i don't think it explicitly failed any tests. Jenkins says 12 skipped.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12543188/HBASE-6234-v2.patch
        against trunk revision .

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

        +1 tests included. The patch appears to include 21 new or modified tests.

        +1 hadoop2.0. The patch compiles against the hadoop 2.0 profile.

        -1 javadoc. The javadoc tool appears to have generated 110 warning messages.

        -1 javac. The applied patch generated 5 javac compiler warnings (more than the trunk's current 4 warnings).

        -1 findbugs. The patch appears to introduce 11 new Findbugs (version 1.3.9) warnings.

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

        -1 core tests. The patch failed these unit tests:

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/2743//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/2743//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/2743//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/2743//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/2743//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/2743//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/2743//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/12543188/HBASE-6234-v2.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 21 new or modified tests. +1 hadoop2.0. The patch compiles against the hadoop 2.0 profile. -1 javadoc. The javadoc tool appears to have generated 110 warning messages. -1 javac. The applied patch generated 5 javac compiler warnings (more than the trunk's current 4 warnings). -1 findbugs. The patch appears to introduce 11 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests: Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/2743//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/2743//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/2743//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/2743//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/2743//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/2743//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/2743//console This message is automatically generated.
        Hide
        Matt Corgan added a comment -

        Sorry - it's been a couple months so i forgot the reasoning behind these things. The problem is that ResourceCheckerJUnitRule depends on ResourceChecker, which depends on HConnectionTestingUtility, which depends on a bunch of things like HRegionInfo... quick guess is that we should eventually cut the cord between ResourceChecker and HConnectionTestingUtility, but i'll have to punt that to another jira.

        Do you think it's ok to drop it for now NK so we can pull up the other stuff? I don't really have an opinion since i don't know what it does (any better than a guess). I will submit the patch without it since i already made it.

        Show
        Matt Corgan added a comment - Sorry - it's been a couple months so i forgot the reasoning behind these things. The problem is that ResourceCheckerJUnitRule depends on ResourceChecker, which depends on HConnectionTestingUtility, which depends on a bunch of things like HRegionInfo... quick guess is that we should eventually cut the cord between ResourceChecker and HConnectionTestingUtility, but i'll have to punt that to another jira. Do you think it's ok to drop it for now NK so we can pull up the other stuff? I don't really have an opinion since i don't know what it does (any better than a guess). I will submit the patch without it since i already made it.
        Hide
        Nicolas Liochon added a comment -

        I would recommend to keep it, it does no harm and it makes it consistent with the rest.
        It was used a few months to sort out hadoop-qa misconfiguration + some issues with the tests. Hopefully we will never need it again, but who knows

        Show
        Nicolas Liochon added a comment - I would recommend to keep it, it does no harm and it makes it consistent with the rest. It was used a few months to sort out hadoop-qa misconfiguration + some issues with the tests. Hopefully we will never need it again, but who knows
        Hide
        Matt Corgan added a comment -

        i think i can remove this from TestKeyValue since it's moving out of the big test suite?

          @org.junit.Rule
          public org.apache.hadoop.hbase.ResourceCheckerJUnitRule cu =
            new org.apache.hadoop.hbase.ResourceCheckerJUnitRule();
        
        Show
        Matt Corgan added a comment - i think i can remove this from TestKeyValue since it's moving out of the big test suite? @org.junit.Rule public org.apache.hadoop.hbase.ResourceCheckerJUnitRule cu = new org.apache.hadoop.hbase.ResourceCheckerJUnitRule();
        Hide
        Matt Corgan added a comment -

        i think i need to rebase it. will look into it

        Show
        Matt Corgan added a comment - i think i need to rebase it. will look into it
        Hide
        Michael Drzal added a comment -

        Matt Corgan can you hit the submit patch button to get this going? It looks like it is stalled. Seems like a good fix to me.

        Show
        Michael Drzal added a comment - Matt Corgan can you hit the submit patch button to get this going? It looks like it is stalled. Seems like a good fix to me.
        Hide
        Jesse Yates added a comment -

        Looks reasonable to me. If it tests fine, I'm +1. Also, I think this is a good paradigm to follow for moving things up.

        Show
        Jesse Yates added a comment - Looks reasonable to me. If it tests fine, I'm +1. Also, I think this is a good paradigm to follow for moving things up.
        Hide
        Matt Corgan added a comment -

        Attaching v1 patch matching the issue description.

        Show
        Matt Corgan added a comment - Attaching v1 patch matching the issue description.

          People

          • Assignee:
            Matt Corgan
            Reporter:
            Matt Corgan
          • Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development