HBase
  1. HBase
  2. HBASE-11507

Enhance test-patch.sh to check for direct calls to HBaseZeroCopyByteString.wrap()

    Details

    • Type: Task Task
    • Status: Open
    • Priority: Trivial Trivial
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None

      Description

      HBaseZeroCopyByteString.wrap() should not be called directly.
      ByteStringer.wrap() should be called instead.

      This task adds check for direct calls to HBaseZeroCopyByteString.wrap() to test-patch.sh.

      1. HBASE-11507-javadoc.patch
        0.9 kB
        Gustavo Anatoly
      2. HBASE-11507.patch
        1.0 kB
        Gustavo Anatoly

        Activity

        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12655543/HBASE-11507-javadoc.patch
        against trunk revision .
        ATTACHMENT ID: 12655543

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

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 javadoc. The javadoc tool did not generate any warning messages.

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

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

        +1 lineLengths. The patch does not introduce lines longer than 100

        +1 site. The mvn site goal succeeds with this patch.

        +1 core tests. The patch passed unit tests in .

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/10057//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10057//artifact/patchprocess/newPatchFindbugsWarningshbase-common.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10057//artifact/patchprocess/newPatchFindbugsWarningshbase-client.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10057//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10057//artifact/patchprocess/newPatchFindbugsWarningshbase-server.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10057//artifact/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10057//artifact/patchprocess/newPatchFindbugsWarningshbase-protocol.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10057//artifact/patchprocess/newPatchFindbugsWarningshbase-thrift.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10057//artifact/patchprocess/newPatchFindbugsWarningshbase-examples.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10057//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/10057//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/12655543/HBASE-11507-javadoc.patch against trunk revision . ATTACHMENT ID: 12655543 +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. -1 findbugs . The patch appears to introduce 1 new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 lineLengths . The patch does not introduce lines longer than 100 +1 site . The mvn site goal succeeds with this patch. +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/10057//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10057//artifact/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10057//artifact/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10057//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10057//artifact/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10057//artifact/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10057//artifact/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10057//artifact/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10057//artifact/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10057//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/10057//console This message is automatically generated.
        Hide
        stack added a comment -

        I disagree...

        ... is usually followed by argument/justification.

        Add this at least the below Gustavo Anatoly so that the unusual developer who happens to stumble upon the zero copy byte string optimization (totally by-passing our use of ByteStringer elsewhere throughout the codebase) is suitably discouraged....

        diff --git a/hbase-protocol/src/main/java/com/google/protobuf/HBaseZeroCopyByteString.java b/hbase-protocol/src/main/java/com/google/protobuf/HBaseZeroCopyByteString.java
        index f7e9c7f..7f59c9c 100644
        --- a/hbase-protocol/src/main/java/com/google/protobuf/HBaseZeroCopyByteString.java
        +++ b/hbase-protocol/src/main/java/com/google/protobuf/HBaseZeroCopyByteString.java
        @@ -26,7 +26,10 @@ package com.google.protobuf;  // This is a lie.
          * from C buffer to JVM buffer).
          *
          * @since 0.96.1
        + * @deprecated Do not use directly. Instead go via org.apache.hadoop.hbase.util.ByteStringer
        + * class instead. See HBASE-11118
          */
        +@Deprecated
         public final class HBaseZeroCopyByteString extends LiteralByteString {
           // Gotten from AsyncHBase code base with permission.
           /** Private constructor so this class cannot be instantiated. */
        

        Thanks.

        Show
        stack added a comment - I disagree... ... is usually followed by argument/justification. Add this at least the below Gustavo Anatoly so that the unusual developer who happens to stumble upon the zero copy byte string optimization (totally by-passing our use of ByteStringer elsewhere throughout the codebase) is suitably discouraged.... diff --git a/hbase-protocol/src/main/java/com/google/protobuf/HBaseZeroCopyByteString.java b/hbase-protocol/src/main/java/com/google/protobuf/HBaseZeroCopyByteString.java index f7e9c7f..7f59c9c 100644 --- a/hbase-protocol/src/main/java/com/google/protobuf/HBaseZeroCopyByteString.java +++ b/hbase-protocol/src/main/java/com/google/protobuf/HBaseZeroCopyByteString.java @@ -26,7 +26,10 @@ package com.google.protobuf; // This is a lie. * from C buffer to JVM buffer). * * @since 0.96.1 + * @deprecated Do not use directly. Instead go via org.apache.hadoop.hbase.util.ByteStringer + * class instead. See HBASE-11118 */ +@Deprecated public final class HBaseZeroCopyByteString extends LiteralByteString { // Gotten from AsyncHBase code base with permission. /** Private constructor so this class cannot be instantiated. */ Thanks.
        Hide
        Ted Yu added a comment -

        I disagree that the effort by Gustavo of preventing regression w.r.t. HBASE-11118 is trivial.

        Show
        Ted Yu added a comment - I disagree that the effort by Gustavo of preventing regression w.r.t. HBASE-11118 is trivial.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12655469/HBASE-11507.patch
        against trunk revision .
        ATTACHMENT ID: 12655469

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

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

        -1 Anti-pattern. The patch appears to have anti-pattern where BYTES_COMPARATOR was omitted:
        warnings=`$GREP 'new TreeMap<byte.*()' $PATCH_DIR/patch`.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        -1 findbugs. The patch appears to introduce 2 new Findbugs (version 2.0.3) warnings.

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

        +1 lineLengths. The patch does not introduce lines longer than 100

        +1 site. The mvn site goal succeeds with this patch.

        +1 core tests. The patch passed unit tests in .

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/10045//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10045//artifact/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10045//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10045//artifact/patchprocess/newPatchFindbugsWarningshbase-server.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10045//artifact/patchprocess/newPatchFindbugsWarningshbase-examples.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10045//artifact/patchprocess/newPatchFindbugsWarningshbase-common.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10045//artifact/patchprocess/newPatchFindbugsWarningshbase-thrift.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10045//artifact/patchprocess/newPatchFindbugsWarningshbase-protocol.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10045//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10045//artifact/patchprocess/newPatchFindbugsWarningshbase-client.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/10045//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/12655469/HBASE-11507.patch against trunk revision . ATTACHMENT ID: 12655469 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 3 new or modified tests. -1 Anti-pattern . The patch appears to have anti-pattern where BYTES_COMPARATOR was omitted: warnings=`$GREP 'new TreeMap<byte.*()' $PATCH_DIR/patch`. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. -1 findbugs . The patch appears to introduce 2 new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 lineLengths . The patch does not introduce lines longer than 100 +1 site . The mvn site goal succeeds with this patch. +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/10045//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10045//artifact/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10045//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10045//artifact/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10045//artifact/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10045//artifact/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10045//artifact/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10045//artifact/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10045//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10045//artifact/patchprocess/newPatchFindbugsWarningshbase-client.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/10045//console This message is automatically generated.
        Hide
        stack added a comment -

        Gustavo Anatoly Anything that helps with the hbase 1.0 project would be appreciated. See HBASE-10856 for its list of items to do and linked issues. If that list has nought of interest just say and can dig up something else. Write me offline if you'd like input or help, np.

        What a weird issue....

        'weird' is wrong in above. The patch attached could be of some, rare 'utility'.

        Grepping 'HBaseZeroCopyByteString\.wrap()' seems wrong though given the wrap takes args.... and there is also zeroCopyGetBytes....

        I'd think we'd at least start by adding warning javadoc on the objectional class itself discouraging its use with why....

        If out to build yourself a track record Gustavo Anatoly, yeah, take on something more substantial. Good on you.

        Show
        stack added a comment - Gustavo Anatoly Anything that helps with the hbase 1.0 project would be appreciated. See HBASE-10856 for its list of items to do and linked issues. If that list has nought of interest just say and can dig up something else. Write me offline if you'd like input or help, np. What a weird issue.... 'weird' is wrong in above. The patch attached could be of some, rare 'utility'. Grepping 'HBaseZeroCopyByteString\.wrap()' seems wrong though given the wrap takes args.... and there is also zeroCopyGetBytes.... I'd think we'd at least start by adding warning javadoc on the objectional class itself discouraging its use with why.... If out to build yourself a track record Gustavo Anatoly , yeah, take on something more substantial. Good on you.
        Hide
        Gustavo Anatoly added a comment -

        Hi, stack. I would like to work with some substance and import. Do you have some task?

        Thanks.

        Show
        Gustavo Anatoly added a comment - Hi, stack . I would like to work with some substance and import. Do you have some task? Thanks.
        Hide
        stack added a comment -

        What a weird issue. Marking trivial.

        Gustavo Anatoly, if you want to work on something with some substance and import, give me a shout and I'll set you right.

        Show
        stack added a comment - What a weird issue. Marking trivial. Gustavo Anatoly , if you want to work on something with some substance and import, give me a shout and I'll set you right.
        Hide
        Ted Yu added a comment -

        QA only accepts level 1 patch.

        Please regenerate patch.

        Show
        Ted Yu added a comment - QA only accepts level 1 patch. Please regenerate patch.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12655467/HBASE-11507.patch
        against trunk revision .
        ATTACHMENT ID: 12655467

        +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/10044//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/12655467/HBASE-11507.patch against trunk revision . ATTACHMENT ID: 12655467 +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/10044//console This message is automatically generated.

          People

          • Assignee:
            Gustavo Anatoly
            Reporter:
            Ted Yu
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:

              Development