HBase
  1. HBase
  2. HBASE-6162

Move KeyValue 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

      • pull KeyValue up to hbase-common module

      This is part of the modularization strategy in HBASE-5977, and is specifically necessary to modularize HBASE-4676.

      also brings these classes to hbase-common:

      • ClassSize, HeapSize
      • HTestConst
      • TestKeyValue, KeyValueTestUtil
      • LoadTestKVGenerator, TestLoadTestKVGenerator
      • MD5Hash

      moves a trivial constant (HRegionInfo.DELIMITER) from HRegionInfo to HConstants

      1. HBASE-6162-v1.patch
        278 kB
        Matt Corgan
      2. HBASE-6162-v2.patch
        308 kB
        Matt Corgan
      3. HBASE-6162-v3.patch
        107 kB
        Matt Corgan
      4. HBASE-6162-v4.patch
        209 kB
        Matt Corgan
      5. HBASE-6162-v5.patch
        209 kB
        Matt Corgan

        Issue Links

          Activity

          stack made changes -
          Status Resolved [ 5 ] Closed [ 6 ]
          Hide
          stack added a comment -

          Marking closed.

          Show
          stack added a comment - Marking closed.
          stack made changes -
          Fix Version/s 0.98.0 [ 12323143 ]
          stack made changes -
          Fix Version/s 0.98.0 [ 12323143 ]
          stack made changes -
          Fix Version/s 0.95.0 [ 12324094 ]
          Fix Version/s 0.96.0 [ 12320040 ]
          Hide
          Matt Corgan added a comment -

          Jesse - thanks. I understand now and agree that's better.

          I created HBASE-6234 with a patch reflecting my latest interpretation of the discussion.

          Show
          Matt Corgan added a comment - Jesse - thanks. I understand now and agree that's better. I created HBASE-6234 with a patch reflecting my latest interpretation of the discussion.
          Hide
          Jesse Yates added a comment -

          Jesse and I are proposing that we put the tests that would go in hbase-common/src/test/java into hbase-common/src/main/java so they becomes visible downstream.

          Not quite. I'd rather see the test utilities go into src/main, for downstream visibility, but keep the tests themselves in src/test. I don't think we actually do any test heirarchy, so this shouldn't be a problem. If we do have some inheritance, then those classes will need to be together (same module), but this should be rare.

          hbase-tests might then have the testing utils in src/main and the tests in src/tests

          FWIW, you can hack the locations to look for test classes with the <testResources\> tag in the pom. Its not pretty, but does provide a route to putting test classes in src/main/test/

          Show
          Jesse Yates added a comment - Jesse and I are proposing that we put the tests that would go in hbase-common/src/test/java into hbase-common/src/main/java so they becomes visible downstream. Not quite. I'd rather see the test utilities go into src/main, for downstream visibility, but keep the tests themselves in src/test. I don't think we actually do any test heirarchy, so this shouldn't be a problem. If we do have some inheritance, then those classes will need to be together (same module), but this should be rare. hbase-tests might then have the testing utils in src/main and the tests in src/tests FWIW, you can hack the locations to look for test classes with the <testResources\> tag in the pom. Its not pretty, but does provide a route to putting test classes in src/main/test/
          Hide
          Ted Yu added a comment -

          I think we should put more efforts into fixing maven so that we don't devise transitional fixes in hbase code base.

          Show
          Ted Yu added a comment - I think we should put more efforts into fixing maven so that we don't devise transitional fixes in hbase code base.
          Hide
          Matt Corgan added a comment -

          You don't think it ok just adding new stuff at o.a.hbase? Maybe when we go to HBase 2.0?

          I'm generally in favor of starting to move towards the long term goal as soon as possible, but thought i'd throw out a warning that it's a pretty ugly transitionary state =)

          (Or are you trying to fix tests in a later module have dependency on code in src/test/java in an earlier one?)

          yep - the hbase-common/src/test/java classes are unfortunately not visible to classes in hbase-server/src/test/java. We want things in hbase-common/src/test/java to be visible by downstream tests, for example LoadTestKVGenerator is pretty fundamental and can be useful for tests in all modules.

          Jesse and I are proposing that we put the tests that would go in hbase-common/src/test/java into hbase-common/src/main/java so they becomes visible downstream. I like this solution (at least until maven gets fixed), but now the problem is that "mvn -pl hbase-common test" doesn't run tests if they're in src/main/java

          Show
          Matt Corgan added a comment - You don't think it ok just adding new stuff at o.a.hbase? Maybe when we go to HBase 2.0? I'm generally in favor of starting to move towards the long term goal as soon as possible, but thought i'd throw out a warning that it's a pretty ugly transitionary state =) (Or are you trying to fix tests in a later module have dependency on code in src/test/java in an earlier one?) yep - the hbase-common/src/test/java classes are unfortunately not visible to classes in hbase-server/src/test/java. We want things in hbase-common/src/test/java to be visible by downstream tests, for example LoadTestKVGenerator is pretty fundamental and can be useful for tests in all modules. Jesse and I are proposing that we put the tests that would go in hbase-common/src/test/java into hbase-common/src/main/java so they becomes visible downstream. I like this solution (at least until maven gets fixed), but now the problem is that "mvn -pl hbase-common test" doesn't run tests if they're in src/main/java
          Hide
          stack added a comment -

          Jesse - have you by chance tried getting tests to run when the test classes are in src/main/java?

          Why you ask Matt? Because if we have an hbase-tests, you think the tests should go into src/main/java rather than into src/test/java?

          ...hbase-common/src/test/java, but are foiled by maven again.

          If you put them there, aren't they run as part of the mvn test goal? (Or are you trying to fix tests in a later module have dependency on code in src/test/java in an earlier one?)

          Looking at it made me think we should drop the hadoop package for everything all at once and not move things piecemeal.

          That'll break the world unfortunately. We could do it internally but our exterior APIs have the hadoop on them.... so we would have to do call through from old APIs to the new ones. You don't think it ok just adding new stuff at o.a.hbase? Maybe when we go to HBase 2.0?

          It would be nice to pull the tests up somehow to help us untangle things... what do you think is the best approach now?

          Yes... but regards the tests you included in earlier versions of the patch, these were dirty because they had dependencies used by tests that were left behind in hbase-server? I'd think the test needs teasing apart? Or would that not work for the said test? Could the dependencies the test relies on be moved into src/main/java..... tools?

          Good on you Matt.

          Show
          stack added a comment - Jesse - have you by chance tried getting tests to run when the test classes are in src/main/java? Why you ask Matt? Because if we have an hbase-tests, you think the tests should go into src/main/java rather than into src/test/java? ...hbase-common/src/test/java, but are foiled by maven again. If you put them there, aren't they run as part of the mvn test goal? (Or are you trying to fix tests in a later module have dependency on code in src/test/java in an earlier one?) Looking at it made me think we should drop the hadoop package for everything all at once and not move things piecemeal. That'll break the world unfortunately. We could do it internally but our exterior APIs have the hadoop on them.... so we would have to do call through from old APIs to the new ones. You don't think it ok just adding new stuff at o.a.hbase? Maybe when we go to HBase 2.0? It would be nice to pull the tests up somehow to help us untangle things... what do you think is the best approach now? Yes... but regards the tests you included in earlier versions of the patch, these were dirty because they had dependencies used by tests that were left behind in hbase-server? I'd think the test needs teasing apart? Or would that not work for the said test? Could the dependencies the test relies on be moved into src/main/java..... tools? Good on you Matt.
          Hide
          Matt Corgan added a comment -

          Jesse - have you by chance tried getting tests to run when the test classes are in src/main/java? I don't think they do by default... any idea how?

          And what did you think of putting the tests in org.apache.hadoop.hbase.test rather than scattering test packages throughout - do you have a preference there?

          I'll make a patch for the tests if we can figure out how to make sure they run.

          Show
          Matt Corgan added a comment - Jesse - have you by chance tried getting tests to run when the test classes are in src/main/java? I don't think they do by default... any idea how? And what did you think of putting the tests in org.apache.hadoop.hbase.test rather than scattering test packages throughout - do you have a preference there? I'll make a patch for the tests if we can figure out how to make sure they run.
          Hide
          Jesse Yates added a comment -

          -1 on an hbase-common-tests module until its really necessary. Generally, we should be able to split tests into two groups: (1) strict unit tests, (2) tests using the mini-cluster.

          Those with the mini-cluster should be somewhat refactored to test much of the same functionality, but with more mocking and smaller pieces.

          The bigger tests might need to be pulled into an hbase-tests module or hbase-

          {module}tests module that depends on an hbase-test module. hbase-test would contain the mini-cluster, LargeTest, etc. and also just depends on the all hbase{module}

          modules (so hbase-test depends on hbase-common, hbase-server, etc).

          Show
          Jesse Yates added a comment - -1 on an hbase-common-tests module until its really necessary. Generally, we should be able to split tests into two groups: (1) strict unit tests, (2) tests using the mini-cluster. Those with the mini-cluster should be somewhat refactored to test much of the same functionality, but with more mocking and smaller pieces. The bigger tests might need to be pulled into an hbase-tests module or hbase- {module} tests module that depends on an hbase-test module. hbase-test would contain the mini-cluster, LargeTest, etc. and also just depends on the all hbase {module} modules (so hbase-test depends on hbase-common, hbase-server, etc).
          Hide
          Matt Corgan added a comment -

          No worries stack - i just wasn't sure of the best approach for the tests so decided to punt for now. I think we all agree they would be best in hbase-common/src/test/java, but are foiled by maven again.

          I actually tried putting them in hbase-common/src/main/java/org/apache/hbase/test (removed hadoop package), but it made for a very confusing package tree in eclipse since the non-test classes were still in org/apache/hadoop/hbase. Looking at it made me think we should drop the hadoop package for everything all at once and not move things peicemeal.

          It would be nice to pull the tests up somehow to help us untangle things... what do you think is the best approach now? I can live with new modules, funny package trees, etc. Just need to pick our poison i guess. I unfortunately don't know enough about maven to even make a suggestion. You thinking an hbase-common-tests module is the best approach for now?

          Show
          Matt Corgan added a comment - No worries stack - i just wasn't sure of the best approach for the tests so decided to punt for now. I think we all agree they would be best in hbase-common/src/test/java, but are foiled by maven again. I actually tried putting them in hbase-common/src/main/java/org/apache/hbase/test (removed hadoop package), but it made for a very confusing package tree in eclipse since the non-test classes were still in org/apache/hadoop/hbase. Looking at it made me think we should drop the hadoop package for everything all at once and not move things peicemeal. It would be nice to pull the tests up somehow to help us untangle things... what do you think is the best approach now? I can live with new modules, funny package trees, etc. Just need to pick our poison i guess. I unfortunately don't know enough about maven to even make a suggestion. You thinking an hbase-common-tests module is the best approach for now?
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK-on-Hadoop-2.0.0 #54 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-2.0.0/54/)
          HBASE-6162 Move KeyValue to hbase-common module (Revision 1350378)

          Result = FAILURE
          stack :
          Files :

          • /hbase/trunk/hbase-common/src/main/java/org/apache/hadoop/hbase/HConstants.java
          • /hbase/trunk/hbase-common/src/main/java/org/apache/hadoop/hbase/KeyValue.java
          • /hbase/trunk/hbase-common/src/main/java/org/apache/hadoop/hbase/io/HeapSize.java
          • /hbase/trunk/hbase-common/src/main/java/org/apache/hadoop/hbase/util/ClassSize.java
          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/HRegionInfo.java
          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/KeyValue.java
          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/catalog/MetaReader.java
          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/io/HeapSize.java
          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/GetClosestRowBeforeTracker.java
          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/util/ClassSize.java
          • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestGetClosestAtOrBefore.java
          Show
          Hudson added a comment - Integrated in HBase-TRUNK-on-Hadoop-2.0.0 #54 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-2.0.0/54/ ) HBASE-6162 Move KeyValue to hbase-common module (Revision 1350378) Result = FAILURE stack : Files : /hbase/trunk/hbase-common/src/main/java/org/apache/hadoop/hbase/HConstants.java /hbase/trunk/hbase-common/src/main/java/org/apache/hadoop/hbase/KeyValue.java /hbase/trunk/hbase-common/src/main/java/org/apache/hadoop/hbase/io/HeapSize.java /hbase/trunk/hbase-common/src/main/java/org/apache/hadoop/hbase/util/ClassSize.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/HRegionInfo.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/KeyValue.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/catalog/MetaReader.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/io/HeapSize.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/GetClosestRowBeforeTracker.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/util/ClassSize.java /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestGetClosestAtOrBefore.java
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK #3025 (See https://builds.apache.org/job/HBase-TRUNK/3025/)
          HBASE-6162 Move KeyValue to hbase-common module (Revision 1350378)

          Result = SUCCESS
          stack :
          Files :

          • /hbase/trunk/hbase-common/src/main/java/org/apache/hadoop/hbase/HConstants.java
          • /hbase/trunk/hbase-common/src/main/java/org/apache/hadoop/hbase/KeyValue.java
          • /hbase/trunk/hbase-common/src/main/java/org/apache/hadoop/hbase/io/HeapSize.java
          • /hbase/trunk/hbase-common/src/main/java/org/apache/hadoop/hbase/util/ClassSize.java
          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/HRegionInfo.java
          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/KeyValue.java
          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/catalog/MetaReader.java
          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/io/HeapSize.java
          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/GetClosestRowBeforeTracker.java
          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/util/ClassSize.java
          • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestGetClosestAtOrBefore.java
          Show
          Hudson added a comment - Integrated in HBase-TRUNK #3025 (See https://builds.apache.org/job/HBase-TRUNK/3025/ ) HBASE-6162 Move KeyValue to hbase-common module (Revision 1350378) Result = SUCCESS stack : Files : /hbase/trunk/hbase-common/src/main/java/org/apache/hadoop/hbase/HConstants.java /hbase/trunk/hbase-common/src/main/java/org/apache/hadoop/hbase/KeyValue.java /hbase/trunk/hbase-common/src/main/java/org/apache/hadoop/hbase/io/HeapSize.java /hbase/trunk/hbase-common/src/main/java/org/apache/hadoop/hbase/util/ClassSize.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/HRegionInfo.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/KeyValue.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/catalog/MetaReader.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/io/HeapSize.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/GetClosestRowBeforeTracker.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/util/ClassSize.java /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestGetClosestAtOrBefore.java
          stack made changes -
          Status Patch Available [ 10002 ] Resolved [ 5 ]
          Hadoop Flags Reviewed [ 10343 ]
          Resolution Fixed [ 1 ]
          Hide
          stack added a comment -

          Committed to trunk. Thanks Matt.

          I wasn't mad about the machinations where we try to fake the push test dependencies into src/main. I was thinking that rather we should refactor the unit test so its module scoped? (At least for the stuff that doesn't need minihbasecluster, etc.)

          Show
          stack added a comment - Committed to trunk. Thanks Matt. I wasn't mad about the machinations where we try to fake the push test dependencies into src/main. I was thinking that rather we should refactor the unit test so its module scoped? (At least for the stuff that doesn't need minihbasecluster, etc.)
          Hide
          Hadoop QA added a comment -

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

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

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

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

          +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 appears to introduce 6 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/2169//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/2169//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/2169//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/2169//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/12532046/HBASE-6162-v5.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 hadoop2.0. The patch compiles against the hadoop 2.0 profile. +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 appears to introduce 6 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/2169//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/2169//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/2169//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/2169//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/12532043/HBASE-6162-v4.patch
          against trunk revision .

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

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

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

          +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 appears to introduce 6 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:
          org.apache.hadoop.hbase.master.TestSplitLogManager

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/2168//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/2168//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/2168//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/2168//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/12532043/HBASE-6162-v4.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 hadoop2.0. The patch compiles against the hadoop 2.0 profile. +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 appears to introduce 6 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: org.apache.hadoop.hbase.master.TestSplitLogManager Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/2168//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/2168//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/2168//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/2168//console This message is automatically generated.
          Matt Corgan made changes -
          Attachment HBASE-6162-v5.patch [ 12532046 ]
          Hide
          Matt Corgan added a comment -

          v5 patch removes the apache header years in HeapSize, ClassSize, and KeyValue

          Show
          Matt Corgan added a comment - v5 patch removes the apache header years in HeapSize, ClassSize, and KeyValue
          Hide
          Ted Yu added a comment -

          Patch looks good.

          +++ b/hbase-common/src/main/java/org/apache/hadoop/hbase/KeyValue.java
          @@ -0,0 +1,2563 @@
          +/**
          + * Copyright 2009 The Apache Software Foundation
          

          Can year be removed from the above file ?
          Same for HeapSize.java and ClassSize.java

          Show
          Ted Yu added a comment - Patch looks good. +++ b/hbase-common/src/main/java/org/apache/hadoop/hbase/KeyValue.java @@ -0,0 +1,2563 @@ +/** + * Copyright 2009 The Apache Software Foundation Can year be removed from the above file ? Same for HeapSize.java and ClassSize.java
          Matt Corgan made changes -
          Attachment HBASE-6162-v4.patch [ 12532043 ]
          Hide
          Matt Corgan added a comment -

          shoot - made the patch before committing so git diff didn't get the "new" files. i am no good at git

          hopefully this v4 will work. i tested applying it to latest trunk

          Show
          Matt Corgan added a comment - shoot - made the patch before committing so git diff didn't get the "new" files. i am no good at git hopefully this v4 will work. i tested applying it to latest trunk
          Hide
          Ted Yu added a comment - - edited

          From https://builds.apache.org/job/PreCommit-HBASE-Build/2166/console, looks like there might be compilation issue.

          I don't see ClassSize.java added to hbase-common module.

          Show
          Ted Yu added a comment - - edited From https://builds.apache.org/job/PreCommit-HBASE-Build/2166/console , looks like there might be compilation issue. I don't see ClassSize.java added to hbase-common module.
          Matt Corgan made changes -
          Attachment HBASE-6162-v3.patch [ 12532038 ]
          Hide
          Matt Corgan added a comment -

          Maybe we want to avoid messing with the tests for now. Here is a v3 patch that does the minimum possible to move KeyValue to hbase-common. It brings HeapSize and ClassSize with it, and it moves a couple constants around.

          After this jira I'll move the DataBlockEncoding base classes and then add the prefix-trie module.

          Show
          Matt Corgan added a comment - Maybe we want to avoid messing with the tests for now. Here is a v3 patch that does the minimum possible to move KeyValue to hbase-common. It brings HeapSize and ClassSize with it, and it moves a couple constants around. After this jira I'll move the DataBlockEncoding base classes and then add the prefix-trie module.
          Matt Corgan made changes -
          Attachment HBASE-6162-v2.patch [ 12531587 ]
          Hide
          Matt Corgan added a comment -

          Attaching v2 patch which moves the test classes into individual .test packages in src/main/java. Not sure of the best place to put them, but had to choose something.

          running "mvn compile -pl hbase-common -am" now gives the below error though. Anyone know why it can't find junit... maybe only the src/test/java folders have the dependency on it?

          [ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:2.0.2:compile (default-compile) on project hbase-common: Compilation failure: Compilation failure:
          [ERROR] /home/mcorgan/hadoop/hbase/hbase-common/src/main/java/org/apache/hadoop/hbase/test/TestKeyValue.java:[28,22] package junit.framework does not exist
          Show
          Matt Corgan added a comment - Attaching v2 patch which moves the test classes into individual .test packages in src/main/java. Not sure of the best place to put them, but had to choose something. running "mvn compile -pl hbase-common -am" now gives the below error though. Anyone know why it can't find junit... maybe only the src/test/java folders have the dependency on it? [ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:2.0.2:compile ( default -compile) on project hbase-common: Compilation failure: Compilation failure: [ERROR] /home/mcorgan/hadoop/hbase/hbase-common/src/main/java/org/apache/hadoop/hbase/test/TestKeyValue.java:[28,22] package junit.framework does not exist
          Hide
          stack added a comment -

          @Matt Makes sense. I'd have seen that if I looked closer. Maybe can undo the classes in hbase-server depending on this utility. I hear you on making an hbase-test.

          Show
          stack added a comment - @Matt Makes sense. I'd have seen that if I looked closer. Maybe can undo the classes in hbase-server depending on this utility. I hear you on making an hbase-test.
          Hide
          Matt Corgan added a comment -

          Why is KeyValueTestUtil going into hbase-common

          Just the line of thinking that core, fundamental classes and their tests should sit in hbase-common. I guess we could wait until things are explicitly needed to pull them up, but the question i was asking myself was "if i'm writing hbase from scratch and sketching out the necessary classes and modules, which module would KeyValueTestUtil go into?". KeyValueTestUtil seemed to pass the "fundamental" test. I suspect we'll need some other simple classes like this up in hbase-common when we add the other modules, but i guess we can wait until then to move them.

          Thought it might help to move them earlier to simplify hbase-server.

          An hbase-test that had test util under src/main would be OTT?

          doesn't offend me, although if we're ok modifying the package names, it might be easier to have less open projects in eclipse (less modules). either way, we could consider it a temporary solution until maven is fixed.

          Show
          Matt Corgan added a comment - Why is KeyValueTestUtil going into hbase-common Just the line of thinking that core, fundamental classes and their tests should sit in hbase-common. I guess we could wait until things are explicitly needed to pull them up, but the question i was asking myself was "if i'm writing hbase from scratch and sketching out the necessary classes and modules, which module would KeyValueTestUtil go into?". KeyValueTestUtil seemed to pass the "fundamental" test. I suspect we'll need some other simple classes like this up in hbase-common when we add the other modules, but i guess we can wait until then to move them. Thought it might help to move them earlier to simplify hbase-server. An hbase-test that had test util under src/main would be OTT? doesn't offend me, although if we're ok modifying the package names, it might be easier to have less open projects in eclipse (less modules). either way, we could consider it a temporary solution until maven is fixed.
          Hide
          stack added a comment -

          Yeah, lets undo these dependencies. TestConst we agreed shouldn't be moved. That'll take care of a few of these issues. Why is KeyValueTestUtil going into hbase-common when it seems it only used in hbase-server?

          An hbase-test that had test util under src/main would be OTT?

          Show
          stack added a comment - Yeah, lets undo these dependencies. TestConst we agreed shouldn't be moved. That'll take care of a few of these issues. Why is KeyValueTestUtil going into hbase-common when it seems it only used in hbase-server? An hbase-test that had test util under src/main would be OTT?
          Hide
          Matt Corgan added a comment -

          I think we are finally starting to grasp the problem Jesse =). That does suck.

          Is there an alternative 1b: put the tests in hbase-common/src/main/java/org/apache/hbase/test/

          {package}

          ? At hotpads we even put a lot of tests at the bottom of the class they're testing. I wouldn't care between 1a and 1b, but perhaps 1b is more similar to the current system. For a package like hbase-common where it's simple unit tests, anything should work ok.

          One problem we'll run into with option 1a or 1b is that some of the tests actually have a hidden dependency on the package they're in because they call package-private constructors and methods on classes in their src/main package. I personally don't like relying on package-private scope, especially across src folders, so in my opinion option 1 would help us find and eliminate those hidden dependencies.

          Show
          Matt Corgan added a comment - I think we are finally starting to grasp the problem Jesse =). That does suck. Is there an alternative 1b: put the tests in hbase-common/src/main/java/org/apache/hbase/test/ {package} ? At hotpads we even put a lot of tests at the bottom of the class they're testing. I wouldn't care between 1a and 1b, but perhaps 1b is more similar to the current system. For a package like hbase-common where it's simple unit tests, anything should work ok. One problem we'll run into with option 1a or 1b is that some of the tests actually have a hidden dependency on the package they're in because they call package-private constructors and methods on classes in their src/main package. I personally don't like relying on package-private scope, especially across src folders, so in my opinion option 1 would help us find and eliminate those hidden dependencies.
          Hide
          Jesse Yates added a comment -

          @stack - yup, that's what I was going on about in the comments in the poms. Essentially, it doesn't build the tests jar for commons until the package phase, which doesn't get reached if you just do a "test-compile" (or just test). Therefore, you aren't going to see the classes in hbase-common/src/test in hbase-server. There are three ways around this.

          (1) Move the test classes to needed by other modules to the hbase-common/src/main/java/...

          {package}

          /test. Basically, just append test to their package names. This separates out the test oriented class from the regular ones, but still makes it easy to get them when running tests
          (2) Always run tests by doing mvn package. This sucks because you need to do a lot more work for testing.
          (3) Fix that maven issue. A ton of work and then everyone needs to use the patched maven. Probably the worst option.

          I'm in favor of just doing (1). Its pretty simple, doesn't clutter the code too much, and doesn't require a ton of work.

          Show
          Jesse Yates added a comment - @stack - yup, that's what I was going on about in the comments in the poms. Essentially, it doesn't build the tests jar for commons until the package phase, which doesn't get reached if you just do a "test-compile" (or just test). Therefore, you aren't going to see the classes in hbase-common/src/test in hbase-server. There are three ways around this. (1) Move the test classes to needed by other modules to the hbase-common/src/main/java/... {package} /test. Basically, just append test to their package names. This separates out the test oriented class from the regular ones, but still makes it easy to get them when running tests (2) Always run tests by doing mvn package. This sucks because you need to do a lot more work for testing. (3) Fix that maven issue. A ton of work and then everyone needs to use the patched maven. Probably the worst option. I'm in favor of just doing (1). Its pretty simple, doesn't clutter the code too much, and doesn't require a ton of work.
          Hide
          stack added a comment -

          When hbase-server compiles its tests, it does not include hbase-common jar so the compile fails. Jesse?

          Show
          stack added a comment - When hbase-server compiles its tests, it does not include hbase-common jar so the compile fails. Jesse?
          Hide
          stack added a comment -

          Ok. Lets leave comparators as they are for now. Can break them out in another jira.

          Locally, tests seem to do the right thing w/ patch applied:

          .....
          [INFO] --- maven-surefire-plugin:2.12-TRUNK-HBASE-2:test (default-test) @ hbase-common ---
          [INFO] Surefire report directory: /Users/stack/checkouts/trunk/hbase-common/target/surefire-reports
          [INFO] Using configured provider org.apache.maven.surefire.junitcore.JUnitCoreProvider
          
          -------------------------------------------------------
           T E S T S
          -------------------------------------------------------
          Running org.apache.hadoop.hbase.TestKeyValue
          Tests run: 14, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.056 sec
          Running org.apache.hadoop.hbase.util.TestBytes
          Tests run: 13, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.04 sec
          Running org.apache.hadoop.hbase.util.TestLoadTestKVGenerator
          Tests run: 3, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 1.626 sec
          
          Results :
          
          Tests run: 30, Failures: 0, Errors: 0, Skipped: 0
          
          ...
          
          
          Show
          stack added a comment - Ok. Lets leave comparators as they are for now. Can break them out in another jira. Locally, tests seem to do the right thing w/ patch applied: ..... [INFO] --- maven-surefire-plugin:2.12-TRUNK-HBASE-2:test ( default -test) @ hbase-common --- [INFO] Surefire report directory: /Users/stack/checkouts/trunk/hbase-common/target/surefire-reports [INFO] Using configured provider org.apache.maven.surefire.junitcore.JUnitCoreProvider ------------------------------------------------------- T E S T S ------------------------------------------------------- Running org.apache.hadoop.hbase.TestKeyValue Tests run: 14, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.056 sec Running org.apache.hadoop.hbase.util.TestBytes Tests run: 13, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.04 sec Running org.apache.hadoop.hbase.util.TestLoadTestKVGenerator Tests run: 3, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 1.626 sec Results : Tests run: 30, Failures: 0, Errors: 0, Skipped: 0 ...
          Hide
          Ted Yu added a comment -

          The patch produces a lot of compilation errors:

          [ERROR] /home/hduser/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/util/TestProcessBasedCluster.java:[29,30] cannot find symbol
          [ERROR] symbol  : class HTestConst
          [ERROR] location: package org.apache.hadoop.hbase
          [ERROR] 
          [ERROR] /home/hduser/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/util/MultiThreadedWriter.java:[121,11] cannot find symbol
          [ERROR] symbol  : variable LoadTestKVGenerator
          [ERROR] location: class org.apache.hadoop.hbase.util.MultiThreadedWriter
          [ERROR] 
          [ERROR] /home/hduser/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/util/MultiThreadedWriter.java:[128,58] cannot find symbol
          [ERROR] symbol  : class LoadTestKVGenerator
          [ERROR] location: class org.apache.hadoop.hbase.util.MultiThreadedWriter.HBaseWriterThread
          [ERROR] 
          [ERROR] /home/hduser/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/util/MultiThreadedWriter.java:[191,24] cannot find symbol
          [ERROR] symbol  : variable LoadTestKVGenerator
          [ERROR] location: class org.apache.hadoop.hbase.util.MultiThreadedWriter.HBaseWriterThread
          [ERROR] 
          [ERROR] /home/hduser/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/util/MultiThreadedReader.java:[226,10] cannot find symbol
          [ERROR] symbol  : variable LoadTestKVGenerator
          [ERROR] location: class org.apache.hadoop.hbase.util.MultiThreadedReader.HBaseReaderThread
          [ERROR] 
          [ERROR] /home/hduser/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/util/MultiThreadedReader.java:[282,17] cannot find symbol
          [ERROR] symbol  : variable LoadTestKVGenerator
          [ERROR] location: class org.apache.hadoop.hbase.util.MultiThreadedReader.HBaseReaderThread
          [ERROR] 
          [ERROR] /home/hduser/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/filter/TestColumnRangeFilter.java:[192,24] cannot find symbol
          [ERROR] symbol  : variable KeyValueTestUtil
          [ERROR] location: class org.apache.hadoop.hbase.filter.TestColumnRangeFilter
          [ERROR] 
          [ERROR] /home/hduser/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreScanner.java:[80,8] cannot find symbol
          [ERROR] symbol  : variable KeyValueTestUtil
          [ERROR] location: class org.apache.hadoop.hbase.regionserver.TestStoreScanner
          
          Show
          Ted Yu added a comment - The patch produces a lot of compilation errors: [ERROR] /home/hduser/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/util/TestProcessBasedCluster.java:[29,30] cannot find symbol [ERROR] symbol : class HTestConst [ERROR] location: package org.apache.hadoop.hbase [ERROR] [ERROR] /home/hduser/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/util/MultiThreadedWriter.java:[121,11] cannot find symbol [ERROR] symbol : variable LoadTestKVGenerator [ERROR] location: class org.apache.hadoop.hbase.util.MultiThreadedWriter [ERROR] [ERROR] /home/hduser/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/util/MultiThreadedWriter.java:[128,58] cannot find symbol [ERROR] symbol : class LoadTestKVGenerator [ERROR] location: class org.apache.hadoop.hbase.util.MultiThreadedWriter.HBaseWriterThread [ERROR] [ERROR] /home/hduser/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/util/MultiThreadedWriter.java:[191,24] cannot find symbol [ERROR] symbol : variable LoadTestKVGenerator [ERROR] location: class org.apache.hadoop.hbase.util.MultiThreadedWriter.HBaseWriterThread [ERROR] [ERROR] /home/hduser/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/util/MultiThreadedReader.java:[226,10] cannot find symbol [ERROR] symbol : variable LoadTestKVGenerator [ERROR] location: class org.apache.hadoop.hbase.util.MultiThreadedReader.HBaseReaderThread [ERROR] [ERROR] /home/hduser/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/util/MultiThreadedReader.java:[282,17] cannot find symbol [ERROR] symbol : variable LoadTestKVGenerator [ERROR] location: class org.apache.hadoop.hbase.util.MultiThreadedReader.HBaseReaderThread [ERROR] [ERROR] /home/hduser/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/filter/TestColumnRangeFilter.java:[192,24] cannot find symbol [ERROR] symbol : variable KeyValueTestUtil [ERROR] location: class org.apache.hadoop.hbase.filter.TestColumnRangeFilter [ERROR] [ERROR] /home/hduser/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreScanner.java:[80,8] cannot find symbol [ERROR] symbol : variable KeyValueTestUtil [ERROR] location: class org.apache.hadoop.hbase.regionserver.TestStoreScanner
          Hide
          Ted Yu added a comment -
          Show
          Ted Yu added a comment - Hadoop QA didn't run any test. See https://builds.apache.org/job/PreCommit-HBASE-Build/2108/console .
          Hide
          Matt Corgan added a comment -

          Stack - based on your recent maven wrangling, does it look like i did the right thing with TestKeyValue? I removed the @SmallTests annotation and removed these lines at the bottom:

          @org.junit.Rule  	
          public org.apache.hadoop.hbase.ResourceCheckerJUnitRule cu = new org.apache.hadoop.hbase.ResourceCheckerJUnitRule();
          

          Just want to make sure the test still gets run by the automated test suite. I think Jesse did the same thing with TestBytes.

          Show
          Matt Corgan added a comment - Stack - based on your recent maven wrangling, does it look like i did the right thing with TestKeyValue? I removed the @SmallTests annotation and removed these lines at the bottom: @org.junit.Rule public org.apache.hadoop.hbase.ResourceCheckerJUnitRule cu = new org.apache.hadoop.hbase.ResourceCheckerJUnitRule(); Just want to make sure the test still gets run by the automated test suite. I think Jesse did the same thing with TestBytes.
          Hide
          Matt Corgan added a comment -

          I started going down that path, but i believe it turned out to be an even bigger refactoring than is obvious at first. I also think it's ok for the common module to contain code that only the server knows about as long as it is small, unit-testable, and has few dependencies. This kind of code can be tightly verified with unit tests and actually helps to de-clutter the bigger server module. I figured these two comparators were just some simple byte operations (no exceptions, threads, IO, reflection, etc), so they are simple enough to be in common.

          Another way to look at it is that the higher up the module tree you go (towards util/common), the more bullet-proof the code needs to be, and I thought these two comparators should be as bullet-proof as can be. We'd like the server module to be bullet-proof, but it will never be as rock-solid as common. And by the time you get down to the web or avro modules or other 3rd party contributions things can get pretty hairy.

          It's a discussion that will keep popping up though. I'm happy to give it another stab the other way... just wanted to explain some of the thinking.

          Show
          Matt Corgan added a comment - I started going down that path, but i believe it turned out to be an even bigger refactoring than is obvious at first. I also think it's ok for the common module to contain code that only the server knows about as long as it is small, unit-testable, and has few dependencies. This kind of code can be tightly verified with unit tests and actually helps to de-clutter the bigger server module. I figured these two comparators were just some simple byte operations (no exceptions, threads, IO, reflection, etc), so they are simple enough to be in common. Another way to look at it is that the higher up the module tree you go (towards util/common), the more bullet-proof the code needs to be, and I thought these two comparators should be as bullet-proof as can be. We'd like the server module to be bullet-proof, but it will never be as rock-solid as common. And by the time you get down to the web or avro modules or other 3rd party contributions things can get pretty hairy. It's a discussion that will keep popping up though. I'm happy to give it another stab the other way... just wanted to explain some of the thinking.
          Hide
          stack added a comment -

          That would make some sense Lars.

          Show
          stack added a comment - That would make some sense Lars.
          Hide
          Lars Hofhansl added a comment -

          Can we remove all the comparators from KeyValue and leave the ROOT/META/REGION ones in the server module?
          Looks like that would be a bigger refactor, but maybe a necessary one...?

          Show
          Lars Hofhansl added a comment - Can we remove all the comparators from KeyValue and leave the ROOT/META/REGION ones in the server module? Looks like that would be a bigger refactor, but maybe a necessary one...?
          Hide
          stack added a comment -

          Do you know if that is a fixable problem? It adds some complication for the prefix trie.

          Yeah. Its a PITA. If ROOT goes away, that'll simplify things some. IIRC, HBASE-2600 would simplify this comparison again though you'd still need a custom compare on .META.

          Show
          stack added a comment - Do you know if that is a fixable problem? It adds some complication for the prefix trie. Yeah. Its a PITA. If ROOT goes away, that'll simplify things some. IIRC, HBASE-2600 would simplify this comparison again though you'd still need a custom compare on .META.
          Hide
          Matt Corgan added a comment -

          haha - he is the late Clarence Clemons from the E-street Band. He actually passed away after i named my computer after him.

          Seems whack that hbase-commons needs to know about the 'REGIONINFO_DELIMITER'

          This looks related to a bigger problem where META and ROOT tables can't share the same comparator as normal tables. Do you know if that is a fixable problem? It adds some complication for the prefix trie.

          I won't bring over TestConst.

          yeah - sorry, doesn't look like it's needed anymore. I forget why I had pulled it up in the first place. thanks for fixing

          Show
          Matt Corgan added a comment - haha - he is the late Clarence Clemons from the E-street Band. He actually passed away after i named my computer after him. Seems whack that hbase-commons needs to know about the 'REGIONINFO_DELIMITER' This looks related to a bigger problem where META and ROOT tables can't share the same comparator as normal tables. Do you know if that is a fixable problem? It adds some complication for the prefix trie. I won't bring over TestConst. yeah - sorry, doesn't look like it's needed anymore. I forget why I had pulled it up in the first place. thanks for fixing
          Hide
          stack added a comment -

          Who is 'clarence'?

          Seems whack that hbase-commons needs to know about the 'REGIONINFO_DELIMITER'... but we can do finer grained cleanup later.

          I won't bring over TestConst. These top-level constants only interface I think an ugly anti-pattern. I'll take care of making things work w/o bringing over TestConst, no worries.

          Otherwise, looks good. Lets see how hadoopqa does.

          Good on you Matt

          Show
          stack added a comment - Who is 'clarence'? Seems whack that hbase-commons needs to know about the 'REGIONINFO_DELIMITER'... but we can do finer grained cleanup later. I won't bring over TestConst. These top-level constants only interface I think an ugly anti-pattern. I'll take care of making things work w/o bringing over TestConst, no worries. Otherwise, looks good. Lets see how hadoopqa does. Good on you Matt
          Ted Yu made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Fix Version/s 0.96.0 [ 12320040 ]
          Matt Corgan made changes -
          Link This issue is blocked by HBASE-6087 [ HBASE-6087 ]
          Matt Corgan made changes -
          Link This issue relates to HBASE-5977 [ HBASE-5977 ]
          Matt Corgan made changes -
          Link This issue blocks HBASE-4676 [ HBASE-4676 ]
          Matt Corgan made changes -
          Field Original Value New Value
          Attachment HBASE-6162-v1.patch [ 12530915 ]
          Hide
          Matt Corgan added a comment -

          Attaching patch matching the description

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

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development