Uploaded image for project: 'Tajo'
  1. Tajo
  2. TAJO-1320

HBaseStorageManager need to support Zookeeper Client Port.

    Details

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

      Description

      Currently, HBaseStorageManager just use Zookeeper Quorum host names. But I think that users may want to change their Zookeeper client port. Thus, we need to support that users can set the port. For reference, HBase defines the port to ZOOKEEPER_CLIENT_PORT in HBaseConstants.

        Activity

        Hide
        githubbot ASF GitHub Bot added a comment -

        GitHub user blrunner opened a pull request:

        https://github.com/apache/tajo/pull/363

        TAJO-1320: HBaseStorageManager need to support Zookeeper Client Port.

        Currently, HBaseStorageManager just use Zookeeper Quorum host names. But I think that users may want to change their Zookeeper client port. Thus, we need to support that users can set the port. For reference, HBase defines the port to ZOOKEEPER_CLIENT_PORT in HBaseConstants.

        You can merge this pull request into a Git repository by running:

        $ git pull https://github.com/blrunner/tajo TAJO-1320

        Alternatively you can review and apply these changes as the patch at:

        https://github.com/apache/tajo/pull/363.patch

        To close this pull request, make a commit to your master/trunk branch
        with (at least) the following in the commit message:

        This closes #363


        commit 3c73920abc0dbf40802d22835bb715d540d44c42
        Author: JaeHwa Jung <blrunner@apache.org>
        Date: 2015-01-28T02:42:55Z

        TAJO-1320: HBaseStorageManager need to support Zookeeper Client Port.


        Show
        githubbot ASF GitHub Bot added a comment - GitHub user blrunner opened a pull request: https://github.com/apache/tajo/pull/363 TAJO-1320 : HBaseStorageManager need to support Zookeeper Client Port. Currently, HBaseStorageManager just use Zookeeper Quorum host names. But I think that users may want to change their Zookeeper client port. Thus, we need to support that users can set the port. For reference, HBase defines the port to ZOOKEEPER_CLIENT_PORT in HBaseConstants. You can merge this pull request into a Git repository by running: $ git pull https://github.com/blrunner/tajo TAJO-1320 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/tajo/pull/363.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #363 commit 3c73920abc0dbf40802d22835bb715d540d44c42 Author: JaeHwa Jung <blrunner@apache.org> Date: 2015-01-28T02:42:55Z TAJO-1320 : HBaseStorageManager need to support Zookeeper Client Port.
        Hide
        blrunner Jaehwa Jung added a comment -

        I just uploaded the patch.

        Show
        blrunner Jaehwa Jung added a comment - I just uploaded the patch.
        Hide
        tajoqa Tajo QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12694907/TAJO-1320.patch
        against master revision release-0.9.0-rc0-155-gc429c97.

        +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 javadoc. The applied patch does not increase the total number of javadoc warnings.

        +1 checkstyle. The patch generated 0 code style errors.

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

        -1 release audit. The applied patch generated 881 release audit warnings.

        +1 core tests. The patch passed unit tests in tajo-storage/tajo-storage-hbase.

        Test results: https://builds.apache.org/job/PreCommit-TAJO-Build/580//testReport/
        Release audit warnings: https://builds.apache.org/job/PreCommit-TAJO-Build/580//artifact/incubator-tajo/patchprocess/patchReleaseAuditProblems.txt
        Findbugs warnings: https://builds.apache.org/job/PreCommit-TAJO-Build/580//artifact/incubator-tajo/patchprocess/newPatchFindbugsWarningstajo-storage-hbase.html
        Console output: https://builds.apache.org/job/PreCommit-TAJO-Build/580//console

        This message is automatically generated.

        Show
        tajoqa Tajo QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12694907/TAJO-1320.patch against master revision release-0.9.0-rc0-155-gc429c97. +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 javadoc. The applied patch does not increase the total number of javadoc warnings. +1 checkstyle. The patch generated 0 code style errors. -1 findbugs. The patch appears to introduce 6 new Findbugs (version 2.0.3) warnings. -1 release audit. The applied patch generated 881 release audit warnings. +1 core tests. The patch passed unit tests in tajo-storage/tajo-storage-hbase. Test results: https://builds.apache.org/job/PreCommit-TAJO-Build/580//testReport/ Release audit warnings: https://builds.apache.org/job/PreCommit-TAJO-Build/580//artifact/incubator-tajo/patchprocess/patchReleaseAuditProblems.txt Findbugs warnings: https://builds.apache.org/job/PreCommit-TAJO-Build/580//artifact/incubator-tajo/patchprocess/newPatchFindbugsWarningstajo-storage-hbase.html Console output: https://builds.apache.org/job/PreCommit-TAJO-Build/580//console This message is automatically generated.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user jihoonson commented on a diff in the pull request:

        https://github.com/apache/tajo/pull/363#discussion_r23663460

        — Diff: tajo-storage/tajo-storage-hbase/src/main/java/org/apache/tajo/storage/hbase/HBaseStorageConstants.java —
        @@ -27,6 +27,7 @@
        public static final String META_SPLIT_ROW_KEYS_KEY = "hbase.split.rowkeys";
        public static final String META_SPLIT_ROW_KEYS_FILE_KEY = "hbase.split.rowkeys.file";
        public static final String META_ZK_QUORUM_KEY = "hbase.zookeeper.quorum";
        + public static final String META_ZK_CLIENT_PORT = "hbase.zookeeper.property.clientPort";
        — End diff –

        We traditionally avoid the Camel style for configuration key names.
        And, we need to make the naming style consistent.
        So, how about ```hbase.zookeeper.client.port```?

        Show
        githubbot ASF GitHub Bot added a comment - Github user jihoonson commented on a diff in the pull request: https://github.com/apache/tajo/pull/363#discussion_r23663460 — Diff: tajo-storage/tajo-storage-hbase/src/main/java/org/apache/tajo/storage/hbase/HBaseStorageConstants.java — @@ -27,6 +27,7 @@ public static final String META_SPLIT_ROW_KEYS_KEY = "hbase.split.rowkeys"; public static final String META_SPLIT_ROW_KEYS_FILE_KEY = "hbase.split.rowkeys.file"; public static final String META_ZK_QUORUM_KEY = "hbase.zookeeper.quorum"; + public static final String META_ZK_CLIENT_PORT = "hbase.zookeeper.property.clientPort"; — End diff – We traditionally avoid the Camel style for configuration key names. And, we need to make the naming style consistent. So, how about ```hbase.zookeeper.client.port```?
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user jihoonson commented on a diff in the pull request:

        https://github.com/apache/tajo/pull/363#discussion_r23663679

        — Diff: tajo-storage/tajo-storage-hbase/src/main/java/org/apache/tajo/storage/hbase/HBaseStorageManager.java —
        @@ -312,6 +312,17 @@ public static Configuration getHBaseConfiguration(Configuration conf, TableMeta
        HBaseStorageConstants.META_ZK_QUORUM_KEY + "' attribute.");
        }

        + String zkPort = hbaseConf.get(HConstants.ZOOKEEPER_CLIENT_PORT);
        — End diff –

        I wonder why we don't use the default value.
        Should users always specify the zookeeper client port?

        Show
        githubbot ASF GitHub Bot added a comment - Github user jihoonson commented on a diff in the pull request: https://github.com/apache/tajo/pull/363#discussion_r23663679 — Diff: tajo-storage/tajo-storage-hbase/src/main/java/org/apache/tajo/storage/hbase/HBaseStorageManager.java — @@ -312,6 +312,17 @@ public static Configuration getHBaseConfiguration(Configuration conf, TableMeta HBaseStorageConstants.META_ZK_QUORUM_KEY + "' attribute."); } + String zkPort = hbaseConf.get(HConstants.ZOOKEEPER_CLIENT_PORT); — End diff – I wonder why we don't use the default value. Should users always specify the zookeeper client port?
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user jihoonson commented on the pull request:

        https://github.com/apache/tajo/pull/363#issuecomment-71775217

        Hi @blrunner, your patch looks good.
        I left some minor comments.
        Please consider them.

        Show
        githubbot ASF GitHub Bot added a comment - Github user jihoonson commented on the pull request: https://github.com/apache/tajo/pull/363#issuecomment-71775217 Hi @blrunner, your patch looks good. I left some minor comments. Please consider them.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user blrunner commented on a diff in the pull request:

        https://github.com/apache/tajo/pull/363#discussion_r23663805

        — Diff: tajo-storage/tajo-storage-hbase/src/main/java/org/apache/tajo/storage/hbase/HBaseStorageConstants.java —
        @@ -27,6 +27,7 @@
        public static final String META_SPLIT_ROW_KEYS_KEY = "hbase.split.rowkeys";
        public static final String META_SPLIT_ROW_KEYS_FILE_KEY = "hbase.split.rowkeys.file";
        public static final String META_ZK_QUORUM_KEY = "hbase.zookeeper.quorum";
        + public static final String META_ZK_CLIENT_PORT = "hbase.zookeeper.property.clientPort";
        — End diff –

        Thank you for your review.
        The name borrowed with HBase Constant class. I think that HBase users may be familiar with it compare than another name.

        Show
        githubbot ASF GitHub Bot added a comment - Github user blrunner commented on a diff in the pull request: https://github.com/apache/tajo/pull/363#discussion_r23663805 — Diff: tajo-storage/tajo-storage-hbase/src/main/java/org/apache/tajo/storage/hbase/HBaseStorageConstants.java — @@ -27,6 +27,7 @@ public static final String META_SPLIT_ROW_KEYS_KEY = "hbase.split.rowkeys"; public static final String META_SPLIT_ROW_KEYS_FILE_KEY = "hbase.split.rowkeys.file"; public static final String META_ZK_QUORUM_KEY = "hbase.zookeeper.quorum"; + public static final String META_ZK_CLIENT_PORT = "hbase.zookeeper.property.clientPort"; — End diff – Thank you for your review. The name borrowed with HBase Constant class. I think that HBase users may be familiar with it compare than another name.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user blrunner commented on a diff in the pull request:

        https://github.com/apache/tajo/pull/363#discussion_r23664033

        — Diff: tajo-storage/tajo-storage-hbase/src/main/java/org/apache/tajo/storage/hbase/HBaseStorageManager.java —
        @@ -312,6 +312,17 @@ public static Configuration getHBaseConfiguration(Configuration conf, TableMeta
        HBaseStorageConstants.META_ZK_QUORUM_KEY + "' attribute.");
        }

        + String zkPort = hbaseConf.get(HConstants.ZOOKEEPER_CLIENT_PORT);
        — End diff –

        We already use the default value.
        If users doesn't set zookeeper hosts or client port, Tajo uses properties of HBase configuration file.
        The above code just be used to check empty value.

        Show
        githubbot ASF GitHub Bot added a comment - Github user blrunner commented on a diff in the pull request: https://github.com/apache/tajo/pull/363#discussion_r23664033 — Diff: tajo-storage/tajo-storage-hbase/src/main/java/org/apache/tajo/storage/hbase/HBaseStorageManager.java — @@ -312,6 +312,17 @@ public static Configuration getHBaseConfiguration(Configuration conf, TableMeta HBaseStorageConstants.META_ZK_QUORUM_KEY + "' attribute."); } + String zkPort = hbaseConf.get(HConstants.ZOOKEEPER_CLIENT_PORT); — End diff – We already use the default value. If users doesn't set zookeeper hosts or client port, Tajo uses properties of HBase configuration file. The above code just be used to check empty value.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user jihoonson commented on the pull request:

        https://github.com/apache/tajo/pull/363#issuecomment-71777378

        Thanks for your comment.
        Those changes look good to me.
        +1!

        Show
        githubbot ASF GitHub Bot added a comment - Github user jihoonson commented on the pull request: https://github.com/apache/tajo/pull/363#issuecomment-71777378 Thanks for your comment. Those changes look good to me. +1!
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user asfgit closed the pull request at:

        https://github.com/apache/tajo/pull/363

        Show
        githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/tajo/pull/363
        Hide
        blrunner Jaehwa Jung added a comment -

        Thanks Jihoon Son

        I've just committed the patch to the master branch.

        Show
        blrunner Jaehwa Jung added a comment - Thanks Jihoon Son I've just committed the patch to the master branch.
        Hide
        hudson Hudson added a comment -

        SUCCESS: Integrated in Tajo-master-build #567 (See https://builds.apache.org/job/Tajo-master-build/567/)
        TAJO-1320: HBaseStorageManager need to support Zookeeper Client Port. (jaehwa) (blrunner: rev b9719ba78ef441772ee7f5ffa44844627df95891)

        • tajo-storage/tajo-storage-hbase/src/main/java/org/apache/tajo/storage/hbase/HBaseStorageManager.java
        • CHANGES
        • tajo-storage/tajo-storage-hbase/src/main/java/org/apache/tajo/storage/hbase/HBaseStorageConstants.java
        Show
        hudson Hudson added a comment - SUCCESS: Integrated in Tajo-master-build #567 (See https://builds.apache.org/job/Tajo-master-build/567/ ) TAJO-1320 : HBaseStorageManager need to support Zookeeper Client Port. (jaehwa) (blrunner: rev b9719ba78ef441772ee7f5ffa44844627df95891) tajo-storage/tajo-storage-hbase/src/main/java/org/apache/tajo/storage/hbase/HBaseStorageManager.java CHANGES tajo-storage/tajo-storage-hbase/src/main/java/org/apache/tajo/storage/hbase/HBaseStorageConstants.java
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Tajo-master-CODEGEN-build #206 (See https://builds.apache.org/job/Tajo-master-CODEGEN-build/206/)
        TAJO-1320: HBaseStorageManager need to support Zookeeper Client Port. (jaehwa) (blrunner: rev b9719ba78ef441772ee7f5ffa44844627df95891)

        • CHANGES
        • tajo-storage/tajo-storage-hbase/src/main/java/org/apache/tajo/storage/hbase/HBaseStorageConstants.java
        • tajo-storage/tajo-storage-hbase/src/main/java/org/apache/tajo/storage/hbase/HBaseStorageManager.java
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Tajo-master-CODEGEN-build #206 (See https://builds.apache.org/job/Tajo-master-CODEGEN-build/206/ ) TAJO-1320 : HBaseStorageManager need to support Zookeeper Client Port. (jaehwa) (blrunner: rev b9719ba78ef441772ee7f5ffa44844627df95891) CHANGES tajo-storage/tajo-storage-hbase/src/main/java/org/apache/tajo/storage/hbase/HBaseStorageConstants.java tajo-storage/tajo-storage-hbase/src/main/java/org/apache/tajo/storage/hbase/HBaseStorageManager.java
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Tajo-master-CODEGEN-build #207 (See https://builds.apache.org/job/Tajo-master-CODEGEN-build/207/)
        TAJO-1320: HBaseStorageManager need to support Zookeeper Client Port. (missing file) (blrunner: rev 015913b7a7eec922475a67d9455457f229367af7)

        • tajo-docs/src/main/sphinx/hbase_integration.rst
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Tajo-master-CODEGEN-build #207 (See https://builds.apache.org/job/Tajo-master-CODEGEN-build/207/ ) TAJO-1320 : HBaseStorageManager need to support Zookeeper Client Port. (missing file) (blrunner: rev 015913b7a7eec922475a67d9455457f229367af7) tajo-docs/src/main/sphinx/hbase_integration.rst
        Hide
        hudson Hudson added a comment -

        SUCCESS: Integrated in Tajo-master-build #568 (See https://builds.apache.org/job/Tajo-master-build/568/)
        TAJO-1320: HBaseStorageManager need to support Zookeeper Client Port. (missing file) (blrunner: rev 015913b7a7eec922475a67d9455457f229367af7)

        • tajo-docs/src/main/sphinx/hbase_integration.rst
        Show
        hudson Hudson added a comment - SUCCESS: Integrated in Tajo-master-build #568 (See https://builds.apache.org/job/Tajo-master-build/568/ ) TAJO-1320 : HBaseStorageManager need to support Zookeeper Client Port. (missing file) (blrunner: rev 015913b7a7eec922475a67d9455457f229367af7) tajo-docs/src/main/sphinx/hbase_integration.rst

          People

          • Assignee:
            blrunner Jaehwa Jung
            Reporter:
            blrunner Jaehwa Jung
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development