HBase
  1. HBase
  2. HBASE-6669

Add BigDecimalColumnInterpreter for doing aggregations using AggregationClient

    Details

    • Hadoop Flags:
      Reviewed

      Description

      I recently created a Class for doing aggregations(sum,min,max,std) on values stored as BigDecimal in HBase. I would like to commit the BigDecimalColumnInterpreter into HBase. In my opinion this class can be used by a wide variety of users. Please let me know if its not appropriate to add this class in HBase.

      Thanks,
      Anil Gupta
      Software Engineer II, Intuit, Inc

      1. 6669-0.94-v4.txt
        28 kB
        Ted Yu
      2. 6669-0.94-v5.txt
        28 kB
        Ted Yu
      3. BigDecimalColumnInterpreter.java
        3 kB
        Anil Gupta
      4. BigDecimalColumnInterpreter.patch
        4 kB
        Anil Gupta
      5. BigDecimalColumnInterpreter.patch
        4 kB
        Anil Gupta
      6. HBASE-6669.patch
        28 kB
        Anil Gupta
      7. HBASE-6669-v2.patch
        28 kB
        Anil Gupta
      8. HBASE-6669-v3.patch
        28 kB
        Anil Gupta
      9. TestBDAggregateProtocol.patch
        180 kB
        Julian Wissmann
      10. TestBigDecimalColumnInterpreter.java
        23 kB
        Anil Gupta

        Issue Links

          Activity

          Hide
          Hudson added a comment -

          Integrated in hbase-0.95 #7 (See https://builds.apache.org/job/hbase-0.95/7/)
          HBASE-7641 Port HBASE-6669 'Add BigDecimalColumnInterpreter for doing aggregations using AggregationClient' to trunk (Julian Wissman) (Revision 1451038)

          Result = FAILURE
          tedyu :
          Files :

          • /hbase/branches/0.95/hbase-protocol/src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java
          • /hbase/branches/0.95/hbase-protocol/src/main/protobuf/hbase.proto
          • /hbase/branches/0.95/hbase-server/src/main/java/org/apache/hadoop/hbase/client/coprocessor/BigDecimalColumnInterpreter.java
          • /hbase/branches/0.95/hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestBigDecimalColumnInterpreter.java
          Show
          Hudson added a comment - Integrated in hbase-0.95 #7 (See https://builds.apache.org/job/hbase-0.95/7/ ) HBASE-7641 Port HBASE-6669 'Add BigDecimalColumnInterpreter for doing aggregations using AggregationClient' to trunk (Julian Wissman) (Revision 1451038) Result = FAILURE tedyu : Files : /hbase/branches/0.95/hbase-protocol/src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java /hbase/branches/0.95/hbase-protocol/src/main/protobuf/hbase.proto /hbase/branches/0.95/hbase-server/src/main/java/org/apache/hadoop/hbase/client/coprocessor/BigDecimalColumnInterpreter.java /hbase/branches/0.95/hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestBigDecimalColumnInterpreter.java
          Hide
          Hudson added a comment -

          Integrated in hbase-0.95-on-hadoop2 #3 (See https://builds.apache.org/job/hbase-0.95-on-hadoop2/3/)
          HBASE-7641 Port HBASE-6669 'Add BigDecimalColumnInterpreter for doing aggregations using AggregationClient' to trunk (Julian Wissman) (Revision 1451038)

          Result = FAILURE
          tedyu :
          Files :

          • /hbase/branches/0.95/hbase-protocol/src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java
          • /hbase/branches/0.95/hbase-protocol/src/main/protobuf/hbase.proto
          • /hbase/branches/0.95/hbase-server/src/main/java/org/apache/hadoop/hbase/client/coprocessor/BigDecimalColumnInterpreter.java
          • /hbase/branches/0.95/hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestBigDecimalColumnInterpreter.java
          Show
          Hudson added a comment - Integrated in hbase-0.95-on-hadoop2 #3 (See https://builds.apache.org/job/hbase-0.95-on-hadoop2/3/ ) HBASE-7641 Port HBASE-6669 'Add BigDecimalColumnInterpreter for doing aggregations using AggregationClient' to trunk (Julian Wissman) (Revision 1451038) Result = FAILURE tedyu : Files : /hbase/branches/0.95/hbase-protocol/src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java /hbase/branches/0.95/hbase-protocol/src/main/protobuf/hbase.proto /hbase/branches/0.95/hbase-server/src/main/java/org/apache/hadoop/hbase/client/coprocessor/BigDecimalColumnInterpreter.java /hbase/branches/0.95/hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestBigDecimalColumnInterpreter.java
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK-on-Hadoop-2.0.0 #423 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-2.0.0/423/)
          HBASE-7641 Port HBASE-6669 'Add BigDecimalColumnInterpreter for doing aggregations using AggregationClient' to trunk (Julian Wissman) (Revision 1451037)

          Result = FAILURE
          tedyu :
          Files :

          • /hbase/trunk/hbase-protocol/src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java
          • /hbase/trunk/hbase-protocol/src/main/protobuf/hbase.proto
          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/client/coprocessor/BigDecimalColumnInterpreter.java
          • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestBigDecimalColumnInterpreter.java
          Show
          Hudson added a comment - Integrated in HBase-TRUNK-on-Hadoop-2.0.0 #423 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-2.0.0/423/ ) HBASE-7641 Port HBASE-6669 'Add BigDecimalColumnInterpreter for doing aggregations using AggregationClient' to trunk (Julian Wissman) (Revision 1451037) Result = FAILURE tedyu : Files : /hbase/trunk/hbase-protocol/src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java /hbase/trunk/hbase-protocol/src/main/protobuf/hbase.proto /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/client/coprocessor/BigDecimalColumnInterpreter.java /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestBigDecimalColumnInterpreter.java
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK #3904 (See https://builds.apache.org/job/HBase-TRUNK/3904/)
          HBASE-7641 Port HBASE-6669 'Add BigDecimalColumnInterpreter for doing aggregations using AggregationClient' to trunk (Julian Wissman) (Revision 1451037)

          Result = FAILURE
          tedyu :
          Files :

          • /hbase/trunk/hbase-protocol/src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java
          • /hbase/trunk/hbase-protocol/src/main/protobuf/hbase.proto
          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/client/coprocessor/BigDecimalColumnInterpreter.java
          • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestBigDecimalColumnInterpreter.java
          Show
          Hudson added a comment - Integrated in HBase-TRUNK #3904 (See https://builds.apache.org/job/HBase-TRUNK/3904/ ) HBASE-7641 Port HBASE-6669 'Add BigDecimalColumnInterpreter for doing aggregations using AggregationClient' to trunk (Julian Wissman) (Revision 1451037) Result = FAILURE tedyu : Files : /hbase/trunk/hbase-protocol/src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java /hbase/trunk/hbase-protocol/src/main/protobuf/hbase.proto /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/client/coprocessor/BigDecimalColumnInterpreter.java /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestBigDecimalColumnInterpreter.java
          Hide
          Hudson added a comment -

          Integrated in HBase-0.94-security-on-Hadoop-23 #11 (See https://builds.apache.org/job/HBase-0.94-security-on-Hadoop-23/11/)
          HBASE-6669 Add BigDecimalColumnInterpreter for doing aggregations using AggregationClient (Anil Gupta) (Revision 1436726)

          Result = FAILURE
          tedyu :
          Files :

          • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/coprocessor/BigDecimalColumnInterpreter.java
          • /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/coprocessor/TestBigDecimalColumnInterpreter.java
          Show
          Hudson added a comment - Integrated in HBase-0.94-security-on-Hadoop-23 #11 (See https://builds.apache.org/job/HBase-0.94-security-on-Hadoop-23/11/ ) HBASE-6669 Add BigDecimalColumnInterpreter for doing aggregations using AggregationClient (Anil Gupta) (Revision 1436726) Result = FAILURE tedyu : Files : /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/coprocessor/BigDecimalColumnInterpreter.java /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/coprocessor/TestBigDecimalColumnInterpreter.java
          Hide
          Lars Hofhansl added a comment -

          Agreed.

          Show
          Lars Hofhansl added a comment - Agreed.
          Hide
          Andrew Purtell added a comment -

          Not worth holding up a release of course, but its not a good idea to have things on a branch with promises to port to trunk of indeterminate timeframe. Makes it difficult to reason about what features may be in what, as time goes on. Really should be avoided.

          Show
          Andrew Purtell added a comment - Not worth holding up a release of course, but its not a good idea to have things on a branch with promises to port to trunk of indeterminate timeframe. Makes it difficult to reason about what features may be in what, as time goes on. Really should be avoided.
          Hide
          Lars Hofhansl added a comment -

          A'right... Then I am closing one again

          Show
          Lars Hofhansl added a comment - A'right... Then I am closing one again
          Hide
          Anil Gupta added a comment -

          Yes, i can create the patch for 0.96. But, i wont be able to do it today. Hope that's fine. If yes, I'll take care of HBASE-7641.

          Show
          Anil Gupta added a comment - Yes, i can create the patch for 0.96. But, i wont be able to do it today. Hope that's fine. If yes, I'll take care of HBASE-7641 .
          Hide
          Lars Hofhansl added a comment -

          Anil Gupta Are you volunteering a 0.96 patch?
          If we cannot do this today, then let's close this and do the work as part of (one of) the forward porting issue(s), so that I can cut 0.94.5RC0.

          Show
          Lars Hofhansl added a comment - Anil Gupta Are you volunteering a 0.96 patch? If we cannot do this today, then let's close this and do the work as part of (one of) the forward porting issue(s), so that I can cut 0.94.5RC0.
          Hide
          Lars Hofhansl added a comment -

          We should follow the Hadoop practice of requiring all new features or cross-version common bugfixes to go through trunk first.

          I think there is no hard rule around this, neither should there be (IMHO). For simple issues (like this one) as long as we keep an issue open about the forward port that is OK (again IMHO).
          Agree 100% that bigger changes must go into trunk first.

          Regarding this particular issue: I'm generating the release notes from jira now. So as long as this issue is not marked as fixed I can't do that.

          Show
          Lars Hofhansl added a comment - We should follow the Hadoop practice of requiring all new features or cross-version common bugfixes to go through trunk first. I think there is no hard rule around this, neither should there be (IMHO). For simple issues (like this one) as long as we keep an issue open about the forward port that is OK (again IMHO). Agree 100% that bigger changes must go into trunk first. Regarding this particular issue: I'm generating the release notes from jira now. So as long as this issue is not marked as fixed I can't do that.
          Hide
          Andrew Purtell added a comment -

          Show
          Andrew Purtell added a comment -
          Hide
          Lars Hofhansl added a comment -

          Wait, there's HBASE-7641 and HBASE-7736.

          Show
          Lars Hofhansl added a comment - Wait, there's HBASE-7641 and HBASE-7736 .
          Hide
          Lars Hofhansl added a comment -

          Sorry... You just did that... Ok, reopening

          Show
          Lars Hofhansl added a comment - Sorry... You just did that... Ok, reopening
          Hide
          Lars Hofhansl added a comment -

          We have a forward port issue (Ted opened HBASE-7641), so we can keep this one closed.
          (Or we could close the new one and keep this one open)

          Show
          Lars Hofhansl added a comment - We have a forward port issue (Ted opened HBASE-7641 ), so we can keep this one closed. (Or we could close the new one and keep this one open)
          Hide
          Ted Yu added a comment -

          @Anil:
          Take a look at LongColumnInterpreter in trunk as an example.
          There're a few methods to be implemented for BigDecimalColumnInterpreter.

          If you have questions, feel free to ask.

          Show
          Ted Yu added a comment - @Anil: Take a look at LongColumnInterpreter in trunk as an example. There're a few methods to be implemented for BigDecimalColumnInterpreter. If you have questions, feel free to ask.
          Hide
          Andrew Purtell added a comment -

          Reopened because we have a branch commit with no trunk commit first here.

          Do we have an explicit policy for this? If not, we should. We should follow the Hadoop practice of requiring all new features or cross-version common bugfixes to go through trunk first.

          Show
          Andrew Purtell added a comment - Reopened because we have a branch commit with no trunk commit first here. Do we have an explicit policy for this? If not, we should. We should follow the Hadoop practice of requiring all new features or cross-version common bugfixes to go through trunk first.
          Hide
          Anil Gupta added a comment -

          Lars Hofhansl Yes, i would like to port it to 0.96 as well. I haven't looked into changes required for porting. I'll try to look into it.

          Show
          Anil Gupta added a comment - Lars Hofhansl Yes, i would like to port it to 0.96 as well. I haven't looked into changes required for porting. I'll try to look into it.
          Hide
          Lars Hofhansl added a comment -

          So this in only in 0.94. I assume we want this in 0.96 as well?

          Show
          Lars Hofhansl added a comment - So this in only in 0.94. I assume we want this in 0.96 as well?
          Hide
          Hudson added a comment -

          Integrated in HBase-0.94-security #96 (See https://builds.apache.org/job/HBase-0.94-security/96/)
          HBASE-6669 Add BigDecimalColumnInterpreter for doing aggregations using AggregationClient (Anil Gupta) (Revision 1436726)

          Result = FAILURE
          tedyu :
          Files :

          • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/coprocessor/BigDecimalColumnInterpreter.java
          • /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/coprocessor/TestBigDecimalColumnInterpreter.java
          Show
          Hudson added a comment - Integrated in HBase-0.94-security #96 (See https://builds.apache.org/job/HBase-0.94-security/96/ ) HBASE-6669 Add BigDecimalColumnInterpreter for doing aggregations using AggregationClient (Anil Gupta) (Revision 1436726) Result = FAILURE tedyu : Files : /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/coprocessor/BigDecimalColumnInterpreter.java /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/coprocessor/TestBigDecimalColumnInterpreter.java
          Hide
          Hudson added a comment -

          Integrated in HBase-0.94 #750 (See https://builds.apache.org/job/HBase-0.94/750/)
          HBASE-6669 Add BigDecimalColumnInterpreter for doing aggregations using AggregationClient (Anil Gupta) (Revision 1436726)

          Result = SUCCESS
          tedyu :
          Files :

          • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/coprocessor/BigDecimalColumnInterpreter.java
          • /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/coprocessor/TestBigDecimalColumnInterpreter.java
          Show
          Hudson added a comment - Integrated in HBase-0.94 #750 (See https://builds.apache.org/job/HBase-0.94/750/ ) HBASE-6669 Add BigDecimalColumnInterpreter for doing aggregations using AggregationClient (Anil Gupta) (Revision 1436726) Result = SUCCESS tedyu : Files : /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/coprocessor/BigDecimalColumnInterpreter.java /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/coprocessor/TestBigDecimalColumnInterpreter.java
          Hide
          Ted Yu added a comment -

          Integrated to 0.94 branch.

          Thanks for the patch, Anil.

          Thanks for the review, Lars.

          Show
          Ted Yu added a comment - Integrated to 0.94 branch. Thanks for the patch, Anil. Thanks for the review, Lars.
          Hide
          Lars Hofhansl added a comment -

          Awesome, thanks Ted!
          +1 on commit.

          Show
          Lars Hofhansl added a comment - Awesome, thanks Ted! +1 on commit.
          Hide
          Ted Yu added a comment -

          Patch v5 addresses latest review comments.

          Running org.apache.hadoop.hbase.coprocessor.TestBigDecimalColumnInterpreter
          2013-01-21 14:06:27.730 java[99778:1203] Unable to load realm info from SCDynamicStore
          Tests run: 38, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 38.557 sec

          Show
          Ted Yu added a comment - Patch v5 addresses latest review comments. Running org.apache.hadoop.hbase.coprocessor.TestBigDecimalColumnInterpreter 2013-01-21 14:06:27.730 java [99778:1203] Unable to load realm info from SCDynamicStore Tests run: 38, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 38.557 sec
          Hide
          Anil Gupta added a comment -

          Lars Hofhansl
          Thanks for extending your support. This trivial patch took me a lot of time/effort in formatting due to unknown reasons. I hope that same doesn't happens in HBASE-7474. Let me know if i need to do anything further in this. Ecstatic to get my first patch in HBase.YAY!!

          Show
          Anil Gupta added a comment - Lars Hofhansl Thanks for extending your support. This trivial patch took me a lot of time/effort in formatting due to unknown reasons. I hope that same doesn't happens in HBASE-7474 . Let me know if i need to do anything further in this. Ecstatic to get my first patch in HBase.YAY!!
          Hide
          Lars Hofhansl added a comment -

          Let's get this in.
          And let's please not argue about empty line.

          I'll format the patch according to our guidelines and will attach a new version soon.

          Show
          Lars Hofhansl added a comment - Let's get this in. And let's please not argue about empty line. I'll format the patch according to our guidelines and will attach a new version soon.
          Hide
          Ted Yu added a comment -
          +  public void testMaxWithValidRange() throws Throwable {
          

          Do you specify range in the above method ? Range is specified in testMaxWithValidRange2

          +  public void testMaxWithValidRangeWithNoCQ() throws Throwable {
          

          Rename the method testMaxWithValidRangeWithoutCQ

          +    log.debug("Inside readFields method of DoubleColumnInterpreter");^M
          

          Please remove unnecessary debug logs.

          +  public BigDecimal getValue(byte[] paramArrayOfByte1, byte[] paramArrayOfByte2, KeyValue kv)^M
          

          The first two parameters are family and qualifier. Please name them accordingly.

          +    if ((((val1 == null) ? 1 : 0) ^ ((val2 == null) ? 1 : 0)) != 0) return ((val1 == null) ? val2^M
          +        : val1);^M
          

          Since the if statement spans two lines, use curly braces to surround the return statement.
          I don't know where the ^M came from. It would be nice to remove them (using tool such as dos2unix).

          Show
          Ted Yu added a comment - + public void testMaxWithValidRange() throws Throwable { Do you specify range in the above method ? Range is specified in testMaxWithValidRange2 + public void testMaxWithValidRangeWithNoCQ() throws Throwable { Rename the method testMaxWithValidRangeWithoutCQ + log.debug( "Inside readFields method of DoubleColumnInterpreter" );^M Please remove unnecessary debug logs. + public BigDecimal getValue( byte [] paramArrayOfByte1, byte [] paramArrayOfByte2, KeyValue kv)^M The first two parameters are family and qualifier. Please name them accordingly. + if ((((val1 == null ) ? 1 : 0) ^ ((val2 == null ) ? 1 : 0)) != 0) return ((val1 == null ) ? val2^M + : val1);^M Since the if statement spans two lines, use curly braces to surround the return statement. I don't know where the ^M came from. It would be nice to remove them (using tool such as dos2unix).
          Hide
          Anil Gupta added a comment -

          anil_gupta:~ anil$ svn --version
          svn, version 1.6.17 (r1128011)
          compiled Nov 8 2011, 18:14:46

          anil_gupta:0.94.3 anil$ svn diff | grep 'Index:'
          svn: The path '.' appears to be part of a Subversion 1.7 or greater
          working copy. Please upgrade your Subversion client to use this
          working copy.

          It seems like a minor update is making a big difference.

          Show
          Anil Gupta added a comment - anil_gupta:~ anil$ svn --version svn, version 1.6.17 (r1128011) compiled Nov 8 2011, 18:14:46 anil_gupta:0.94.3 anil$ svn diff | grep 'Index:' svn: The path '.' appears to be part of a Subversion 1.7 or greater working copy. Please upgrade your Subversion client to use this working copy. It seems like a minor update is making a big difference.
          Hide
          Ted Yu added a comment -

          Here is the svn I am using:

          tyu$ svn --version
          svn, version 1.6.18 (r1303927)

          Show
          Ted Yu added a comment - Here is the svn I am using: tyu$ svn --version svn, version 1.6.18 (r1303927)
          Hide
          Anil Gupta added a comment -

          Attaching the source and test file.
          I could not run svn command line since HBase seems to use svn1.7 and my client is svn1.6. I got an error on running svn diff command. I need to update my svn which seems quite an effort.

          I even did a comparison of my class and LongColumnInterpreter to find which empty lines are causing problems but i could not find any significant thing. I would appreciate if you could point the empty lines from the source files. So, in future i will try to avoid this mistake.
          Thanks a lot for the help, Ted.

          ~Anil Gupta
          Software Engineer II, Intuit, Inc

          Show
          Anil Gupta added a comment - Attaching the source and test file. I could not run svn command line since HBase seems to use svn1.7 and my client is svn1.6. I got an error on running svn diff command. I need to update my svn which seems quite an effort. I even did a comparison of my class and LongColumnInterpreter to find which empty lines are causing problems but i could not find any significant thing. I would appreciate if you could point the empty lines from the source files. So, in future i will try to avoid this mistake. Thanks a lot for the help, Ted. ~Anil Gupta Software Engineer II, Intuit, Inc
          Hide
          Ted Yu added a comment -

          If patch v4 still contains empty line, you can attach the two new files to this JIRA.

          Show
          Ted Yu added a comment - If patch v4 still contains empty line, you can attach the two new files to this JIRA.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12563143/HBASE-6669-v3.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 patch. The patch command could not apply the patch.

          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/3837//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/12563143/HBASE-6669-v3.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 patch . The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/3837//console This message is automatically generated.
          Hide
          Ted Yu added a comment -

          There is still empty line in patch v3.

          I use the following alias to obtain list of files modified locally:

          alias slst="svn diff | grep 'Index:'"

          Once you have the list, you can use the following to generate patch:

          svn diff 'path-to-file1' > 6669-0.94.txt
          svn diff 'path-to-file2' >> 6669-0.94.txt

          Show
          Ted Yu added a comment - There is still empty line in patch v3. I use the following alias to obtain list of files modified locally: alias slst="svn diff | grep 'Index:'" Once you have the list, you can use the following to generate patch: svn diff 'path-to-file1' > 6669-0.94.txt svn diff 'path-to-file2' >> 6669-0.94.txt
          Hide
          Anil Gupta added a comment -

          Attaching the updated patch. I removed the empty lines between import statements. Couldn't see them because import statement are folded by default in Eclipse. Sorry for the oversight.
          I am using this license header:
          /*
          *

          • Licensed to the Apache Software Foundation (ASF) under one
          • or more contributor license agreements. See the NOTICE file
          • distributed with this work for additional information
          • regarding copyright ownership. The ASF licenses this file
          • to you under the Apache License, Version 2.0 (the
          • "License"); you may not use this file except in compliance
          • with the License. You may obtain a copy of the License at
            *
          • http://www.apache.org/licenses/LICENSE-2.0
            *
          • Unless required by applicable law or agreed to in writing, software
          • distributed under the License is distributed on an "AS IS" BASIS,
          • WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          • See the License for the specific language governing permissions and
          • limitations under the License.
            */

          Please let me know the correct license headers if the above is wrong.

          I haven't used svn command since i don't know the command to exclude the files for HBASE-7474.
          I am a newbie at Apache patching so i am trying my best to learn about the stringent Guidelines.
          Thanks a lot for your help and valuable time.

          Show
          Anil Gupta added a comment - Attaching the updated patch. I removed the empty lines between import statements. Couldn't see them because import statement are folded by default in Eclipse. Sorry for the oversight. I am using this license header: /* * Licensed to the Apache Software Foundation (ASF) under one or more contributor license agreements. See the NOTICE file distributed with this work for additional information regarding copyright ownership. The ASF licenses this file to you under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at * http://www.apache.org/licenses/LICENSE-2.0 * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. */ Please let me know the correct license headers if the above is wrong. I haven't used svn command since i don't know the command to exclude the files for HBASE-7474 . I am a newbie at Apache patching so i am trying my best to learn about the stringent Guidelines. Thanks a lot for your help and valuable time.
          Hide
          Ted Yu added a comment -

          There're empty lines in BigDecimalColumnInterpreter.java but not in TestBigDecimalColumnInterpreter.java

          The license headers in both files don't look the same as in other source files.
          When I tried to apply the patch:

          p0 6669-v2.patch
          patching file src/main/java/org/apache/hadoop/hbase/client/coprocessor/BigDecimalColumnInterpreter.java
          patch: **** malformed patch at line 7: 
          

          Have you tried using svn command to generate patch ?
          Please remove the empty lines in patch.

          Show
          Ted Yu added a comment - There're empty lines in BigDecimalColumnInterpreter.java but not in TestBigDecimalColumnInterpreter.java The license headers in both files don't look the same as in other source files. When I tried to apply the patch: p0 6669-v2.patch patching file src/main/java/org/apache/hadoop/hbase/client/coprocessor/BigDecimalColumnInterpreter.java patch: **** malformed patch at line 7: Have you tried using svn command to generate patch ? Please remove the empty lines in patch.
          Hide
          Anil Gupta added a comment -

          Can someone tell me what's wrong with the patch now? I followed the instructions to create then also it didn't go through. Thanks in advance.

          Show
          Anil Gupta added a comment - Can someone tell me what's wrong with the patch now? I followed the instructions to create then also it didn't go through. Thanks in advance.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12563128/HBASE-6669-v2.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 patch. The patch command could not apply the patch.

          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/3830//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/12563128/HBASE-6669-v2.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 patch . The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/3830//console This message is automatically generated.
          Hide
          Anil Gupta added a comment -

          Removed empty lines from code. Followed instruction on http://commons.apache.org/patches.html to create a patch. Here is the snippet of instruction:
          " If using Eclipse to create the patch, set "Patch Root" to "Project" - not the default "Workspace". [Workspace-relative patches are not portable unless exactly the same project names are used.] "

          Hope it goes fine this time.

          Show
          Anil Gupta added a comment - Removed empty lines from code. Followed instruction on http://commons.apache.org/patches.html to create a patch. Here is the snippet of instruction: " If using Eclipse to create the patch, set "Patch Root" to "Project" - not the default "Workspace". [Workspace-relative patches are not portable unless exactly the same project names are used.] " Hope it goes fine this time.
          Hide
          Ted Yu added a comment -

          @Anil:
          The formatter wouldn't remove empty lines.
          Do you see empty lines in Eclipse ?

          Show
          Ted Yu added a comment - @Anil: The formatter wouldn't remove empty lines. Do you see empty lines in Eclipse ?
          Hide
          Anil Gupta added a comment -

          Hi Ted,

          I've already used the HBase Formatter in the current patch. Does the formatter removes empty lines?

          Show
          Anil Gupta added a comment - Hi Ted, I've already used the HBase Formatter in the current patch. Does the formatter removes empty lines?
          Hide
          Ted Yu added a comment -

          Take a look at http://hbase.apache.org/book.html#submitting.patches

          In trunk, you can import dev-support/hbase_eclipse_formatter.xml to Eclipse.

          In your recent patch, there are empty lines between code. Those empty lines should be removed.

          Show
          Ted Yu added a comment - Take a look at http://hbase.apache.org/book.html#submitting.patches In trunk, you can import dev-support/hbase_eclipse_formatter.xml to Eclipse. In your recent patch, there are empty lines between code. Those empty lines should be removed.
          Hide
          Anil Gupta added a comment -

          Can anyone tell me how to create the patch in the correct way from eclipse? or svn command?

          Thanks,
          Anil Gupta
          Software Engineer II, Intuit, inc

          Show
          Anil Gupta added a comment - Can anyone tell me how to create the patch in the correct way from eclipse? or svn command? Thanks, Anil Gupta Software Engineer II, Intuit, inc
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12562860/HBASE-6669.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 patch. The patch command could not apply the patch.

          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/3797//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/12562860/HBASE-6669.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 patch . The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/3797//console This message is automatically generated.
          Hide
          Anil Gupta added a comment -

          I created this patch from the 0.94.3 tag of HBase. Hope it's fine. We can create another Jira to port this ColumnInterpreter to HBase0.96. Is that fine?
          I have created TestClass named as TestBigDecimalColumnInterpreter. This test is similar to TestAggregateProtocol.
          Let me know if the patch required some more changes.

          Thanks,
          Anil Gupta
          Software Engineer II, Intuit, inc

          Show
          Anil Gupta added a comment - I created this patch from the 0.94.3 tag of HBase. Hope it's fine. We can create another Jira to port this ColumnInterpreter to HBase0.96. Is that fine? I have created TestClass named as TestBigDecimalColumnInterpreter. This test is similar to TestAggregateProtocol. Let me know if the patch required some more changes. Thanks, Anil Gupta Software Engineer II, Intuit, inc
          Hide
          Anil Gupta added a comment -

          Hi Julian,

          I am curious to know whether you got the opportunity to test BDCI utility i sent(on hbase user list) last week along with some suggestions on using it? Did it run successfully?

          I will try to have a look at your unit test over weekend.

          Thanks,
          Anil Gupta
          Software Engineer II, Intuit, Inc

          Show
          Anil Gupta added a comment - Hi Julian, I am curious to know whether you got the opportunity to test BDCI utility i sent(on hbase user list) last week along with some suggestions on using it? Did it run successfully? I will try to have a look at your unit test over weekend. Thanks, Anil Gupta Software Engineer II, Intuit, Inc
          Hide
          Julian Wissmann added a comment -

          I've written a Test for BDColumnInterpreter, however, all Medium Tests requiring MiniDFSCluster fail on my machine:

          -------------------------------------------------------------------------------
          Test set: org.apache.hadoop.hbase.coprocessor.TestAggregateProtocol
          -------------------------------------------------------------------------------
          Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 3.239 sec <<< FAILURE!
          org.apache.hadoop.hbase.coprocessor.TestAggregateProtocol Time elapsed: 0.001 sec <<< ERROR!
          java.lang.NullPointerException
          at org.apache.hadoop.hdfs.MiniDFSCluster.startDataNodes(MiniDFSCluster.java:422)
          at org.apache.hadoop.hdfs.MiniDFSCluster.<init>(MiniDFSCluster.java:280)
          at org.apache.hadoop.hbase.HBaseTestingUtility.startMiniDFSCluster(HBaseTestingUtility.java:433)
          at org.apache.hadoop.hbase.HBaseTestingUtility.startMiniCluster(HBaseTestingUtility.java:653)
          at org.apache.hadoop.hbase.HBaseTestingUtility.startMiniCluster(HBaseTestingUtility.java:603)
          at org.apache.hadoop.hbase.HBaseTestingUtility.startMiniCluster(HBaseTestingUtility.java:590)
          at org.apache.hadoop.hbase.coprocessor.TestAggregateProtocol.setupBeforeClass(TestAggregateProtocol.java:77)
          at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
          at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
          at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
          at java.lang.reflect.Method.invoke(Method.java:597)
          at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:45)
          at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:15)
          at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:42)
          at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:27)
          at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:30)
          at org.junit.runners.ParentRunner.run(ParentRunner.java:300)
          at org.junit.runners.Suite.runChild(Suite.java:128)
          at org.junit.runners.Suite.runChild(Suite.java:24)
          at org.junit.runners.ParentRunner$3.run(ParentRunner.java:231)
          at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:441)
          at java.util.concurrent.FutureTask$Sync.innerRun(FutureTask.java:303)
          at java.util.concurrent.FutureTask.run(FutureTask.java:138)
          at java.util.concurrent.ThreadPoolExecutor$Worker.runTask(ThreadPoolExecutor.java:886)
          at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:908)
          at java.lang.Thread.run(Thread.java:662)

          Therefor I have not been able to do a run of my test, so far. I can however attach it here, if someone who does not run into this problem is willing to give it a try.

          Show
          Julian Wissmann added a comment - I've written a Test for BDColumnInterpreter, however, all Medium Tests requiring MiniDFSCluster fail on my machine: ------------------------------------------------------------------------------- Test set: org.apache.hadoop.hbase.coprocessor.TestAggregateProtocol ------------------------------------------------------------------------------- Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 3.239 sec <<< FAILURE! org.apache.hadoop.hbase.coprocessor.TestAggregateProtocol Time elapsed: 0.001 sec <<< ERROR! java.lang.NullPointerException at org.apache.hadoop.hdfs.MiniDFSCluster.startDataNodes(MiniDFSCluster.java:422) at org.apache.hadoop.hdfs.MiniDFSCluster.<init>(MiniDFSCluster.java:280) at org.apache.hadoop.hbase.HBaseTestingUtility.startMiniDFSCluster(HBaseTestingUtility.java:433) at org.apache.hadoop.hbase.HBaseTestingUtility.startMiniCluster(HBaseTestingUtility.java:653) at org.apache.hadoop.hbase.HBaseTestingUtility.startMiniCluster(HBaseTestingUtility.java:603) at org.apache.hadoop.hbase.HBaseTestingUtility.startMiniCluster(HBaseTestingUtility.java:590) at org.apache.hadoop.hbase.coprocessor.TestAggregateProtocol.setupBeforeClass(TestAggregateProtocol.java:77) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) at java.lang.reflect.Method.invoke(Method.java:597) at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:45) at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:15) at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:42) at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:27) at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:30) at org.junit.runners.ParentRunner.run(ParentRunner.java:300) at org.junit.runners.Suite.runChild(Suite.java:128) at org.junit.runners.Suite.runChild(Suite.java:24) at org.junit.runners.ParentRunner$3.run(ParentRunner.java:231) at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:441) at java.util.concurrent.FutureTask$Sync.innerRun(FutureTask.java:303) at java.util.concurrent.FutureTask.run(FutureTask.java:138) at java.util.concurrent.ThreadPoolExecutor$Worker.runTask(ThreadPoolExecutor.java:886) at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:908) at java.lang.Thread.run(Thread.java:662) Therefor I have not been able to do a run of my test, so far. I can however attach it here, if someone who does not run into this problem is willing to give it a try.
          Hide
          Ted Yu added a comment -

          Since there're two BigDecimal fields in BigDecimalColumnInterpreter, you need to implement readFields() and write() for serialization.

          Show
          Ted Yu added a comment - Since there're two BigDecimal fields in BigDecimalColumnInterpreter, you need to implement readFields() and write() for serialization.
          Hide
          Ted Yu added a comment -

          To generate patch, from the root of your workspace, type:

          svn diff hbase-server/src/main/java/org/apache/hadoop/hbase/client/coprocessor/BigDecimalColumnInterpreter.java
          

          Year is not needed for license:

          + * Copyright 2011 The Apache Software Foundation
          

          Remove the following comment:

          +    // TODO Auto-generated method stub
          

          Either move the return statement to the end of if statement or enclose it in curly braces:

          +    if (val1 == null)
          +      return null;
          

          The rest looks fine.
          TestAggregateProtocol tests LongColumnInterpreter. You should create a new test file to test your class.

          Thanks

          Show
          Ted Yu added a comment - To generate patch, from the root of your workspace, type: svn diff hbase-server/src/main/java/org/apache/hadoop/hbase/client/coprocessor/BigDecimalColumnInterpreter.java Year is not needed for license: + * Copyright 2011 The Apache Software Foundation Remove the following comment: + // TODO Auto-generated method stub Either move the return statement to the end of if statement or enclose it in curly braces: + if (val1 == null ) + return null ; The rest looks fine. TestAggregateProtocol tests LongColumnInterpreter. You should create a new test file to test your class. Thanks
          Hide
          Anil Gupta added a comment -

          New Patch.

          Show
          Anil Gupta added a comment - New Patch.
          Hide
          Anil Gupta added a comment -

          Ted Yu

          This time i created the patch from "hbase-server"->"src/main/java". I hope it's ok this time. Sorry, this is the first time i am submitting patch.
          I changed 0.0D/0.0D to "Double.NaN" in divideForAvg(). Is this fine? Should i create a separate class for Unit Tests or put my test cases in TestAggregateProtocol?

          Thanks,
          Anil Gupta
          Software Engineer II, Intuit, Inc

          Show
          Anil Gupta added a comment - Ted Yu This time i created the patch from "hbase-server"->"src/main/java". I hope it's ok this time. Sorry, this is the first time i am submitting patch. I changed 0.0D/0.0D to "Double.NaN" in divideForAvg(). Is this fine? Should i create a separate class for Unit Tests or put my test cases in TestAggregateProtocol? Thanks, Anil Gupta Software Engineer II, Intuit, Inc
          Hide
          Ted Yu added a comment -

          @Anil:
          Can you attach patch for trunk where BigDeciamlColumnInterpreter resides in hbase-server module ?
          Please add following annotation to BigDeciamlColumnInterpreter class:

          @InterfaceAudience.Public
          @InterfaceStability.Evolving
          

          Why did you choose 0.0D / 0.0D in divideForAvg() ?

          + public double divideForAvg(BigDecimal val1, Long paramLong) {
          +   return (((paramLong == null) || (val1 == null)) ? (0.0D / 0.0D) : val1.doubleValue()/paramLong.doubleValue());
          

          See HBASE-3678 for an Eclipse formatter.
          Limit line length to 100 characters.

          Thanks

          Show
          Ted Yu added a comment - @Anil: Can you attach patch for trunk where BigDeciamlColumnInterpreter resides in hbase-server module ? Please add following annotation to BigDeciamlColumnInterpreter class: @InterfaceAudience.Public @InterfaceStability.Evolving Why did you choose 0.0D / 0.0D in divideForAvg() ? + public double divideForAvg(BigDecimal val1, Long paramLong) { + return (((paramLong == null ) || (val1 == null )) ? (0.0D / 0.0D) : val1.doubleValue()/paramLong.doubleValue()); See HBASE-3678 for an Eclipse formatter. Limit line length to 100 characters. Thanks
          Hide
          Anil Gupta added a comment -

          Patch file.

          Show
          Anil Gupta added a comment - Patch file.
          Hide
          Anil Gupta added a comment -

          Source file

          Show
          Anil Gupta added a comment - Source file
          Hide
          Anil Gupta added a comment -

          Source file for BigDecimalColumnInterpreter.

          Show
          Anil Gupta added a comment - Source file for BigDecimalColumnInterpreter.
          Hide
          Anil Gupta added a comment -

          Initial Patch for BigDecimalColumnInterpreter.

          Show
          Anil Gupta added a comment - Initial Patch for BigDecimalColumnInterpreter.
          Hide
          Anil Gupta added a comment -

          Himanshu Vashishtha
          Please find attached the patch for BigDeciamlColumnInterpreter for review. I haven't worked on Unit Test and formatting the source code yet. I hope it's ok for reviewing the code.

          @Julian Wissmann,
          I am attaching the java file for BigDeciamlColumnInterpreter file. You wont need to recompile HBase since you can use it directly at client side. Let me know if you face any problem.

          Thanks,
          Anil Gupta
          Software Engineer II, Intuit, Inc

          Show
          Anil Gupta added a comment - Himanshu Vashishtha Please find attached the patch for BigDeciamlColumnInterpreter for review. I haven't worked on Unit Test and formatting the source code yet. I hope it's ok for reviewing the code. @Julian Wissmann, I am attaching the java file for BigDeciamlColumnInterpreter file. You wont need to recompile HBase since you can use it directly at client side. Let me know if you face any problem. Thanks, Anil Gupta Software Engineer II, Intuit, Inc
          Hide
          Anil Gupta added a comment -

          Hi Himanshu,

          I don't have the Hbase source set-up on my eclipse yet so it might take me little more time than usual to upload the batch.

          Thanks,
          Anil Gupta
          Software Engineer II, Intuit, Inc

          Show
          Anil Gupta added a comment - Hi Himanshu, I don't have the Hbase source set-up on my eclipse yet so it might take me little more time than usual to upload the batch. Thanks, Anil Gupta Software Engineer II, Intuit, Inc
          Hide
          Himanshu Vashishtha added a comment -

          Please upload the patch

          Show
          Himanshu Vashishtha added a comment - Please upload the patch

            People

            • Assignee:
              Anil Gupta
              Reporter:
              Anil Gupta
            • Votes:
              0 Vote for this issue
              Watchers:
              12 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development