HBase
  1. HBase
  2. HBASE-5357

Use builder pattern in HColumnDescriptor

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None

      Description

      We have five ways to create an HFile writer, two ways to create a StoreFile writer, and the sets of parameters keep changing, creating a lot of confusion, especially when porting patches across branches. The same thing is happening to HColumnDescriptor. I think we should move to a "builder pattern" solution, e.g.

        HFileWriter w = HFile.getWriterBuilder(conf, <some common args>)
            .setParameter1(value1)
            .setParameter2(value2)
            ...
            .build();
      

      Each parameter setter being on its own line will make merges/cherry-pick work properly, we will not have to even mention default parameters again, and we can eliminate a dozen impossible-to-remember constructors.

      This particular JIRA addresses the HColumnDescriptor refactoring. For StoreFile/HFile refactoring see HBASE-5442.

        Activity

        Hide
        Todd Lipcon added a comment -

        +1!

        Show
        Todd Lipcon added a comment - +1!
        Hide
        Phabricator added a comment -

        mbautin requested code review of "[jira] HBASE-5357 Refactoring: use the builder pattern for HColumnDescriptor".
        Reviewers: JIRA, todd, stack, tedyu, Kannan, Karthik, Liyin

        Deprecate three ugly HColumnDescriptor constructors and use a "builder" pattern instead, e.g. new HColumnDescriptor(cfName).setMaxVersions(5).setBlockSize(8192), etc. Setters have now become "builder" methods.

        TEST PLAN
        Run unit tests

        REVISION DETAIL
        https://reviews.facebook.net/D1851

        AFFECTED FILES
        src/main/java/org/apache/hadoop/hbase/HColumnDescriptor.java
        src/main/java/org/apache/hadoop/hbase/HTableDescriptor.java
        src/main/java/org/apache/hadoop/hbase/client/UnmodifyableHColumnDescriptor.java
        src/main/java/org/apache/hadoop/hbase/thrift/ThriftUtilities.java
        src/test/java/org/apache/hadoop/hbase/HBaseTestCase.java
        src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java
        src/test/java/org/apache/hadoop/hbase/TestSerialization.java
        src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java
        src/test/java/org/apache/hadoop/hbase/io/encoding/TestEncodedSeekers.java
        src/test/java/org/apache/hadoop/hbase/io/hfile/TestCacheOnWrite.java
        src/test/java/org/apache/hadoop/hbase/io/hfile/TestForceCacheImportantBlocks.java
        src/test/java/org/apache/hadoop/hbase/io/hfile/TestScannerSelectionUsingTTL.java
        src/test/java/org/apache/hadoop/hbase/mapreduce/TestHFileOutputFormat.java
        src/test/java/org/apache/hadoop/hbase/mapreduce/TestImportExport.java
        src/test/java/org/apache/hadoop/hbase/regionserver/TestBlocksRead.java
        src/test/java/org/apache/hadoop/hbase/regionserver/TestColumnSeeking.java
        src/test/java/org/apache/hadoop/hbase/regionserver/TestFSErrorsExposed.java
        src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java
        src/test/java/org/apache/hadoop/hbase/regionserver/TestMultiColumnScanner.java
        src/test/java/org/apache/hadoop/hbase/regionserver/TestScanWithBloomError.java
        src/test/java/org/apache/hadoop/hbase/regionserver/TestScanner.java
        src/test/java/org/apache/hadoop/hbase/regionserver/TestSeekOptimizations.java
        src/test/java/org/apache/hadoop/hbase/regionserver/TestWideScanner.java
        src/test/java/org/apache/hadoop/hbase/thrift2/TestThriftHBaseServiceHandler.java

        MANAGE HERALD DIFFERENTIAL RULES
        https://reviews.facebook.net/herald/view/differential/

        WHY DID I GET THIS EMAIL?
        https://reviews.facebook.net/herald/transcript/3933/

        Tip: use the X-Herald-Rules header to filter Herald messages in your client.

        Show
        Phabricator added a comment - mbautin requested code review of " [jira] HBASE-5357 Refactoring: use the builder pattern for HColumnDescriptor". Reviewers: JIRA, todd, stack, tedyu, Kannan, Karthik, Liyin Deprecate three ugly HColumnDescriptor constructors and use a "builder" pattern instead, e.g. new HColumnDescriptor(cfName).setMaxVersions(5).setBlockSize(8192), etc. Setters have now become "builder" methods. TEST PLAN Run unit tests REVISION DETAIL https://reviews.facebook.net/D1851 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/HColumnDescriptor.java src/main/java/org/apache/hadoop/hbase/HTableDescriptor.java src/main/java/org/apache/hadoop/hbase/client/UnmodifyableHColumnDescriptor.java src/main/java/org/apache/hadoop/hbase/thrift/ThriftUtilities.java src/test/java/org/apache/hadoop/hbase/HBaseTestCase.java src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java src/test/java/org/apache/hadoop/hbase/TestSerialization.java src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java src/test/java/org/apache/hadoop/hbase/io/encoding/TestEncodedSeekers.java src/test/java/org/apache/hadoop/hbase/io/hfile/TestCacheOnWrite.java src/test/java/org/apache/hadoop/hbase/io/hfile/TestForceCacheImportantBlocks.java src/test/java/org/apache/hadoop/hbase/io/hfile/TestScannerSelectionUsingTTL.java src/test/java/org/apache/hadoop/hbase/mapreduce/TestHFileOutputFormat.java src/test/java/org/apache/hadoop/hbase/mapreduce/TestImportExport.java src/test/java/org/apache/hadoop/hbase/regionserver/TestBlocksRead.java src/test/java/org/apache/hadoop/hbase/regionserver/TestColumnSeeking.java src/test/java/org/apache/hadoop/hbase/regionserver/TestFSErrorsExposed.java src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java src/test/java/org/apache/hadoop/hbase/regionserver/TestMultiColumnScanner.java src/test/java/org/apache/hadoop/hbase/regionserver/TestScanWithBloomError.java src/test/java/org/apache/hadoop/hbase/regionserver/TestScanner.java src/test/java/org/apache/hadoop/hbase/regionserver/TestSeekOptimizations.java src/test/java/org/apache/hadoop/hbase/regionserver/TestWideScanner.java src/test/java/org/apache/hadoop/hbase/thrift2/TestThriftHBaseServiceHandler.java MANAGE HERALD DIFFERENTIAL RULES https://reviews.facebook.net/herald/view/differential/ WHY DID I GET THIS EMAIL? https://reviews.facebook.net/herald/transcript/3933/ Tip: use the X-Herald-Rules header to filter Herald messages in your client.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12515534/Use-builder-pattern-for-HColumnDescriptor-2012-02-21_19_13_35.patch
        against trunk revision .

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

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

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

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

        -1 findbugs. The patch appears to introduce 159 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.regionserver.TestWideScanner

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1004//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1004//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1004//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/12515534/Use-builder-pattern-for-HColumnDescriptor-2012-02-21_19_13_35.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 78 new or modified tests. -1 javadoc. The javadoc tool appears to have generated -136 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 159 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.regionserver.TestWideScanner Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1004//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1004//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1004//console This message is automatically generated.
        Hide
        Phabricator added a comment -

        tedyu has commented on the revision "[jira] HBASE-5357 Refactoring: use the builder pattern for HColumnDescriptor".

        Patch looks good.

        INLINE COMMENTS
        src/main/java/org/apache/hadoop/hbase/HTableDescriptor.java:1119 Do we need 10 versions for ROOT table ?
        src/main/java/org/apache/hadoop/hbase/HTableDescriptor.java:1131 Do we need 10 versions for .META. table ?

        REVISION DETAIL
        https://reviews.facebook.net/D1851

        Show
        Phabricator added a comment - tedyu has commented on the revision " [jira] HBASE-5357 Refactoring: use the builder pattern for HColumnDescriptor". Patch looks good. INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/HTableDescriptor.java:1119 Do we need 10 versions for ROOT table ? src/main/java/org/apache/hadoop/hbase/HTableDescriptor.java:1131 Do we need 10 versions for .META. table ? REVISION DETAIL https://reviews.facebook.net/D1851
        Hide
        Phabricator added a comment -

        stack has commented on the revision "[jira] HBASE-5357 Refactoring: use the builder pattern for HColumnDescriptor".

        +1 on commit

        INLINE COMMENTS
        src/main/java/org/apache/hadoop/hbase/HColumnDescriptor.java:489 Want to add the @return? Might help with your Builder case if doc shows we're returning 'this'.
        src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java:851 Nice

        REVISION DETAIL
        https://reviews.facebook.net/D1851

        Show
        Phabricator added a comment - stack has commented on the revision " [jira] HBASE-5357 Refactoring: use the builder pattern for HColumnDescriptor". +1 on commit INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/HColumnDescriptor.java:489 Want to add the @return? Might help with your Builder case if doc shows we're returning 'this'. src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java:851 Nice REVISION DETAIL https://reviews.facebook.net/D1851
        Hide
        Phabricator added a comment -

        mbautin has commented on the revision "[jira] HBASE-5357 Refactoring: use the builder pattern for HColumnDescriptor".

        INLINE COMMENTS
        src/main/java/org/apache/hadoop/hbase/HTableDescriptor.java:1119 Not sure, but it is better to deal with changing the default number of versions in ROOT or META in a separate patch. This one is just a refactoring.
        src/main/java/org/apache/hadoop/hbase/HTableDescriptor.java:1131 See my comment above.
        src/main/java/org/apache/hadoop/hbase/HColumnDescriptor.java:489 Sure, I'll update setter javadocs.

        REVISION DETAIL
        https://reviews.facebook.net/D1851

        Show
        Phabricator added a comment - mbautin has commented on the revision " [jira] HBASE-5357 Refactoring: use the builder pattern for HColumnDescriptor". INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/HTableDescriptor.java:1119 Not sure, but it is better to deal with changing the default number of versions in ROOT or META in a separate patch. This one is just a refactoring. src/main/java/org/apache/hadoop/hbase/HTableDescriptor.java:1131 See my comment above. src/main/java/org/apache/hadoop/hbase/HColumnDescriptor.java:489 Sure, I'll update setter javadocs. REVISION DETAIL https://reviews.facebook.net/D1851
        Hide
        Phabricator added a comment -

        mbautin updated the revision "[jira] HBASE-5357 Refactoring: use the builder pattern for HColumnDescriptor".
        Reviewers: JIRA, todd, stack, tedyu, Kannan, Karthik, Liyin

        Addressing Stack's comment.

        REVISION DETAIL
        https://reviews.facebook.net/D1851

        AFFECTED FILES
        src/main/java/org/apache/hadoop/hbase/HColumnDescriptor.java
        src/main/java/org/apache/hadoop/hbase/HTableDescriptor.java
        src/main/java/org/apache/hadoop/hbase/client/UnmodifyableHColumnDescriptor.java
        src/main/java/org/apache/hadoop/hbase/thrift/ThriftUtilities.java
        src/test/java/org/apache/hadoop/hbase/HBaseTestCase.java
        src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java
        src/test/java/org/apache/hadoop/hbase/TestSerialization.java
        src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java
        src/test/java/org/apache/hadoop/hbase/io/encoding/TestEncodedSeekers.java
        src/test/java/org/apache/hadoop/hbase/io/hfile/TestCacheOnWrite.java
        src/test/java/org/apache/hadoop/hbase/io/hfile/TestForceCacheImportantBlocks.java
        src/test/java/org/apache/hadoop/hbase/io/hfile/TestScannerSelectionUsingTTL.java
        src/test/java/org/apache/hadoop/hbase/mapreduce/TestHFileOutputFormat.java
        src/test/java/org/apache/hadoop/hbase/mapreduce/TestImportExport.java
        src/test/java/org/apache/hadoop/hbase/regionserver/TestBlocksRead.java
        src/test/java/org/apache/hadoop/hbase/regionserver/TestColumnSeeking.java
        src/test/java/org/apache/hadoop/hbase/regionserver/TestFSErrorsExposed.java
        src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java
        src/test/java/org/apache/hadoop/hbase/regionserver/TestMultiColumnScanner.java
        src/test/java/org/apache/hadoop/hbase/regionserver/TestScanWithBloomError.java
        src/test/java/org/apache/hadoop/hbase/regionserver/TestScanner.java
        src/test/java/org/apache/hadoop/hbase/regionserver/TestSeekOptimizations.java
        src/test/java/org/apache/hadoop/hbase/regionserver/TestWideScanner.java
        src/test/java/org/apache/hadoop/hbase/thrift2/TestThriftHBaseServiceHandler.java

        Show
        Phabricator added a comment - mbautin updated the revision " [jira] HBASE-5357 Refactoring: use the builder pattern for HColumnDescriptor". Reviewers: JIRA, todd, stack, tedyu, Kannan, Karthik, Liyin Addressing Stack's comment. REVISION DETAIL https://reviews.facebook.net/D1851 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/HColumnDescriptor.java src/main/java/org/apache/hadoop/hbase/HTableDescriptor.java src/main/java/org/apache/hadoop/hbase/client/UnmodifyableHColumnDescriptor.java src/main/java/org/apache/hadoop/hbase/thrift/ThriftUtilities.java src/test/java/org/apache/hadoop/hbase/HBaseTestCase.java src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java src/test/java/org/apache/hadoop/hbase/TestSerialization.java src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java src/test/java/org/apache/hadoop/hbase/io/encoding/TestEncodedSeekers.java src/test/java/org/apache/hadoop/hbase/io/hfile/TestCacheOnWrite.java src/test/java/org/apache/hadoop/hbase/io/hfile/TestForceCacheImportantBlocks.java src/test/java/org/apache/hadoop/hbase/io/hfile/TestScannerSelectionUsingTTL.java src/test/java/org/apache/hadoop/hbase/mapreduce/TestHFileOutputFormat.java src/test/java/org/apache/hadoop/hbase/mapreduce/TestImportExport.java src/test/java/org/apache/hadoop/hbase/regionserver/TestBlocksRead.java src/test/java/org/apache/hadoop/hbase/regionserver/TestColumnSeeking.java src/test/java/org/apache/hadoop/hbase/regionserver/TestFSErrorsExposed.java src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java src/test/java/org/apache/hadoop/hbase/regionserver/TestMultiColumnScanner.java src/test/java/org/apache/hadoop/hbase/regionserver/TestScanWithBloomError.java src/test/java/org/apache/hadoop/hbase/regionserver/TestScanner.java src/test/java/org/apache/hadoop/hbase/regionserver/TestSeekOptimizations.java src/test/java/org/apache/hadoop/hbase/regionserver/TestWideScanner.java src/test/java/org/apache/hadoop/hbase/thrift2/TestThriftHBaseServiceHandler.java
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12515626/D1851.2.patch
        against trunk revision .

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

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

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

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

        -1 findbugs. The patch appears to introduce 151 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.regionserver.TestWideScanner

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1012//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1012//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1012//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/12515626/D1851.2.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 60 new or modified tests. -1 javadoc. The javadoc tool appears to have generated -136 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 151 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.regionserver.TestWideScanner Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1012//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1012//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1012//console This message is automatically generated.
        Hide
        Phabricator added a comment -

        mbautin updated the revision "[jira] HBASE-5357 Refactoring: use the builder pattern for HColumnDescriptor".
        Reviewers: JIRA, todd, stack, tedyu, Kannan, Karthik, Liyin

        Rebasing on trunk changes.

        REVISION DETAIL
        https://reviews.facebook.net/D1851

        AFFECTED FILES
        src/main/java/org/apache/hadoop/hbase/HColumnDescriptor.java
        src/main/java/org/apache/hadoop/hbase/HTableDescriptor.java
        src/main/java/org/apache/hadoop/hbase/client/UnmodifyableHColumnDescriptor.java
        src/main/java/org/apache/hadoop/hbase/thrift/ThriftUtilities.java
        src/test/java/org/apache/hadoop/hbase/HBaseTestCase.java
        src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java
        src/test/java/org/apache/hadoop/hbase/TestSerialization.java
        src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java
        src/test/java/org/apache/hadoop/hbase/io/encoding/TestEncodedSeekers.java
        src/test/java/org/apache/hadoop/hbase/io/hfile/TestCacheOnWrite.java
        src/test/java/org/apache/hadoop/hbase/io/hfile/TestForceCacheImportantBlocks.java
        src/test/java/org/apache/hadoop/hbase/io/hfile/TestScannerSelectionUsingTTL.java
        src/test/java/org/apache/hadoop/hbase/mapreduce/TestHFileOutputFormat.java
        src/test/java/org/apache/hadoop/hbase/mapreduce/TestImportExport.java
        src/test/java/org/apache/hadoop/hbase/regionserver/TestBlocksRead.java
        src/test/java/org/apache/hadoop/hbase/regionserver/TestColumnSeeking.java
        src/test/java/org/apache/hadoop/hbase/regionserver/TestFSErrorsExposed.java
        src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java
        src/test/java/org/apache/hadoop/hbase/regionserver/TestMultiColumnScanner.java
        src/test/java/org/apache/hadoop/hbase/regionserver/TestScanWithBloomError.java
        src/test/java/org/apache/hadoop/hbase/regionserver/TestScanner.java
        src/test/java/org/apache/hadoop/hbase/regionserver/TestSeekOptimizations.java
        src/test/java/org/apache/hadoop/hbase/regionserver/TestWideScanner.java
        src/test/java/org/apache/hadoop/hbase/thrift2/TestThriftHBaseServiceHandler.java

        Show
        Phabricator added a comment - mbautin updated the revision " [jira] HBASE-5357 Refactoring: use the builder pattern for HColumnDescriptor". Reviewers: JIRA, todd, stack, tedyu, Kannan, Karthik, Liyin Rebasing on trunk changes. REVISION DETAIL https://reviews.facebook.net/D1851 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/HColumnDescriptor.java src/main/java/org/apache/hadoop/hbase/HTableDescriptor.java src/main/java/org/apache/hadoop/hbase/client/UnmodifyableHColumnDescriptor.java src/main/java/org/apache/hadoop/hbase/thrift/ThriftUtilities.java src/test/java/org/apache/hadoop/hbase/HBaseTestCase.java src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java src/test/java/org/apache/hadoop/hbase/TestSerialization.java src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java src/test/java/org/apache/hadoop/hbase/io/encoding/TestEncodedSeekers.java src/test/java/org/apache/hadoop/hbase/io/hfile/TestCacheOnWrite.java src/test/java/org/apache/hadoop/hbase/io/hfile/TestForceCacheImportantBlocks.java src/test/java/org/apache/hadoop/hbase/io/hfile/TestScannerSelectionUsingTTL.java src/test/java/org/apache/hadoop/hbase/mapreduce/TestHFileOutputFormat.java src/test/java/org/apache/hadoop/hbase/mapreduce/TestImportExport.java src/test/java/org/apache/hadoop/hbase/regionserver/TestBlocksRead.java src/test/java/org/apache/hadoop/hbase/regionserver/TestColumnSeeking.java src/test/java/org/apache/hadoop/hbase/regionserver/TestFSErrorsExposed.java src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java src/test/java/org/apache/hadoop/hbase/regionserver/TestMultiColumnScanner.java src/test/java/org/apache/hadoop/hbase/regionserver/TestScanWithBloomError.java src/test/java/org/apache/hadoop/hbase/regionserver/TestScanner.java src/test/java/org/apache/hadoop/hbase/regionserver/TestSeekOptimizations.java src/test/java/org/apache/hadoop/hbase/regionserver/TestWideScanner.java src/test/java/org/apache/hadoop/hbase/thrift2/TestThriftHBaseServiceHandler.java
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12515781/D1851.3.patch
        against trunk revision .

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

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

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

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

        -1 findbugs. The patch appears to introduce 152 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.regionserver.TestWideScanner

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1025//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1025//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1025//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/12515781/D1851.3.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 60 new or modified tests. -1 javadoc. The javadoc tool appears to have generated -136 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 152 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.regionserver.TestWideScanner Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1025//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1025//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1025//console This message is automatically generated.
        Hide
        Phabricator added a comment -

        mbautin updated the revision "[jira] HBASE-5357 Refactoring: use the builder pattern for HColumnDescriptor".
        Reviewers: JIRA, todd, stack, tedyu, Kannan, Karthik, Liyin

        Fix TestWideScanner failure.

        REVISION DETAIL
        https://reviews.facebook.net/D1851

        AFFECTED FILES
        src/main/java/org/apache/hadoop/hbase/HColumnDescriptor.java
        src/main/java/org/apache/hadoop/hbase/HTableDescriptor.java
        src/main/java/org/apache/hadoop/hbase/client/UnmodifyableHColumnDescriptor.java
        src/main/java/org/apache/hadoop/hbase/thrift/ThriftUtilities.java
        src/test/java/org/apache/hadoop/hbase/HBaseTestCase.java
        src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java
        src/test/java/org/apache/hadoop/hbase/TestSerialization.java
        src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java
        src/test/java/org/apache/hadoop/hbase/io/encoding/TestEncodedSeekers.java
        src/test/java/org/apache/hadoop/hbase/io/hfile/TestCacheOnWrite.java
        src/test/java/org/apache/hadoop/hbase/io/hfile/TestForceCacheImportantBlocks.java
        src/test/java/org/apache/hadoop/hbase/io/hfile/TestScannerSelectionUsingTTL.java
        src/test/java/org/apache/hadoop/hbase/mapreduce/TestHFileOutputFormat.java
        src/test/java/org/apache/hadoop/hbase/mapreduce/TestImportExport.java
        src/test/java/org/apache/hadoop/hbase/regionserver/TestBlocksRead.java
        src/test/java/org/apache/hadoop/hbase/regionserver/TestColumnSeeking.java
        src/test/java/org/apache/hadoop/hbase/regionserver/TestFSErrorsExposed.java
        src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java
        src/test/java/org/apache/hadoop/hbase/regionserver/TestMultiColumnScanner.java
        src/test/java/org/apache/hadoop/hbase/regionserver/TestScanWithBloomError.java
        src/test/java/org/apache/hadoop/hbase/regionserver/TestScanner.java
        src/test/java/org/apache/hadoop/hbase/regionserver/TestSeekOptimizations.java
        src/test/java/org/apache/hadoop/hbase/regionserver/TestWideScanner.java
        src/test/java/org/apache/hadoop/hbase/thrift2/TestThriftHBaseServiceHandler.java

        Show
        Phabricator added a comment - mbautin updated the revision " [jira] HBASE-5357 Refactoring: use the builder pattern for HColumnDescriptor". Reviewers: JIRA, todd, stack, tedyu, Kannan, Karthik, Liyin Fix TestWideScanner failure. REVISION DETAIL https://reviews.facebook.net/D1851 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/HColumnDescriptor.java src/main/java/org/apache/hadoop/hbase/HTableDescriptor.java src/main/java/org/apache/hadoop/hbase/client/UnmodifyableHColumnDescriptor.java src/main/java/org/apache/hadoop/hbase/thrift/ThriftUtilities.java src/test/java/org/apache/hadoop/hbase/HBaseTestCase.java src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java src/test/java/org/apache/hadoop/hbase/TestSerialization.java src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java src/test/java/org/apache/hadoop/hbase/io/encoding/TestEncodedSeekers.java src/test/java/org/apache/hadoop/hbase/io/hfile/TestCacheOnWrite.java src/test/java/org/apache/hadoop/hbase/io/hfile/TestForceCacheImportantBlocks.java src/test/java/org/apache/hadoop/hbase/io/hfile/TestScannerSelectionUsingTTL.java src/test/java/org/apache/hadoop/hbase/mapreduce/TestHFileOutputFormat.java src/test/java/org/apache/hadoop/hbase/mapreduce/TestImportExport.java src/test/java/org/apache/hadoop/hbase/regionserver/TestBlocksRead.java src/test/java/org/apache/hadoop/hbase/regionserver/TestColumnSeeking.java src/test/java/org/apache/hadoop/hbase/regionserver/TestFSErrorsExposed.java src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java src/test/java/org/apache/hadoop/hbase/regionserver/TestMultiColumnScanner.java src/test/java/org/apache/hadoop/hbase/regionserver/TestScanWithBloomError.java src/test/java/org/apache/hadoop/hbase/regionserver/TestScanner.java src/test/java/org/apache/hadoop/hbase/regionserver/TestSeekOptimizations.java src/test/java/org/apache/hadoop/hbase/regionserver/TestWideScanner.java src/test/java/org/apache/hadoop/hbase/thrift2/TestThriftHBaseServiceHandler.java
        Hide
        Phabricator added a comment -

        mbautin has commented on the revision "[jira] HBASE-5357 Refactoring: use the builder pattern for HColumnDescriptor".

        This should pass all unit tests now (re-running internally as well as on Hadoop QA). Someone please accept this patch if there are no additional comments. Thanks!

        REVISION DETAIL
        https://reviews.facebook.net/D1851

        Show
        Phabricator added a comment - mbautin has commented on the revision " [jira] HBASE-5357 Refactoring: use the builder pattern for HColumnDescriptor". This should pass all unit tests now (re-running internally as well as on Hadoop QA). Someone please accept this patch if there are no additional comments. Thanks! REVISION DETAIL https://reviews.facebook.net/D1851
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12515787/Use-builder-pattern-for-HColumnDescriptor-20120223113155-e387d251.patch
        against trunk revision .

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

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

        -1 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1026//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/12515787/Use-builder-pattern-for-HColumnDescriptor-20120223113155-e387d251.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 78 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1026//console This message is automatically generated.
        Hide
        Ted Yu added a comment -

        @Mikhail:
        Can you attach patch generated with --no-prefix ?

        Show
        Ted Yu added a comment - @Mikhail: Can you attach patch generated with --no-prefix ?
        Hide
        Phabricator added a comment -

        Kannan has commented on the revision "[jira] HBASE-5357 Refactoring: use the builder pattern for HColumnDescriptor".

        nice change-- the unit tests are a lot more readable now! One inlined comment...

        INLINE COMMENTS
        src/main/java/org/apache/hadoop/hbase/HColumnDescriptor.java:438 changing the return type can break some existing apps out there, no?

        REVISION DETAIL
        https://reviews.facebook.net/D1851

        Show
        Phabricator added a comment - Kannan has commented on the revision " [jira] HBASE-5357 Refactoring: use the builder pattern for HColumnDescriptor". nice change-- the unit tests are a lot more readable now! One inlined comment... INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/HColumnDescriptor.java:438 changing the return type can break some existing apps out there, no? REVISION DETAIL https://reviews.facebook.net/D1851
        Hide
        Mikhail Bautin added a comment -

        @Ted: I thought that was what I did. Attaching again.

        Show
        Mikhail Bautin added a comment - @Ted: I thought that was what I did. Attaching again.
        Hide
        Phabricator added a comment -

        mbautin has commented on the revision "[jira] HBASE-5357 Refactoring: use the builder pattern for HColumnDescriptor".

        The patch passed all unit tests in my map-reduce run, except TestAtomicOperation, which passed locally. Still waiting for the Hadoop QA run.

        INLINE COMMENTS
        src/main/java/org/apache/hadoop/hbase/HColumnDescriptor.java:438 I don't think this can break existing code. The return value will most likely be simply ignored.

        From http://docs.oracle.com/javase/tutorial/java/javaOO/methods.html:

        "You cannot declare more than one method with the same name and the same number and type of arguments, because the compiler cannot tell them apart.

        The compiler does not consider return type when differentiating methods, so you cannot declare two methods with the same signature even if they have a different return type."

        REVISION DETAIL
        https://reviews.facebook.net/D1851

        Show
        Phabricator added a comment - mbautin has commented on the revision " [jira] HBASE-5357 Refactoring: use the builder pattern for HColumnDescriptor". The patch passed all unit tests in my map-reduce run, except TestAtomicOperation, which passed locally. Still waiting for the Hadoop QA run. INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/HColumnDescriptor.java:438 I don't think this can break existing code. The return value will most likely be simply ignored. From http://docs.oracle.com/javase/tutorial/java/javaOO/methods.html: "You cannot declare more than one method with the same name and the same number and type of arguments, because the compiler cannot tell them apart. The compiler does not consider return type when differentiating methods, so you cannot declare two methods with the same signature even if they have a different return type." REVISION DETAIL https://reviews.facebook.net/D1851
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12515810/Use-builder-pattern-for-HColumnDescriptor-2012-02-23_12_42_49.patch
        against trunk revision .

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

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

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

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

        -1 findbugs. The patch appears to introduce 152 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.mapreduce.TestHFileOutputFormat
        org.apache.hadoop.hbase.mapred.TestTableMapReduce
        org.apache.hadoop.hbase.mapreduce.TestImportTsv

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1028//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1028//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1028//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/12515810/Use-builder-pattern-for-HColumnDescriptor-2012-02-23_12_42_49.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 78 new or modified tests. -1 javadoc. The javadoc tool appears to have generated -136 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 152 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.mapreduce.TestHFileOutputFormat org.apache.hadoop.hbase.mapred.TestTableMapReduce org.apache.hadoop.hbase.mapreduce.TestImportTsv Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1028//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1028//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1028//console This message is automatically generated.
        Hide
        Phabricator added a comment -

        tedyu has accepted the revision "[jira] HBASE-5357 Refactoring: use the builder pattern for HColumnDescriptor".

        I see 'This diff has Lint Problems.' because of Lint being skipped.

        REVISION DETAIL
        https://reviews.facebook.net/D1851

        BRANCH
        hcd_builder2

        Show
        Phabricator added a comment - tedyu has accepted the revision " [jira] HBASE-5357 Refactoring: use the builder pattern for HColumnDescriptor". I see 'This diff has Lint Problems.' because of Lint being skipped. REVISION DETAIL https://reviews.facebook.net/D1851 BRANCH hcd_builder2
        Hide
        Mikhail Bautin added a comment -

        Re-ran failed unit tests locally:

        Running org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat
        Tests run: 9, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 211.925 sec
        Running org.apache.hadoop.hbase.mapreduce.TestImportTsv
        Tests run: 8, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 81.352 sec
        Running org.apache.hadoop.hbase.mapreduce.TestTableMapReduce
        Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 105.09 sec
        Running org.apache.hadoop.hbase.mapred.TestTableMapReduce
        Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 68.055 sec

        Results :

        Tests run: 19, Failures: 0, Errors: 0, Skipped: 0

        Show
        Mikhail Bautin added a comment - Re-ran failed unit tests locally: Running org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat Tests run: 9, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 211.925 sec Running org.apache.hadoop.hbase.mapreduce.TestImportTsv Tests run: 8, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 81.352 sec Running org.apache.hadoop.hbase.mapreduce.TestTableMapReduce Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 105.09 sec Running org.apache.hadoop.hbase.mapred.TestTableMapReduce Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 68.055 sec Results : Tests run: 19, Failures: 0, Errors: 0, Skipped: 0
        Hide
        Phabricator added a comment -

        mbautin has commented on the revision "[jira] HBASE-5357 Refactoring: use the builder pattern for HColumnDescriptor".

        @tedyu: as far as I know, we don't yet have a consistent Phabricator lint configuration for the HBase repository.

        REVISION DETAIL
        https://reviews.facebook.net/D1851

        BRANCH
        hcd_builder2

        Show
        Phabricator added a comment - mbautin has commented on the revision " [jira] HBASE-5357 Refactoring: use the builder pattern for HColumnDescriptor". @tedyu: as far as I know, we don't yet have a consistent Phabricator lint configuration for the HBase repository. REVISION DETAIL https://reviews.facebook.net/D1851 BRANCH hcd_builder2
        Hide
        Phabricator added a comment -

        mbautin has committed the revision "[jira] HBASE-5357 Refactoring: use the builder pattern for HColumnDescriptor".

        REVISION DETAIL
        https://reviews.facebook.net/D1851

        COMMIT
        https://reviews.facebook.net/rHBASE1293026

        Show
        Phabricator added a comment - mbautin has committed the revision " [jira] HBASE-5357 Refactoring: use the builder pattern for HColumnDescriptor". REVISION DETAIL https://reviews.facebook.net/D1851 COMMIT https://reviews.facebook.net/rHBASE1293026
        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK #2668 (See https://builds.apache.org/job/HBase-TRUNK/2668/)
        [jira] HBASE-5357 Refactoring: use the builder pattern for HColumnDescriptor

        Summary: Deprecate three ugly HColumnDescriptor constructors and use a "builder"
        pattern instead, e.g. new
        HColumnDescriptor(cfName).setMaxVersions(5).setBlockSize(8192), etc. Setters
        have now become "builder" methods.

        Test Plan: Run unit tests

        Reviewers: JIRA, todd, stack, tedyu, Kannan, Karthik, Liyin

        Reviewed By: tedyu

        Differential Revision: https://reviews.facebook.net/D1851 (Revision 1293026)

        Result = SUCCESS
        mbautin :
        Files :

        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/HColumnDescriptor.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/HTableDescriptor.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/UnmodifyableHColumnDescriptor.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift/ThriftUtilities.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/HBaseTestCase.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/TestSerialization.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/io/encoding/TestEncodedSeekers.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/io/hfile/TestCacheOnWrite.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/io/hfile/TestForceCacheImportantBlocks.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/io/hfile/TestScannerSelectionUsingTTL.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/mapreduce/TestHFileOutputFormat.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/mapreduce/TestImportExport.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestBlocksRead.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestColumnSeeking.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestFSErrorsExposed.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestMultiColumnScanner.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestScanWithBloomError.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestScanner.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestSeekOptimizations.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestWideScanner.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/thrift2/TestThriftHBaseServiceHandler.java
        Show
        Hudson added a comment - Integrated in HBase-TRUNK #2668 (See https://builds.apache.org/job/HBase-TRUNK/2668/ ) [jira] HBASE-5357 Refactoring: use the builder pattern for HColumnDescriptor Summary: Deprecate three ugly HColumnDescriptor constructors and use a "builder" pattern instead, e.g. new HColumnDescriptor(cfName).setMaxVersions(5).setBlockSize(8192), etc. Setters have now become "builder" methods. Test Plan: Run unit tests Reviewers: JIRA, todd, stack, tedyu, Kannan, Karthik, Liyin Reviewed By: tedyu Differential Revision: https://reviews.facebook.net/D1851 (Revision 1293026) Result = SUCCESS mbautin : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/HColumnDescriptor.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/HTableDescriptor.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/UnmodifyableHColumnDescriptor.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift/ThriftUtilities.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/HBaseTestCase.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/TestSerialization.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/io/encoding/TestEncodedSeekers.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/io/hfile/TestCacheOnWrite.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/io/hfile/TestForceCacheImportantBlocks.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/io/hfile/TestScannerSelectionUsingTTL.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/mapreduce/TestHFileOutputFormat.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/mapreduce/TestImportExport.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestBlocksRead.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestColumnSeeking.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestFSErrorsExposed.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestMultiColumnScanner.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestScanWithBloomError.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestScanner.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestSeekOptimizations.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestWideScanner.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/thrift2/TestThriftHBaseServiceHandler.java
        Hide
        Phabricator added a comment -

        mbautin requested code review of "[jira] HBASE-5357 [89-fb] Refactoring: use the builder pattern for HColumnDescriptor".
        Reviewers: Kannan, Liyin, Karthik, JIRA

        Deprecate three ugly HColumnDescriptor constructors and use a "builder" pattern instead, e.g. new HColumnDescriptor(cfName).setMaxVersions(5).setBlockSize(8192), etc. Setters have now become "builder" methods.

        This is the 89-fb version of D1851.

        TEST PLAN
        Run unit tests

        REVISION DETAIL
        https://reviews.facebook.net/D1929

        AFFECTED FILES
        src/main/java/org/apache/hadoop/hbase/HColumnDescriptor.java
        src/main/java/org/apache/hadoop/hbase/HTableDescriptor.java
        src/main/java/org/apache/hadoop/hbase/client/UnmodifyableHColumnDescriptor.java
        src/main/java/org/apache/hadoop/hbase/thrift/ThriftUtilities.java
        src/test/java/org/apache/hadoop/hbase/HBaseTestCase.java
        src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java
        src/test/java/org/apache/hadoop/hbase/TestSerialization.java
        src/test/java/org/apache/hadoop/hbase/io/encoding/TestEncodedSeekers.java
        src/test/java/org/apache/hadoop/hbase/io/hfile/TestCacheOnWrite.java
        src/test/java/org/apache/hadoop/hbase/io/hfile/TestForceCacheImportantBlocks.java
        src/test/java/org/apache/hadoop/hbase/io/hfile/TestScannerSelectionUsingTTL.java
        src/test/java/org/apache/hadoop/hbase/mapreduce/TestHFileOutputFormat.java
        src/test/java/org/apache/hadoop/hbase/regionserver/TestBlocksRead.java
        src/test/java/org/apache/hadoop/hbase/regionserver/TestColumnSeeking.java
        src/test/java/org/apache/hadoop/hbase/regionserver/TestFSErrorsExposed.java
        src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java
        src/test/java/org/apache/hadoop/hbase/regionserver/TestMultiColumnScanner.java
        src/test/java/org/apache/hadoop/hbase/regionserver/TestScanWithBloomError.java
        src/test/java/org/apache/hadoop/hbase/regionserver/TestScanner.java
        src/test/java/org/apache/hadoop/hbase/regionserver/TestSeekOptimizations.java
        src/test/java/org/apache/hadoop/hbase/regionserver/TestWideScanner.java

        Show
        Phabricator added a comment - mbautin requested code review of " [jira] HBASE-5357 [89-fb] Refactoring: use the builder pattern for HColumnDescriptor". Reviewers: Kannan, Liyin, Karthik, JIRA Deprecate three ugly HColumnDescriptor constructors and use a "builder" pattern instead, e.g. new HColumnDescriptor(cfName).setMaxVersions(5).setBlockSize(8192), etc. Setters have now become "builder" methods. This is the 89-fb version of D1851. TEST PLAN Run unit tests REVISION DETAIL https://reviews.facebook.net/D1929 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/HColumnDescriptor.java src/main/java/org/apache/hadoop/hbase/HTableDescriptor.java src/main/java/org/apache/hadoop/hbase/client/UnmodifyableHColumnDescriptor.java src/main/java/org/apache/hadoop/hbase/thrift/ThriftUtilities.java src/test/java/org/apache/hadoop/hbase/HBaseTestCase.java src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java src/test/java/org/apache/hadoop/hbase/TestSerialization.java src/test/java/org/apache/hadoop/hbase/io/encoding/TestEncodedSeekers.java src/test/java/org/apache/hadoop/hbase/io/hfile/TestCacheOnWrite.java src/test/java/org/apache/hadoop/hbase/io/hfile/TestForceCacheImportantBlocks.java src/test/java/org/apache/hadoop/hbase/io/hfile/TestScannerSelectionUsingTTL.java src/test/java/org/apache/hadoop/hbase/mapreduce/TestHFileOutputFormat.java src/test/java/org/apache/hadoop/hbase/regionserver/TestBlocksRead.java src/test/java/org/apache/hadoop/hbase/regionserver/TestColumnSeeking.java src/test/java/org/apache/hadoop/hbase/regionserver/TestFSErrorsExposed.java src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java src/test/java/org/apache/hadoop/hbase/regionserver/TestMultiColumnScanner.java src/test/java/org/apache/hadoop/hbase/regionserver/TestScanWithBloomError.java src/test/java/org/apache/hadoop/hbase/regionserver/TestScanner.java src/test/java/org/apache/hadoop/hbase/regionserver/TestSeekOptimizations.java src/test/java/org/apache/hadoop/hbase/regionserver/TestWideScanner.java
        Hide
        Hadoop QA added a comment -

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

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

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

        -1 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1036//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/12515861/D1929.1.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 51 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1036//console This message is automatically generated.
        Hide
        Mikhail Bautin added a comment -

        Committed to trunk. 89-fb patch pending review.

        Show
        Mikhail Bautin added a comment - Committed to trunk. 89-fb patch pending review.
        Hide
        Phabricator added a comment -

        mbautin updated the revision "[jira] HBASE-5357 [89-fb] Refactoring: use the builder pattern for HColumnDescriptor".
        Reviewers: Kannan, Liyin, Karthik, JIRA

        Replacing a few remaining invocations of deprecated HColumnDescriptor constructors with the new builder pattern. Those invocations do not seem to be present in trunk.

        REVISION DETAIL
        https://reviews.facebook.net/D1929

        AFFECTED FILES
        src/main/java/org/apache/hadoop/hbase/HColumnDescriptor.java
        src/main/java/org/apache/hadoop/hbase/HTableDescriptor.java
        src/main/java/org/apache/hadoop/hbase/client/UnmodifyableHColumnDescriptor.java
        src/main/java/org/apache/hadoop/hbase/thrift/ThriftUtilities.java
        src/test/java/org/apache/hadoop/hbase/HBaseTestCase.java
        src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java
        src/test/java/org/apache/hadoop/hbase/TestSerialization.java
        src/test/java/org/apache/hadoop/hbase/io/encoding/TestEncodedSeekers.java
        src/test/java/org/apache/hadoop/hbase/io/hfile/TestCacheOnWrite.java
        src/test/java/org/apache/hadoop/hbase/io/hfile/TestForceCacheImportantBlocks.java
        src/test/java/org/apache/hadoop/hbase/io/hfile/TestScannerSelectionUsingTTL.java
        src/test/java/org/apache/hadoop/hbase/loadtest/HBaseUtils.java
        src/test/java/org/apache/hadoop/hbase/mapreduce/TestHFileOutputFormat.java
        src/test/java/org/apache/hadoop/hbase/regionserver/TestBlocksRead.java
        src/test/java/org/apache/hadoop/hbase/regionserver/TestColumnSeeking.java
        src/test/java/org/apache/hadoop/hbase/regionserver/TestFSErrorsExposed.java
        src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java
        src/test/java/org/apache/hadoop/hbase/regionserver/TestMultiColumnScanner.java
        src/test/java/org/apache/hadoop/hbase/regionserver/TestScanWithBloomError.java
        src/test/java/org/apache/hadoop/hbase/regionserver/TestScanner.java
        src/test/java/org/apache/hadoop/hbase/regionserver/TestScannerResets.java
        src/test/java/org/apache/hadoop/hbase/regionserver/TestSeekOptimizations.java
        src/test/java/org/apache/hadoop/hbase/regionserver/TestWideScanner.java

        Show
        Phabricator added a comment - mbautin updated the revision " [jira] HBASE-5357 [89-fb] Refactoring: use the builder pattern for HColumnDescriptor". Reviewers: Kannan, Liyin, Karthik, JIRA Replacing a few remaining invocations of deprecated HColumnDescriptor constructors with the new builder pattern. Those invocations do not seem to be present in trunk. REVISION DETAIL https://reviews.facebook.net/D1929 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/HColumnDescriptor.java src/main/java/org/apache/hadoop/hbase/HTableDescriptor.java src/main/java/org/apache/hadoop/hbase/client/UnmodifyableHColumnDescriptor.java src/main/java/org/apache/hadoop/hbase/thrift/ThriftUtilities.java src/test/java/org/apache/hadoop/hbase/HBaseTestCase.java src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java src/test/java/org/apache/hadoop/hbase/TestSerialization.java src/test/java/org/apache/hadoop/hbase/io/encoding/TestEncodedSeekers.java src/test/java/org/apache/hadoop/hbase/io/hfile/TestCacheOnWrite.java src/test/java/org/apache/hadoop/hbase/io/hfile/TestForceCacheImportantBlocks.java src/test/java/org/apache/hadoop/hbase/io/hfile/TestScannerSelectionUsingTTL.java src/test/java/org/apache/hadoop/hbase/loadtest/HBaseUtils.java src/test/java/org/apache/hadoop/hbase/mapreduce/TestHFileOutputFormat.java src/test/java/org/apache/hadoop/hbase/regionserver/TestBlocksRead.java src/test/java/org/apache/hadoop/hbase/regionserver/TestColumnSeeking.java src/test/java/org/apache/hadoop/hbase/regionserver/TestFSErrorsExposed.java src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java src/test/java/org/apache/hadoop/hbase/regionserver/TestMultiColumnScanner.java src/test/java/org/apache/hadoop/hbase/regionserver/TestScanWithBloomError.java src/test/java/org/apache/hadoop/hbase/regionserver/TestScanner.java src/test/java/org/apache/hadoop/hbase/regionserver/TestScannerResets.java src/test/java/org/apache/hadoop/hbase/regionserver/TestSeekOptimizations.java src/test/java/org/apache/hadoop/hbase/regionserver/TestWideScanner.java
        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK-security #121 (See https://builds.apache.org/job/HBase-TRUNK-security/121/)
        [jira] HBASE-5357 Refactoring: use the builder pattern for HColumnDescriptor

        Summary: Deprecate three ugly HColumnDescriptor constructors and use a "builder"
        pattern instead, e.g. new
        HColumnDescriptor(cfName).setMaxVersions(5).setBlockSize(8192), etc. Setters
        have now become "builder" methods.

        Test Plan: Run unit tests

        Reviewers: JIRA, todd, stack, tedyu, Kannan, Karthik, Liyin

        Reviewed By: tedyu

        Differential Revision: https://reviews.facebook.net/D1851 (Revision 1293026)

        Result = SUCCESS
        mbautin :
        Files :

        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/HColumnDescriptor.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/HTableDescriptor.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/UnmodifyableHColumnDescriptor.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift/ThriftUtilities.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/HBaseTestCase.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/TestSerialization.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/io/encoding/TestEncodedSeekers.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/io/hfile/TestCacheOnWrite.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/io/hfile/TestForceCacheImportantBlocks.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/io/hfile/TestScannerSelectionUsingTTL.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/mapreduce/TestHFileOutputFormat.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/mapreduce/TestImportExport.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestBlocksRead.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestColumnSeeking.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestFSErrorsExposed.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestMultiColumnScanner.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestScanWithBloomError.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestScanner.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestSeekOptimizations.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestWideScanner.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/thrift2/TestThriftHBaseServiceHandler.java
        Show
        Hudson added a comment - Integrated in HBase-TRUNK-security #121 (See https://builds.apache.org/job/HBase-TRUNK-security/121/ ) [jira] HBASE-5357 Refactoring: use the builder pattern for HColumnDescriptor Summary: Deprecate three ugly HColumnDescriptor constructors and use a "builder" pattern instead, e.g. new HColumnDescriptor(cfName).setMaxVersions(5).setBlockSize(8192), etc. Setters have now become "builder" methods. Test Plan: Run unit tests Reviewers: JIRA, todd, stack, tedyu, Kannan, Karthik, Liyin Reviewed By: tedyu Differential Revision: https://reviews.facebook.net/D1851 (Revision 1293026) Result = SUCCESS mbautin : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/HColumnDescriptor.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/HTableDescriptor.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/UnmodifyableHColumnDescriptor.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift/ThriftUtilities.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/HBaseTestCase.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/TestSerialization.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/io/encoding/TestEncodedSeekers.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/io/hfile/TestCacheOnWrite.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/io/hfile/TestForceCacheImportantBlocks.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/io/hfile/TestScannerSelectionUsingTTL.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/mapreduce/TestHFileOutputFormat.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/mapreduce/TestImportExport.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestBlocksRead.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestColumnSeeking.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestFSErrorsExposed.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestMultiColumnScanner.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestScanWithBloomError.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestScanner.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestSeekOptimizations.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestWideScanner.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/thrift2/TestThriftHBaseServiceHandler.java
        Hide
        Phabricator added a comment -

        Kannan has commented on the revision "[jira] HBASE-5357 [89-fb] Refactoring: use the builder pattern for HColumnDescriptor".

        INLINE COMMENTS
        src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java:486 INTEGER.MAX_VALUE seems to be the block size. Not sure why it was this in the past. But the new behavior, you are defaulting back to the default (64k).
        src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java:544 ditto.

        REVISION DETAIL
        https://reviews.facebook.net/D1929

        Show
        Phabricator added a comment - Kannan has commented on the revision " [jira] HBASE-5357 [89-fb] Refactoring: use the builder pattern for HColumnDescriptor". INLINE COMMENTS src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java:486 INTEGER.MAX_VALUE seems to be the block size. Not sure why it was this in the past. But the new behavior, you are defaulting back to the default (64k). src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java:544 ditto. REVISION DETAIL https://reviews.facebook.net/D1929
        Hide
        Phabricator added a comment -

        mbautin has commented on the revision "[jira] HBASE-5357 [89-fb] Refactoring: use the builder pattern for HColumnDescriptor".

        INLINE COMMENTS
        src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java:486 Yes, that block size looked like a bug to me, so I left it out. If someone explains to me why that was reasonable, I would be happy to add it back.

        REVISION DETAIL
        https://reviews.facebook.net/D1929

        Show
        Phabricator added a comment - mbautin has commented on the revision " [jira] HBASE-5357 [89-fb] Refactoring: use the builder pattern for HColumnDescriptor". INLINE COMMENTS src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java:486 Yes, that block size looked like a bug to me, so I left it out. If someone explains to me why that was reasonable, I would be happy to add it back. REVISION DETAIL https://reviews.facebook.net/D1929
        Hide
        Phabricator added a comment -

        stack has commented on the revision "[jira] HBASE-5357 [89-fb] Refactoring: use the builder pattern for HColumnDescriptor".

        Sounds like the old stuff was wrong.

        REVISION DETAIL
        https://reviews.facebook.net/D1929

        Show
        Phabricator added a comment - stack has commented on the revision " [jira] HBASE-5357 [89-fb] Refactoring: use the builder pattern for HColumnDescriptor". Sounds like the old stuff was wrong. REVISION DETAIL https://reviews.facebook.net/D1929
        Hide
        Phabricator added a comment -

        Kannan has accepted the revision "[jira] HBASE-5357 [89-fb] Refactoring: use the builder pattern for HColumnDescriptor".

        nice!

        REVISION DETAIL
        https://reviews.facebook.net/D1929

        BRANCH
        hcd_builder7

        Show
        Phabricator added a comment - Kannan has accepted the revision " [jira] HBASE-5357 [89-fb] Refactoring: use the builder pattern for HColumnDescriptor". nice! REVISION DETAIL https://reviews.facebook.net/D1929 BRANCH hcd_builder7
        Hide
        Phabricator added a comment -

        mbautin has committed the revision "[jira] HBASE-5357 [89-fb] Refactoring: use the builder pattern for HColumnDescriptor".

        REVISION DETAIL
        https://reviews.facebook.net/D1929

        COMMIT
        https://reviews.facebook.net/rHBASEEIGHTNINEFBBRANCH1294394

        Show
        Phabricator added a comment - mbautin has committed the revision " [jira] HBASE-5357 [89-fb] Refactoring: use the builder pattern for HColumnDescriptor". REVISION DETAIL https://reviews.facebook.net/D1929 COMMIT https://reviews.facebook.net/rHBASEEIGHTNINEFBBRANCH1294394

          People

          • Assignee:
            Mikhail Bautin
            Reporter:
            Mikhail Bautin
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development