You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@phoenix.apache.org by rahulsIOT <gi...@git.apache.org> on 2017/03/24 05:31:36 UTC

[GitHub] phoenix pull request #236: Loadbalancer

GitHub user rahulsIOT opened a pull request:

    https://github.com/apache/phoenix/pull/236

    Loadbalancer

    Hi All,
    
    https://issues.apache.org/jira/browse/PHOENIX-3654
    
    The following changes are made in the pull request
    
    1. Added project phoenix-load-balancer. The project exposes Loadbalancer mechanism for thin client. 
    2. The following additional phoenix parameters is exposed in LoadBalancerConfiguration. These configuration is read from hbase-site.xml. The configuration are as follows
        clusterName = "phoenix.queryserver.base.path"
        serviceName = "phoenix.queryserver.service.name"
        defaultClusterName = "phoenix"
        defaultServiceName = "queryserver"
        zkLbUserName = "phoenix.queryserver.zookeeper.acl.username"
        zkLbPassword = "phoenix.queryserver.zookeeper.acl.password"
        defaultZkLbUserName = "phoenixuser"
        defaultZkLbPassword = "Xsjdhxsd"
    3. phoenix-load-balancer is not dependent on phoenix core, which was one of requirement for the project. It has dependencies on apache-curator and hbase-common for hbase configuration.
    4. The project phoenix-queryserver is modified such that when server loads up, it will find zookeeper configuration from hbase-site.xml and registers itself to the configuration. The registered node will have patch /clusterName/serviceName/hostname_port (e.g. /phoenix/queryserver/host1_1234). This will be emphimaral node and gets removed when session has ended. With each ephimeral node, there is a json attached. The Json is of the form {"host":"host1","port":"1234"}. In future this can be expanded to store more values for the node. The Node has ACL = digest with username:password as id. There is default username/password shared between Loadbalancer project and phoenix query server project. User can specify the username/password as properties "phoenix.queryserver.zookeeper.acl.username"/"phoenix.queryserver.zookeeper.acl.password" in hbase-site.xml. 
    5. For phoenix-queryserver , there is a dependency of phoenix-load-balancer. This is needed to load configuration related to only LoadBalancer. To avoid code duplication, we have kept all the configuration for LoadBalancer in the file LoadBalancerConfiguration.java within project phoenix-load-balancer. 
    6. IT tests cases were written for load balancer.

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

    $ git pull https://github.com/rahulsIOT/phoenix Loadbalancer

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

    https://github.com/apache/phoenix/pull/236.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 #236
    
----
commit 8bff60831e55d2d2c1e4fffc6e662dc25fe5be0a
Author: Rahul <rs...@salesforce.com>
Date:   2017-03-01T23:38:52Z

    Initial set of files for Load Balancer

commit 4f2422562afc59106a02e82eb9672ac9d6429bbd
Author: Rahul <rs...@salesforce.com>
Date:   2017-03-01T23:42:45Z

    pom for load balancer project

commit 8951a7c24da1be331fc43ee4986ff5f33435616f
Author: Rahul <rs...@salesforce.com>
Date:   2017-03-02T01:57:41Z

    added code for service discovery

commit 2fd2e82667af5157f0e5a5bb12e7aec6319dc430
Author: Rahul <rs...@salesforce.com>
Date:   2017-03-04T02:11:16Z

    added registration code to query server

commit 55a59388fe523d7340440b1736fce400f4fe9429
Author: Rahul <rs...@salesforce.com>
Date:   2017-03-08T21:44:05Z

    added extra changes for including tests

commit 6420f7348e8a313b0c18d969654bbb49632c4d48
Author: Rahul <rs...@salesforce.com>
Date:   2017-03-13T17:10:51Z

     still making changes to initial commit

commit 8e712b4bbc52524554cd4ed3ca5ec923d99fdb46
Author: Rahul <rs...@salesforce.com>
Date:   2017-03-14T18:03:03Z

     Fixed naming of variables along with other cosmetic fixes

commit a7404435abaa2af5ad284972933d0ecb2cb94f45
Author: Rahul <rs...@salesforce.com>
Date:   2017-03-21T07:07:18Z

    fixed all the IT cases in LoadBalancer

commit 5a9b7021dc8d51884f546973db633b6ca0ee59e5
Author: Rahul <rs...@salesforce.com>
Date:   2017-03-24T04:58:05Z

    completed the initial patch for load balancer

commit d3d8bea7299dbb6830bbdf2925fc43512599df71
Author: Rahul <rs...@salesforce.com>
Date:   2017-03-24T05:11:36Z

    removed unneeded sections of pom.xml including curator version. This version is in parent pom.xml

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix issue #236: PHOENIX-3654 Load Balancer for thin client

Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on the issue:

    https://github.com/apache/phoenix/pull/236
  
    Also, maybe I'm just not seeing it, but how does this load balancer get used by clients? It seems like this is only the advertisement component, and there is nothing for the thin client to use it. Is that intended? (in other words, you would do the rest later)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: Loadbalancer

Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r109749299
  
    --- Diff: phoenix-queryserver/pom.xml ---
    @@ -147,6 +147,14 @@
           <groupId>commons-logging</groupId>
           <artifactId>commons-logging</artifactId>
         </dependency>
    +    <dependency>
    +      <groupId>org.apache.phoenix</groupId>
    +      <artifactId>phoenix-queryserver-loadbalancer</artifactId>
    --- End diff --
    
    I'm pretty sure I put this on the last PR (the one you closed). I think it's a bad idea to strongly tie PQS to the load balancer. You should make an interface in PQS which defines what it needs to know and then include the load balancer impl optionally (e.g. via ServiceLoader).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: PHOENIX-3654 Load Balancer for thin client

Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r129067705
  
    --- Diff: phoenix-core/src/main/java/org/apache/phoenix/query/QueryServicesOptions.java ---
    @@ -284,14 +288,18 @@
         public static final int DEFAULT_COLUMN_ENCODED_BYTES = QualifierEncodingScheme.TWO_BYTE_QUALIFIERS.getSerializedMetadataValue();
         public static final String DEFAULT_IMMUTABLE_STORAGE_SCHEME = ImmutableStorageScheme.SINGLE_CELL_ARRAY_WITH_OFFSETS.toString();
         public static final String DEFAULT_MULTITENANT_IMMUTABLE_STORAGE_SCHEME = ImmutableStorageScheme.ONE_CELL_PER_COLUMN.toString();
    +    public  final static String DEFAULT_PHOENIX_QUERY_SERVER_CLUSTER_BASE_PATH = "/phoenix";
    +    public  final static String DEFAULT_PHOENIX_QUERY_SERVER_SERVICE_NAME = "queryserver";
    +    public  final static String DEFAULT_PHOENIX_QUERY_SERVER_ZK_ACL_USERNAME = "phoenixuser";
    +    public  final static String DEFAULT_PHOENIX_QUERY_SERVER_ZK_ACL_PASSWORD = "Xsjdhxsd";
     
         //by default, max connections from one client to one cluster is unlimited
         public static final int DEFAULT_CLIENT_CONNECTION_MAX_ALLOWED_CONNECTIONS = 0;
    -    public static final boolean DEFAULT_STATS_COLLECTION_ENABLED = true;
    -    public static final boolean DEFAULT_USE_STATS_FOR_PARALLELIZATION = true;
     
         //default update cache frequency
         public static final int DEFAULT_UPDATE_CACHE_FREQUENCY = 0;
    +    public static final boolean DEFAULT_STATS_COLLECTION_ENABLED = true;
    +    public static final boolean DEFAULT_USE_STATS_FOR_PARALLELIZATION = true;
    --- End diff --
    
    Nit: undo this move please.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: Loadbalancer

Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r109748463
  
    --- Diff: phoenix-load-balancer/src/main/java/org/apache/phoenix/loadbalancer/service/LoadBalancer.java ---
    @@ -0,0 +1,165 @@
    +/**
    + *
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.phoenix.loadbalancer.service;
    +
    +import com.google.common.base.Preconditions;
    +import com.google.common.collect.Lists;
    +import org.apache.commons.logging.Log;
    +import org.apache.commons.logging.LogFactory;
    +import org.apache.curator.framework.CuratorFramework;
    +import org.apache.curator.framework.CuratorFrameworkFactory;
    +import org.apache.curator.framework.api.UnhandledErrorListener;
    +import org.apache.curator.framework.recipes.cache.ChildData;
    +import org.apache.curator.framework.recipes.cache.PathChildrenCache;
    +import org.apache.curator.framework.recipes.cache.PathChildrenCacheEvent;
    +import org.apache.curator.framework.recipes.cache.PathChildrenCacheListener;
    +import org.apache.curator.framework.state.ConnectionState;
    +import org.apache.curator.framework.state.ConnectionStateListener;
    +import org.apache.curator.retry.ExponentialBackoffRetry;
    +import org.apache.curator.utils.CloseableUtils;
    +import org.apache.phoenix.loadbalancer.exception.NoPhoenixQueryServerRegisteredException;
    +
    +import java.nio.charset.StandardCharsets;
    +import java.util.ArrayList;
    +import java.util.concurrent.ThreadLocalRandom;
    +import java.io.Closeable;
    +import java.util.List;
    +
    +
    +public class LoadBalancer {
    +
    +    private static final LoadBalancerConfiguration CONFIG = new LoadBalancerConfiguration();
    +    private static CuratorFramework curaFramework = null;
    +    protected static final Log LOG = LogFactory.getLog(LoadBalancer.class);
    +    private static PathChildrenCache   cache = null;
    +    private static final LoadBalancer loadBalancer = new LoadBalancer();
    +
    +    private LoadBalancer()  {
    +        ConnectionStateListener connectionStateListener = null;
    +        UnhandledErrorListener unhandledErrorListener = null;
    +        List<Closeable> closeAbles = Lists.newArrayList();
    +        try  {
    +
    +            curaFramework = CuratorFrameworkFactory.newClient(getZkConnectString(),
    +                    new ExponentialBackoffRetry(1000, 3));
    +            curaFramework.start();
    +            curaFramework.setACL().withACL(CONFIG.getAcls());
    +            connectionStateListener = getConnectionStateListener();
    +            curaFramework.getConnectionStateListenable()
    +                    .addListener(connectionStateListener);
    +            unhandledErrorListener = getUnhandledErrorListener();
    +            curaFramework.getUnhandledErrorListenable()
    +                    .addListener(unhandledErrorListener);
    +            cache = new PathChildrenCache(curaFramework, CONFIG.getParentPath(), true);
    +            cache.start(PathChildrenCache.StartMode.BUILD_INITIAL_CACHE);
    +            closeAbles.add(cache);
    +            closeAbles.add(curaFramework);
    +        }catch(Exception ex){
    +            LOG.error("Exception while creating a zookeeper clients and cache",ex);
    +            if ((curaFramework != null) && (connectionStateListener != null)){
    +                curaFramework.getConnectionStateListenable()
    +                        .removeListener(connectionStateListener);
    +            }
    +            if ((curaFramework != null) && (unhandledErrorListener != null)){
    +                curaFramework.getUnhandledErrorListenable()
    +                        .removeListener(unhandledErrorListener);
    +            }
    +            for (Closeable closeable : closeAbles) {
    +                CloseableUtils.closeQuietly(closeable);
    +            }
    +        }
    +    }
    +
    +    /**
    +     * Return Singleton Load Balancer every single time.
    +     * @return LoadBalancer
    +     */
    +    public static LoadBalancer getLoadBalancer() {
    +        return loadBalancer;
    +    }
    +
    +    /**
    +     * return the location of Phoenix Query Server
    +     * in form of String. The method return a random PQS server
    +     * from the cache.
    +     * @return - PQS Location ( host:port)
    +     * @throws Exception
    +     */
    +    public PhoenixQueryServerNode getSingleServiceLocation()  throws Exception{
    +        List<PhoenixQueryServerNode> childNodes = conductSanityCheckAndReturn();
    +        // get an random connect string
    +        int i = ThreadLocalRandom.current().nextInt(0, childNodes.size());
    +        return childNodes.get(i);
    +    }
    +
    +    /**
    +     * return all the location of Phoenix Query Server from cache
    +     * @return - PQS Locations ( host1:port1,host2:port2)
    +     * @throws Exception
    +     */
    +    public List<PhoenixQueryServerNode> getAllServiceLocation()  throws Exception{
    +        return conductSanityCheckAndReturn();
    +    }
    +
    +    private List<PhoenixQueryServerNode> conductSanityCheckAndReturn() throws Exception{
    +        Preconditions.checkNotNull(curaFramework
    +                ," curator framework in not initialized ");
    +        Preconditions.checkNotNull(cache," cache value is not initialized");
    +        boolean connected = curaFramework.getZookeeperClient().isConnected();
    +        if (!connected) {
    +            String message = " Zookeeper seems to be down. The data is stale ";
    +            Exception exception =
    +                    new Exception(message);
    +            LOG.error(message, exception);
    +            throw exception;
    +        }
    +        List<String> currentNodes = curaFramework.getChildren().forPath(CONFIG.getParentPath());
    +        List<PhoenixQueryServerNode> returnNodes = new ArrayList<>();
    +        for(String node:currentNodes) {
    +            byte[] bytes = curaFramework.getData().forPath(CONFIG.getParentPath() + "/" + node);
    +            returnNodes.add(PhoenixQueryServerNode.fromJsonString(new String(
    +                    bytes, StandardCharsets.UTF_8)));
    +        }
    +        return returnNodes;
    +    }
    +    private String getZkConnectString(){
    +        return CONFIG.getZkConnectString();
    +    }
    +
    +    private ConnectionStateListener getConnectionStateListener(){
    +        return new ConnectionStateListener() {
    +            @Override
    +            public void stateChanged(CuratorFramework client, ConnectionState newState) {
    +                if (!newState.isConnected()) {
    +                    LOG.error(" connection to zookeeper broken ");
    --- End diff --
    
    Please include the connection state. ZK has a complicated graph of connectivity states (many of which are recoverable). To debug this, we would want to know what state we moved into.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: Loadbalancer

Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r109749668
  
    --- Diff: phoenix-queryserver/src/main/java/org/apache/phoenix/queryserver/register/Registry.java ---
    @@ -0,0 +1,31 @@
    +/**
    + *
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.phoenix.queryserver.register;
    +
    +
    +import org.apache.phoenix.loadbalancer.service.LoadBalancerConfiguration;
    +
    +public interface  Registry  {
    +
    +    public void deRegisterTheServer() throws Exception;
    +
    +    public void registerServer(LoadBalancerConfiguration loadBalancerConfiguration, int port
    --- End diff --
    
    Would passing in a PhoenixQueryServerNode make more sense?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: Loadbalancer

Posted by rahulsIOT <gi...@git.apache.org>.
Github user rahulsIOT commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r117437437
  
    --- Diff: phoenix-load-balancer/src/main/java/org/apache/phoenix/loadbalancer/service/LoadBalancer.java ---
    @@ -0,0 +1,165 @@
    +/**
    + *
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.phoenix.loadbalancer.service;
    +
    +import com.google.common.base.Preconditions;
    +import com.google.common.collect.Lists;
    +import org.apache.commons.logging.Log;
    +import org.apache.commons.logging.LogFactory;
    +import org.apache.curator.framework.CuratorFramework;
    +import org.apache.curator.framework.CuratorFrameworkFactory;
    +import org.apache.curator.framework.api.UnhandledErrorListener;
    +import org.apache.curator.framework.recipes.cache.ChildData;
    +import org.apache.curator.framework.recipes.cache.PathChildrenCache;
    +import org.apache.curator.framework.recipes.cache.PathChildrenCacheEvent;
    +import org.apache.curator.framework.recipes.cache.PathChildrenCacheListener;
    +import org.apache.curator.framework.state.ConnectionState;
    +import org.apache.curator.framework.state.ConnectionStateListener;
    +import org.apache.curator.retry.ExponentialBackoffRetry;
    +import org.apache.curator.utils.CloseableUtils;
    +import org.apache.phoenix.loadbalancer.exception.NoPhoenixQueryServerRegisteredException;
    +
    +import java.nio.charset.StandardCharsets;
    +import java.util.ArrayList;
    +import java.util.concurrent.ThreadLocalRandom;
    +import java.io.Closeable;
    +import java.util.List;
    +
    +
    +public class LoadBalancer {
    +
    +    private static final LoadBalancerConfiguration CONFIG = new LoadBalancerConfiguration();
    +    private static CuratorFramework curaFramework = null;
    +    protected static final Log LOG = LogFactory.getLog(LoadBalancer.class);
    +    private static PathChildrenCache   cache = null;
    +    private static final LoadBalancer loadBalancer = new LoadBalancer();
    +
    +    private LoadBalancer()  {
    +        ConnectionStateListener connectionStateListener = null;
    +        UnhandledErrorListener unhandledErrorListener = null;
    +        List<Closeable> closeAbles = Lists.newArrayList();
    +        try  {
    +
    +            curaFramework = CuratorFrameworkFactory.newClient(getZkConnectString(),
    +                    new ExponentialBackoffRetry(1000, 3));
    +            curaFramework.start();
    +            curaFramework.setACL().withACL(CONFIG.getAcls());
    +            connectionStateListener = getConnectionStateListener();
    +            curaFramework.getConnectionStateListenable()
    +                    .addListener(connectionStateListener);
    +            unhandledErrorListener = getUnhandledErrorListener();
    +            curaFramework.getUnhandledErrorListenable()
    +                    .addListener(unhandledErrorListener);
    +            cache = new PathChildrenCache(curaFramework, CONFIG.getParentPath(), true);
    +            cache.start(PathChildrenCache.StartMode.BUILD_INITIAL_CACHE);
    +            closeAbles.add(cache);
    +            closeAbles.add(curaFramework);
    +        }catch(Exception ex){
    +            LOG.error("Exception while creating a zookeeper clients and cache",ex);
    +            if ((curaFramework != null) && (connectionStateListener != null)){
    +                curaFramework.getConnectionStateListenable()
    +                        .removeListener(connectionStateListener);
    +            }
    +            if ((curaFramework != null) && (unhandledErrorListener != null)){
    +                curaFramework.getUnhandledErrorListenable()
    +                        .removeListener(unhandledErrorListener);
    +            }
    +            for (Closeable closeable : closeAbles) {
    +                CloseableUtils.closeQuietly(closeable);
    +            }
    +        }
    +    }
    +
    +    /**
    +     * Return Singleton Load Balancer every single time.
    +     * @return LoadBalancer
    +     */
    +    public static LoadBalancer getLoadBalancer() {
    +        return loadBalancer;
    +    }
    +
    +    /**
    +     * return the location of Phoenix Query Server
    +     * in form of String. The method return a random PQS server
    +     * from the cache.
    +     * @return - PQS Location ( host:port)
    +     * @throws Exception
    +     */
    +    public PhoenixQueryServerNode getSingleServiceLocation()  throws Exception{
    +        List<PhoenixQueryServerNode> childNodes = conductSanityCheckAndReturn();
    +        // get an random connect string
    +        int i = ThreadLocalRandom.current().nextInt(0, childNodes.size());
    +        return childNodes.get(i);
    +    }
    +
    +    /**
    +     * return all the location of Phoenix Query Server from cache
    +     * @return - PQS Locations ( host1:port1,host2:port2)
    +     * @throws Exception
    +     */
    +    public List<PhoenixQueryServerNode> getAllServiceLocation()  throws Exception{
    +        return conductSanityCheckAndReturn();
    +    }
    +
    +    private List<PhoenixQueryServerNode> conductSanityCheckAndReturn() throws Exception{
    +        Preconditions.checkNotNull(curaFramework
    +                ," curator framework in not initialized ");
    +        Preconditions.checkNotNull(cache," cache value is not initialized");
    +        boolean connected = curaFramework.getZookeeperClient().isConnected();
    +        if (!connected) {
    +            String message = " Zookeeper seems to be down. The data is stale ";
    +            Exception exception =
    +                    new Exception(message);
    +            LOG.error(message, exception);
    +            throw exception;
    +        }
    +        List<String> currentNodes = curaFramework.getChildren().forPath(CONFIG.getParentPath());
    +        List<PhoenixQueryServerNode> returnNodes = new ArrayList<>();
    +        for(String node:currentNodes) {
    +            byte[] bytes = curaFramework.getData().forPath(CONFIG.getParentPath() + "/" + node);
    +            returnNodes.add(PhoenixQueryServerNode.fromJsonString(new String(
    +                    bytes, StandardCharsets.UTF_8)));
    +        }
    +        return returnNodes;
    +    }
    +    private String getZkConnectString(){
    +        return CONFIG.getZkConnectString();
    +    }
    +
    +    private ConnectionStateListener getConnectionStateListener(){
    +        return new ConnectionStateListener() {
    +            @Override
    +            public void stateChanged(CuratorFramework client, ConnectionState newState) {
    +                if (!newState.isConnected()) {
    +                    LOG.error(" connection to zookeeper broken ");
    +                }
    +            }
    +        };
    +    }
    +
    +    private UnhandledErrorListener getUnhandledErrorListener(){
    +        return new UnhandledErrorListener() {
    +            @Override
    +            public void unhandledError(String message, Throwable e) {
    +                LOG.error("unhandled exception ",e);
    --- End diff --
    
    done.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: Loadbalancer

Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r109750151
  
    --- Diff: phoenix-queryserver/src/main/java/org/apache/phoenix/queryserver/register/ZookeeperRegistry.java ---
    @@ -0,0 +1,73 @@
    +/**
    + *
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.phoenix.queryserver.register;
    +
    +
    +import org.apache.commons.logging.Log;
    +import org.apache.commons.logging.LogFactory;
    +import org.apache.curator.framework.CuratorFramework;
    +import org.apache.curator.framework.CuratorFrameworkFactory;
    +import org.apache.curator.retry.ExponentialBackoffRetry;
    +import org.apache.curator.utils.CloseableUtils;
    +import org.apache.phoenix.loadbalancer.service.LoadBalancerConfiguration;
    +import org.apache.phoenix.loadbalancer.service.PhoenixQueryServerNode;
    +import org.apache.phoenix.queryserver.server.QueryServer;
    +import org.apache.zookeeper.CreateMode;
    +import org.apache.zookeeper.data.Stat;
    +import java.net.InetAddress;
    +import java.nio.charset.StandardCharsets;
    +
    +
    +public class ZookeeperRegistry implements Registry {
    +
    +    protected static final Log LOG = LogFactory.getLog(QueryServer.class);
    +    private CuratorFramework client;
    +
    +    public ZookeeperRegistry(){}
    +
    +    @Override
    +    public  void registerServer(LoadBalancerConfiguration loadBalancerConfiguration, int avaticaServerPort,
    +                                String zookeeperConnectString)
    +            throws Exception {
    +
    +        this.client = CuratorFrameworkFactory.newClient(zookeeperConnectString,
    +                new ExponentialBackoffRetry(1000,10));
    +        this.client.start();
    +        String host = InetAddress.getLocalHost().getHostName();
    +        String path = loadBalancerConfiguration.getFullPathToNode(host,String.valueOf(avaticaServerPort));
    +        PhoenixQueryServerNode phoenixQueryServerNode = new PhoenixQueryServerNode(host, String.valueOf(avaticaServerPort));
    +        String node = phoenixQueryServerNode.toJsonString();
    +
    +        this.client.create().creatingParentsIfNeeded().withMode(CreateMode.EPHEMERAL).forPath(path
    +                ,node.getBytes(StandardCharsets.UTF_8));
    +        Stat stat = this.client.setACL().withACL(loadBalancerConfiguration.getAcls()).forPath(path);
    +        if (stat != null) {
    +            LOG.info(" node created with right ACL");
    +        }
    +        else {
    +            LOG.error("could not create node with right ACL");
    --- End diff --
    
    I would think that this should exit. If the expected ACL could not be set, this is a security risk.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: PHOENIX-3654 Load Balancer for thin client

Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r129069369
  
    --- Diff: phoenix-queryserver/src/main/java/org/apache/phoenix/queryserver/register/Registry.java ---
    @@ -0,0 +1,31 @@
    +/**
    + *
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.phoenix.queryserver.register;
    +
    +
    +import org.apache.phoenix.loadbalancer.service.LoadBalanceZookeeperConf;
    +
    +public interface  Registry  {
    +
    +    public void deRegisterTheServer() throws Exception;
    --- End diff --
    
    nit: `deregisterServer()` instead. Or, should it be `deregisterServers()`? Does it remove all servers since it has no arguments? (javadoc here would also be nice :))
    
    Also, you can drop the `public` modifier -- this is unnecessary for interfaces.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: PHOENIX-3654 Load Balancer for thin client

Posted by rahulsIOT <gi...@git.apache.org>.
Github user rahulsIOT commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r122086056
  
    --- Diff: pom.xml ---
    @@ -953,6 +961,26 @@
             <artifactId>joda-time</artifactId>
             <version>${jodatime.version}</version>
           </dependency>
    +      <dependency>
    +        <groupId>org.apache.curator</groupId>
    +        <artifactId>curator-framework</artifactId>
    --- End diff --
    
    removed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: PHOENIX-3654 Load Balancer for thin client

Posted by rahulsIOT <gi...@git.apache.org>.
Github user rahulsIOT commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r122060006
  
    --- Diff: phoenix-queryserver/src/main/java/org/apache/phoenix/queryserver/server/QueryServer.java ---
    @@ -233,16 +240,29 @@ public int run(String[] args) throws Exception {
           // Build and start the HttpServer
           server = builder.build();
           server.start();
    +      registerToServiceProvider(hostname);
           runningLatch.countDown();
           server.join();
           return 0;
         } catch (Throwable t) {
           LOG.fatal("Unrecoverable service error. Shutting down.", t);
           this.t = t;
           return -1;
    +    } finally {
    +      deRegister();
         }
       }
     
    +  private void registerToServiceProvider(String hostName) throws Exception {
    +      PqsZookeeperConf pqsZookeeperConf = new PqsZookeeperConfImpl(getConf());
    --- End diff --
    
    The implementation of service loader is simple but I think I am stuck at the point of resolving dependencies. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: PHOENIX-3654 Load Balancer for thin client

Posted by rahulsIOT <gi...@git.apache.org>.
Github user rahulsIOT commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r122059868
  
    --- Diff: phoenix-queryserver/src/main/java/org/apache/phoenix/queryserver/server/QueryServer.java ---
    @@ -233,16 +240,29 @@ public int run(String[] args) throws Exception {
           // Build and start the HttpServer
           server = builder.build();
           server.start();
    +      registerToServiceProvider(hostname);
           runningLatch.countDown();
           server.join();
           return 0;
         } catch (Throwable t) {
           LOG.fatal("Unrecoverable service error. Shutting down.", t);
           this.t = t;
           return -1;
    +    } finally {
    +      deRegister();
         }
       }
     
    +  private void registerToServiceProvider(String hostName) throws Exception {
    +      PqsZookeeperConf pqsZookeeperConf = new PqsZookeeperConfImpl(getConf());
    --- End diff --
    
    I think you suggest having implementation in load balancer module and interface def in query server module. Then load the implementation in queryserver module via service loader from loadbalancer module. But, How do make query server dependent on load balancer module when load balancer has a dependency on queryserver. Is there any plugin to load the jars from loadbalancer into queryserver target ? Please advise.  


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: Loadbalancer

Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r109746752
  
    --- Diff: phoenix-load-balancer/pom.xml ---
    @@ -0,0 +1,58 @@
    +<?xml version='1.0'?>
    +<!--
    +
    + Licensed to the Apache Software Foundation (ASF) under one
    + or more contributor license agreements.  See the NOTICE file
    + distributed with this work for additional information
    + regarding copyright ownership.  The ASF licenses this file
    + to you under the Apache License, Version 2.0 (the
    + "License"); you may not use this file except in compliance
    + with the License.  You may obtain a copy of the License at
    +
    +   http://www.apache.org/licenses/LICENSE-2.0
    +
    + Unless required by applicable law or agreed to in writing,
    + software distributed under the License is distributed on an
    + "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + KIND, either express or implied.  See the License for the
    + specific language governing permissions and limitations
    + under the License.
    +
    +-->
    +
    +<project xmlns="http://maven.apache.org/POM/4.0.0"
    +         xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
    +         xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
    +  <modelVersion>4.0.0</modelVersion>
    +  <parent>
    +    <groupId>org.apache.phoenix</groupId>
    +    <artifactId>phoenix</artifactId>
    +    <version>4.10.0-HBase-1.2-SNAPSHOT</version>
    --- End diff --
    
    Should be 4.11.0-HBase-1.2-SNAPSHOT


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: PHOENIX-3654 Load Balancer for thin client

Posted by rahulsIOT <gi...@git.apache.org>.
Github user rahulsIOT commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r123116803
  
    --- Diff: phoenix-queryserver/src/it/resources/log4j.properties ---
    @@ -1,63 +0,0 @@
    -# Licensed to the Apache Software Foundation (ASF) under one
    -# or more contributor license agreements.  See the NOTICE file
    -# distributed with this work for additional information
    -# regarding copyright ownership.  The ASF licenses this file
    -# to you under the Apache License, Version 2.0 (the
    -# "License"); you may not use this file except in compliance
    -# with the License.  You may obtain a copy of the License at
    -#
    -#     http://www.apache.org/licenses/LICENSE-2.0
    -#
    -# Unless required by applicable law or agreed to in writing, software
    -# distributed under the License is distributed on an "AS IS" BASIS,
    -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    -# See the License for the specific language governing permissions and
    -# limitations under the License.
    -
    -# Define some default values that can be overridden by system properties
    -hbase.root.logger=DEBUG,console
    --- End diff --
    
    yes. sorry. will quickly fix this.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: Loadbalancer

Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r117391916
  
    --- Diff: phoenix-queryserver/pom.xml ---
    @@ -147,6 +147,14 @@
           <groupId>commons-logging</groupId>
           <artifactId>commons-logging</artifactId>
         </dependency>
    +    <dependency>
    +      <groupId>org.apache.phoenix</groupId>
    +      <artifactId>phoenix-queryserver-loadbalancer</artifactId>
    --- End diff --
    
    I don't understand how that is the "same issue". PQS doesn't depend on the load balancer; the load-balancer should depend on PQS (for the interface).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: Loadbalancer

Posted by rahulsIOT <gi...@git.apache.org>.
Github user rahulsIOT commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r116863959
  
    --- Diff: phoenix-load-balancer/pom.xml ---
    @@ -0,0 +1,58 @@
    +<?xml version='1.0'?>
    +<!--
    +
    + Licensed to the Apache Software Foundation (ASF) under one
    + or more contributor license agreements.  See the NOTICE file
    + distributed with this work for additional information
    + regarding copyright ownership.  The ASF licenses this file
    + to you under the Apache License, Version 2.0 (the
    + "License"); you may not use this file except in compliance
    + with the License.  You may obtain a copy of the License at
    +
    +   http://www.apache.org/licenses/LICENSE-2.0
    +
    + Unless required by applicable law or agreed to in writing,
    + software distributed under the License is distributed on an
    + "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + KIND, either express or implied.  See the License for the
    + specific language governing permissions and limitations
    + under the License.
    +
    +-->
    +
    +<project xmlns="http://maven.apache.org/POM/4.0.0"
    +         xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
    +         xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
    +  <modelVersion>4.0.0</modelVersion>
    +  <parent>
    +    <groupId>org.apache.phoenix</groupId>
    +    <artifactId>phoenix</artifactId>
    +    <version>4.10.0-HBase-1.2-SNAPSHOT</version>
    --- End diff --
    
    ok. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: Loadbalancer

Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r109749026
  
    --- Diff: phoenix-load-balancer/src/main/java/org/apache/phoenix/loadbalancer/service/LoadBalancerConfiguration.java ---
    @@ -0,0 +1,91 @@
    +/**
    + *
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.phoenix.loadbalancer.service;
    +
    +
    +
    +import org.apache.hadoop.conf.Configuration;
    +import org.apache.hadoop.hbase.HBaseConfiguration;
    +import org.apache.zookeeper.ZooDefs;
    +import org.apache.zookeeper.data.ACL;
    +import org.apache.zookeeper.data.Id;
    +import java.util.Arrays;
    +import java.util.List;
    +
    +/**
    + * Basic Configuration specific to Load Balancer only.
    + * The instance of {@link LoadBalancerConfiguration} is
    + * used as singleton so that we do not instantiate the class
    + * many times.
    + */
    +public class LoadBalancerConfiguration {
    +
    +    public  final static String clusterName = "phoenix.queryserver.base.path";
    +    public  final static String serviceName = "phoenix.queryserver.service.name";
    +    public  final static String defaultClusterName = "phoenix";
    +    public  final static String defaultServiceName = "queryserver";
    +    public  static String zkQuorum = "hbase.zookeeper.quorum";
    +    public  static String zkPort = "hbase.zookeeper.property.clientPort";
    +    public  static String zkLbUserName = "phoenix.queryserver.zookeeper.acl.username";
    +    public  static String zkLbPassword = "phoenix.queryserver.zookeeper.acl.password";
    +    public  static String defaultZkLbUserName = "phoenixuser";
    +    public  static String defaultZkLbPassword = "Xsjdhxsd";
    +    private static final Configuration configuration = HBaseConfiguration.create();
    +
    +    public LoadBalancerConfiguration() {}
    +
    +    public String getQueryServerBasePath(){
    +        return "/"+configuration.get(clusterName, defaultClusterName);
    +    }
    +
    +    public String getServiceName(){
    +        return configuration.get(serviceName, defaultServiceName);
    +    }
    +
    +    public String getZkConnectString(){
    +        return String.format("%s:%s",configuration.get(zkQuorum,"localhost"),configuration.get(zkPort,"2183"));
    +    }
    +
    +    private String getZkLbUserName(){
    +        return configuration.get(zkLbUserName,defaultZkLbUserName);
    +    }
    +
    +    private String getZkLbPassword(){
    +        return configuration.get(zkLbPassword,defaultZkLbPassword);
    +    }
    +
    +    public List<ACL> getAcls() {
    +        ACL acl = new ACL();
    +        acl.setId(new Id("digest",getZkLbUserName()+":"+getZkLbPassword()));
    +        acl.setPerms(ZooDefs.Perms.READ);
    +        return Arrays.asList(acl);
    +    }
    +
    +    public String getParentPath() {
    +        String path = String.format("%s/%s",getQueryServerBasePath(),getServiceName());
    +        return path;
    +    }
    +
    +    public String getFullPathToNode(String host,String avaticaServerPort) {
    --- End diff --
    
    String args instead of PhoenixQueryServerNode?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: PHOENIX-3654 Load Balancer for thin client

Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r123116500
  
    --- Diff: phoenix-queryserver/src/it/resources/log4j.properties ---
    @@ -1,63 +0,0 @@
    -# Licensed to the Apache Software Foundation (ASF) under one
    -# or more contributor license agreements.  See the NOTICE file
    -# distributed with this work for additional information
    -# regarding copyright ownership.  The ASF licenses this file
    -# to you under the Apache License, Version 2.0 (the
    -# "License"); you may not use this file except in compliance
    -# with the License.  You may obtain a copy of the License at
    -#
    -#     http://www.apache.org/licenses/LICENSE-2.0
    -#
    -# Unless required by applicable law or agreed to in writing, software
    -# distributed under the License is distributed on an "AS IS" BASIS,
    -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    -# See the License for the specific language governing permissions and
    -# limitations under the License.
    -
    -# Define some default values that can be overridden by system properties
    -hbase.root.logger=DEBUG,console
    --- End diff --
    
    Did you accidentally delete this file..?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: PHOENIX-3654 Load Balancer for thin client

Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r120395606
  
    --- Diff: pom.xml ---
    @@ -953,6 +961,26 @@
             <artifactId>joda-time</artifactId>
             <version>${jodatime.version}</version>
           </dependency>
    +      <dependency>
    +        <groupId>org.apache.curator</groupId>
    +        <artifactId>curator-framework</artifactId>
    --- End diff --
    
    This appears to be unused too.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: Loadbalancer

Posted by rahulsIOT <gi...@git.apache.org>.
Github user rahulsIOT commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r117437387
  
    --- Diff: phoenix-load-balancer/src/main/java/org/apache/phoenix/loadbalancer/service/LoadBalancer.java ---
    @@ -0,0 +1,165 @@
    +/**
    + *
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.phoenix.loadbalancer.service;
    +
    +import com.google.common.base.Preconditions;
    +import com.google.common.collect.Lists;
    +import org.apache.commons.logging.Log;
    +import org.apache.commons.logging.LogFactory;
    +import org.apache.curator.framework.CuratorFramework;
    +import org.apache.curator.framework.CuratorFrameworkFactory;
    +import org.apache.curator.framework.api.UnhandledErrorListener;
    +import org.apache.curator.framework.recipes.cache.ChildData;
    +import org.apache.curator.framework.recipes.cache.PathChildrenCache;
    +import org.apache.curator.framework.recipes.cache.PathChildrenCacheEvent;
    +import org.apache.curator.framework.recipes.cache.PathChildrenCacheListener;
    +import org.apache.curator.framework.state.ConnectionState;
    +import org.apache.curator.framework.state.ConnectionStateListener;
    +import org.apache.curator.retry.ExponentialBackoffRetry;
    +import org.apache.curator.utils.CloseableUtils;
    +import org.apache.phoenix.loadbalancer.exception.NoPhoenixQueryServerRegisteredException;
    +
    +import java.nio.charset.StandardCharsets;
    +import java.util.ArrayList;
    +import java.util.concurrent.ThreadLocalRandom;
    +import java.io.Closeable;
    +import java.util.List;
    +
    +
    +public class LoadBalancer {
    +
    +    private static final LoadBalancerConfiguration CONFIG = new LoadBalancerConfiguration();
    +    private static CuratorFramework curaFramework = null;
    +    protected static final Log LOG = LogFactory.getLog(LoadBalancer.class);
    +    private static PathChildrenCache   cache = null;
    +    private static final LoadBalancer loadBalancer = new LoadBalancer();
    +
    +    private LoadBalancer()  {
    +        ConnectionStateListener connectionStateListener = null;
    +        UnhandledErrorListener unhandledErrorListener = null;
    +        List<Closeable> closeAbles = Lists.newArrayList();
    +        try  {
    +
    +            curaFramework = CuratorFrameworkFactory.newClient(getZkConnectString(),
    +                    new ExponentialBackoffRetry(1000, 3));
    +            curaFramework.start();
    +            curaFramework.setACL().withACL(CONFIG.getAcls());
    +            connectionStateListener = getConnectionStateListener();
    +            curaFramework.getConnectionStateListenable()
    +                    .addListener(connectionStateListener);
    +            unhandledErrorListener = getUnhandledErrorListener();
    +            curaFramework.getUnhandledErrorListenable()
    +                    .addListener(unhandledErrorListener);
    +            cache = new PathChildrenCache(curaFramework, CONFIG.getParentPath(), true);
    +            cache.start(PathChildrenCache.StartMode.BUILD_INITIAL_CACHE);
    +            closeAbles.add(cache);
    +            closeAbles.add(curaFramework);
    +        }catch(Exception ex){
    +            LOG.error("Exception while creating a zookeeper clients and cache",ex);
    +            if ((curaFramework != null) && (connectionStateListener != null)){
    +                curaFramework.getConnectionStateListenable()
    +                        .removeListener(connectionStateListener);
    +            }
    +            if ((curaFramework != null) && (unhandledErrorListener != null)){
    +                curaFramework.getUnhandledErrorListenable()
    +                        .removeListener(unhandledErrorListener);
    +            }
    +            for (Closeable closeable : closeAbles) {
    +                CloseableUtils.closeQuietly(closeable);
    +            }
    +        }
    +    }
    +
    +    /**
    +     * Return Singleton Load Balancer every single time.
    +     * @return LoadBalancer
    +     */
    +    public static LoadBalancer getLoadBalancer() {
    +        return loadBalancer;
    +    }
    +
    +    /**
    +     * return the location of Phoenix Query Server
    +     * in form of String. The method return a random PQS server
    +     * from the cache.
    +     * @return - PQS Location ( host:port)
    +     * @throws Exception
    +     */
    +    public PhoenixQueryServerNode getSingleServiceLocation()  throws Exception{
    +        List<PhoenixQueryServerNode> childNodes = conductSanityCheckAndReturn();
    +        // get an random connect string
    +        int i = ThreadLocalRandom.current().nextInt(0, childNodes.size());
    +        return childNodes.get(i);
    +    }
    +
    +    /**
    +     * return all the location of Phoenix Query Server from cache
    +     * @return - PQS Locations ( host1:port1,host2:port2)
    +     * @throws Exception
    +     */
    +    public List<PhoenixQueryServerNode> getAllServiceLocation()  throws Exception{
    +        return conductSanityCheckAndReturn();
    +    }
    +
    +    private List<PhoenixQueryServerNode> conductSanityCheckAndReturn() throws Exception{
    +        Preconditions.checkNotNull(curaFramework
    +                ," curator framework in not initialized ");
    +        Preconditions.checkNotNull(cache," cache value is not initialized");
    +        boolean connected = curaFramework.getZookeeperClient().isConnected();
    +        if (!connected) {
    +            String message = " Zookeeper seems to be down. The data is stale ";
    +            Exception exception =
    +                    new Exception(message);
    +            LOG.error(message, exception);
    +            throw exception;
    +        }
    +        List<String> currentNodes = curaFramework.getChildren().forPath(CONFIG.getParentPath());
    +        List<PhoenixQueryServerNode> returnNodes = new ArrayList<>();
    +        for(String node:currentNodes) {
    +            byte[] bytes = curaFramework.getData().forPath(CONFIG.getParentPath() + "/" + node);
    +            returnNodes.add(PhoenixQueryServerNode.fromJsonString(new String(
    --- End diff --
    
    add try catch to not error out when faced with bad entry.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: Loadbalancer

Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r109749583
  
    --- Diff: phoenix-queryserver/src/main/java/org/apache/phoenix/queryserver/register/Registry.java ---
    @@ -0,0 +1,31 @@
    +/**
    + *
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.phoenix.queryserver.register;
    +
    +
    +import org.apache.phoenix.loadbalancer.service.LoadBalancerConfiguration;
    +
    +public interface  Registry  {
    +
    +    public void deRegisterTheServer() throws Exception;
    --- End diff --
    
    Shouldn't this accept a host and port to deregister?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: Loadbalancer

Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r109747220
  
    --- Diff: phoenix-load-balancer/src/main/java/org/apache/phoenix/loadbalancer/exception/NoPhoenixQueryServerRegisteredException.java ---
    @@ -0,0 +1,15 @@
    +package org.apache.phoenix.loadbalancer.exception;
    +
    +/**
    + * Created by rshrivastava on 3/20/17.
    --- End diff --
    
    Delete


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: Loadbalancer

Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r109748328
  
    --- Diff: phoenix-load-balancer/src/main/java/org/apache/phoenix/loadbalancer/service/LoadBalancer.java ---
    @@ -0,0 +1,165 @@
    +/**
    + *
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.phoenix.loadbalancer.service;
    +
    +import com.google.common.base.Preconditions;
    +import com.google.common.collect.Lists;
    +import org.apache.commons.logging.Log;
    +import org.apache.commons.logging.LogFactory;
    +import org.apache.curator.framework.CuratorFramework;
    +import org.apache.curator.framework.CuratorFrameworkFactory;
    +import org.apache.curator.framework.api.UnhandledErrorListener;
    +import org.apache.curator.framework.recipes.cache.ChildData;
    +import org.apache.curator.framework.recipes.cache.PathChildrenCache;
    +import org.apache.curator.framework.recipes.cache.PathChildrenCacheEvent;
    +import org.apache.curator.framework.recipes.cache.PathChildrenCacheListener;
    +import org.apache.curator.framework.state.ConnectionState;
    +import org.apache.curator.framework.state.ConnectionStateListener;
    +import org.apache.curator.retry.ExponentialBackoffRetry;
    +import org.apache.curator.utils.CloseableUtils;
    +import org.apache.phoenix.loadbalancer.exception.NoPhoenixQueryServerRegisteredException;
    +
    +import java.nio.charset.StandardCharsets;
    +import java.util.ArrayList;
    +import java.util.concurrent.ThreadLocalRandom;
    +import java.io.Closeable;
    +import java.util.List;
    +
    +
    +public class LoadBalancer {
    +
    +    private static final LoadBalancerConfiguration CONFIG = new LoadBalancerConfiguration();
    +    private static CuratorFramework curaFramework = null;
    +    protected static final Log LOG = LogFactory.getLog(LoadBalancer.class);
    +    private static PathChildrenCache   cache = null;
    +    private static final LoadBalancer loadBalancer = new LoadBalancer();
    +
    +    private LoadBalancer()  {
    +        ConnectionStateListener connectionStateListener = null;
    +        UnhandledErrorListener unhandledErrorListener = null;
    +        List<Closeable> closeAbles = Lists.newArrayList();
    +        try  {
    +
    +            curaFramework = CuratorFrameworkFactory.newClient(getZkConnectString(),
    +                    new ExponentialBackoffRetry(1000, 3));
    +            curaFramework.start();
    +            curaFramework.setACL().withACL(CONFIG.getAcls());
    +            connectionStateListener = getConnectionStateListener();
    +            curaFramework.getConnectionStateListenable()
    +                    .addListener(connectionStateListener);
    +            unhandledErrorListener = getUnhandledErrorListener();
    +            curaFramework.getUnhandledErrorListenable()
    +                    .addListener(unhandledErrorListener);
    +            cache = new PathChildrenCache(curaFramework, CONFIG.getParentPath(), true);
    +            cache.start(PathChildrenCache.StartMode.BUILD_INITIAL_CACHE);
    +            closeAbles.add(cache);
    +            closeAbles.add(curaFramework);
    +        }catch(Exception ex){
    +            LOG.error("Exception while creating a zookeeper clients and cache",ex);
    +            if ((curaFramework != null) && (connectionStateListener != null)){
    +                curaFramework.getConnectionStateListenable()
    +                        .removeListener(connectionStateListener);
    +            }
    +            if ((curaFramework != null) && (unhandledErrorListener != null)){
    +                curaFramework.getUnhandledErrorListenable()
    +                        .removeListener(unhandledErrorListener);
    +            }
    +            for (Closeable closeable : closeAbles) {
    +                CloseableUtils.closeQuietly(closeable);
    +            }
    +        }
    +    }
    +
    +    /**
    +     * Return Singleton Load Balancer every single time.
    +     * @return LoadBalancer
    +     */
    +    public static LoadBalancer getLoadBalancer() {
    +        return loadBalancer;
    +    }
    +
    +    /**
    +     * return the location of Phoenix Query Server
    +     * in form of String. The method return a random PQS server
    +     * from the cache.
    +     * @return - PQS Location ( host:port)
    +     * @throws Exception
    +     */
    +    public PhoenixQueryServerNode getSingleServiceLocation()  throws Exception{
    +        List<PhoenixQueryServerNode> childNodes = conductSanityCheckAndReturn();
    +        // get an random connect string
    +        int i = ThreadLocalRandom.current().nextInt(0, childNodes.size());
    +        return childNodes.get(i);
    +    }
    +
    +    /**
    +     * return all the location of Phoenix Query Server from cache
    +     * @return - PQS Locations ( host1:port1,host2:port2)
    +     * @throws Exception
    +     */
    +    public List<PhoenixQueryServerNode> getAllServiceLocation()  throws Exception{
    +        return conductSanityCheckAndReturn();
    +    }
    +
    +    private List<PhoenixQueryServerNode> conductSanityCheckAndReturn() throws Exception{
    +        Preconditions.checkNotNull(curaFramework
    +                ," curator framework in not initialized ");
    +        Preconditions.checkNotNull(cache," cache value is not initialized");
    +        boolean connected = curaFramework.getZookeeperClient().isConnected();
    +        if (!connected) {
    +            String message = " Zookeeper seems to be down. The data is stale ";
    +            Exception exception =
    +                    new Exception(message);
    +            LOG.error(message, exception);
    +            throw exception;
    +        }
    +        List<String> currentNodes = curaFramework.getChildren().forPath(CONFIG.getParentPath());
    +        List<PhoenixQueryServerNode> returnNodes = new ArrayList<>();
    +        for(String node:currentNodes) {
    +            byte[] bytes = curaFramework.getData().forPath(CONFIG.getParentPath() + "/" + node);
    +            returnNodes.add(PhoenixQueryServerNode.fromJsonString(new String(
    +                    bytes, StandardCharsets.UTF_8)));
    +        }
    +        return returnNodes;
    +    }
    +    private String getZkConnectString(){
    +        return CONFIG.getZkConnectString();
    +    }
    +
    +    private ConnectionStateListener getConnectionStateListener(){
    +        return new ConnectionStateListener() {
    +            @Override
    +            public void stateChanged(CuratorFramework client, ConnectionState newState) {
    +                if (!newState.isConnected()) {
    +                    LOG.error(" connection to zookeeper broken ");
    +                }
    +            }
    +        };
    +    }
    +
    +    private UnhandledErrorListener getUnhandledErrorListener(){
    +        return new UnhandledErrorListener() {
    +            @Override
    +            public void unhandledError(String message, Throwable e) {
    +                LOG.error("unhandled exception ",e);
    --- End diff --
    
    Include the message?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: Loadbalancer

Posted by churrodog <gi...@git.apache.org>.
Github user churrodog commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r117542085
  
    --- Diff: phoenix-queryserver/pom.xml ---
    @@ -147,6 +147,14 @@
           <groupId>commons-logging</groupId>
           <artifactId>commons-logging</artifactId>
         </dependency>
    +    <dependency>
    +      <groupId>org.apache.phoenix</groupId>
    +      <artifactId>phoenix-queryserver-loadbalancer</artifactId>
    --- End diff --
    
    how do you propose sharing these configurations, which is needed by the load balancer and PQS?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: Loadbalancer

Posted by rahulsIOT <gi...@git.apache.org>.
Github user rahulsIOT commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r117437481
  
    --- Diff: phoenix-load-balancer/src/main/java/org/apache/phoenix/loadbalancer/service/LoadBalancerConfiguration.java ---
    @@ -0,0 +1,91 @@
    +/**
    + *
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.phoenix.loadbalancer.service;
    +
    +
    +
    +import org.apache.hadoop.conf.Configuration;
    +import org.apache.hadoop.hbase.HBaseConfiguration;
    +import org.apache.zookeeper.ZooDefs;
    +import org.apache.zookeeper.data.ACL;
    +import org.apache.zookeeper.data.Id;
    +import java.util.Arrays;
    +import java.util.List;
    +
    +/**
    + * Basic Configuration specific to Load Balancer only.
    + * The instance of {@link LoadBalancerConfiguration} is
    + * used as singleton so that we do not instantiate the class
    + * many times.
    + */
    +public class LoadBalancerConfiguration {
    +
    +    public  final static String clusterName = "phoenix.queryserver.base.path";
    +    public  final static String serviceName = "phoenix.queryserver.service.name";
    +    public  final static String defaultClusterName = "phoenix";
    +    public  final static String defaultServiceName = "queryserver";
    +    public  static String zkQuorum = "hbase.zookeeper.quorum";
    +    public  static String zkPort = "hbase.zookeeper.property.clientPort";
    +    public  static String zkLbUserName = "phoenix.queryserver.zookeeper.acl.username";
    +    public  static String zkLbPassword = "phoenix.queryserver.zookeeper.acl.password";
    +    public  static String defaultZkLbUserName = "phoenixuser";
    +    public  static String defaultZkLbPassword = "Xsjdhxsd";
    +    private static final Configuration configuration = HBaseConfiguration.create();
    +
    +    public LoadBalancerConfiguration() {}
    +
    +    public String getQueryServerBasePath(){
    +        return "/"+configuration.get(clusterName, defaultClusterName);
    +    }
    +
    +    public String getServiceName(){
    +        return configuration.get(serviceName, defaultServiceName);
    +    }
    +
    +    public String getZkConnectString(){
    +        return String.format("%s:%s",configuration.get(zkQuorum,"localhost"),configuration.get(zkPort,"2183"));
    --- End diff --
    
    modified to 2181. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: PHOENIX-3654 Load Balancer for thin client

Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r122076328
  
    --- Diff: phoenix-queryserver/src/main/java/org/apache/phoenix/queryserver/server/QueryServer.java ---
    @@ -233,16 +240,29 @@ public int run(String[] args) throws Exception {
           // Build and start the HttpServer
           server = builder.build();
           server.start();
    +      registerToServiceProvider(hostname);
           runningLatch.countDown();
           server.join();
           return 0;
         } catch (Throwable t) {
           LOG.fatal("Unrecoverable service error. Shutting down.", t);
           this.t = t;
           return -1;
    +    } finally {
    +      deRegister();
         }
       }
     
    +  private void registerToServiceProvider(String hostName) throws Exception {
    +      PqsZookeeperConf pqsZookeeperConf = new PqsZookeeperConfImpl(getConf());
    --- End diff --
    
    > implementation in load balancer module and interface def in query server module
    
    Correct.
    
    > Then load the implementation in queryserver module via service loader from loadbalancer module
    
    Yup
    
    > How do make query server dependent on load balancer module when load balancer has a dependency on queryserver. Is there any plugin to load the jars from loadbalancer into queryserver target
    
    This is what ServiceLoader solves. Perhaps you do not understand how ServiceLoader works? At compile time, PQS only knows about the interface. At runtime, both the interface and the implementation are present. ServiceLoader provides the implementation of the interface (based on the runtime classpath).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: PHOENIX-3654 Load Balancer for thin client

Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r129070107
  
    --- Diff: phoenix-queryserver/pom.xml ---
    @@ -176,5 +176,10 @@
           <type>test-jar</type>
           <scope>test</scope>
         </dependency>
    +    <dependency>
    +      <groupId>org.mockito</groupId>
    +      <artifactId>mockito-all</artifactId>
    --- End diff --
    
    This doesn't appear to be used to me.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: Loadbalancer

Posted by rahulsIOT <gi...@git.apache.org>.
Github user rahulsIOT commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r117436956
  
    --- Diff: phoenix-load-balancer/src/main/java/org/apache/phoenix/loadbalancer/exception/NoPhoenixQueryServerRegisteredException.java ---
    @@ -0,0 +1,15 @@
    +package org.apache.phoenix.loadbalancer.exception;
    +
    +/**
    + * Created by rshrivastava on 3/20/17.
    --- End diff --
    
    deleted.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix issue #236: PHOENIX-3654 Load Balancer for thin client

Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on the issue:

    https://github.com/apache/phoenix/pull/236
  
    Ok, thanks for the pointer, Rahul.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: Loadbalancer

Posted by rahulsIOT <gi...@git.apache.org>.
Github user rahulsIOT commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r117376467
  
    --- Diff: phoenix-queryserver/pom.xml ---
    @@ -147,6 +147,14 @@
           <groupId>commons-logging</groupId>
           <artifactId>commons-logging</artifactId>
         </dependency>
    +    <dependency>
    +      <groupId>org.apache.phoenix</groupId>
    +      <artifactId>phoenix-queryserver-loadbalancer</artifactId>
    --- End diff --
    
    If the interface is defined in phoenix-queryserver moduleand implementation in phoenix-Load-balancer module, will it not make loadbalancer module depend on phoenix-queryserver module for compile time ( via maven). Which bring us back to same issue ?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: PHOENIX-3654 Load Balancer for thin client

Posted by rahulsIOT <gi...@git.apache.org>.
Github user rahulsIOT commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r122046406
  
    --- Diff: phoenix-queryserver/src/main/java/org/apache/phoenix/loadbalancer/service/PqsZookeeperConfImpl.java ---
    @@ -0,0 +1,97 @@
    +/**
    + *
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +
    +package org.apache.phoenix.loadbalancer.service;
    +
    +import com.google.common.net.HostAndPort;
    +import org.apache.hadoop.conf.Configuration;
    +import org.apache.hadoop.hbase.HBaseConfiguration;
    +import org.apache.phoenix.query.QueryServices;
    +import org.apache.phoenix.query.QueryServicesOptions;
    +import org.apache.zookeeper.ZooDefs;
    +import org.apache.zookeeper.data.ACL;
    +import org.apache.zookeeper.data.Id;
    +
    +import java.util.Arrays;
    +import java.util.List;
    +
    +
    +
    +public class PqsZookeeperConfImpl implements PqsZookeeperConf {
    +
    +        private final Configuration configuration;
    +
    +        public PqsZookeeperConfImpl() {
    +           this.configuration = HBaseConfiguration.create();
    +        }
    +
    +        public PqsZookeeperConfImpl(Configuration configuration) {
    +            this.configuration = configuration;
    +        }
    +
    +        @Override
    +        public String getQueryServerBasePath(){
    +            return "/"+configuration.get(QueryServices.PHOENIX_QUERY_SERVER_CLUSTER_BASE_PATH,
    --- End diff --
    
    ok. will remove the pre-prending "/"


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix issue #236: PHOENIX-3654 Load Balancer for thin client

Posted by rahulsIOT <gi...@git.apache.org>.
Github user rahulsIOT commented on the issue:

    https://github.com/apache/phoenix/pull/236
  
    @joshelser  - I have rebased the code,  and file PHOENIX-4085 for SASL issue. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: Loadbalancer

Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r109748745
  
    --- Diff: phoenix-load-balancer/src/main/java/org/apache/phoenix/loadbalancer/service/LoadBalancerConfiguration.java ---
    @@ -0,0 +1,91 @@
    +/**
    + *
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.phoenix.loadbalancer.service;
    +
    +
    +
    +import org.apache.hadoop.conf.Configuration;
    +import org.apache.hadoop.hbase.HBaseConfiguration;
    +import org.apache.zookeeper.ZooDefs;
    +import org.apache.zookeeper.data.ACL;
    +import org.apache.zookeeper.data.Id;
    +import java.util.Arrays;
    +import java.util.List;
    +
    +/**
    + * Basic Configuration specific to Load Balancer only.
    + * The instance of {@link LoadBalancerConfiguration} is
    + * used as singleton so that we do not instantiate the class
    + * many times.
    + */
    +public class LoadBalancerConfiguration {
    +
    +    public  final static String clusterName = "phoenix.queryserver.base.path";
    +    public  final static String serviceName = "phoenix.queryserver.service.name";
    +    public  final static String defaultClusterName = "phoenix";
    +    public  final static String defaultServiceName = "queryserver";
    +    public  static String zkQuorum = "hbase.zookeeper.quorum";
    +    public  static String zkPort = "hbase.zookeeper.property.clientPort";
    +    public  static String zkLbUserName = "phoenix.queryserver.zookeeper.acl.username";
    +    public  static String zkLbPassword = "phoenix.queryserver.zookeeper.acl.password";
    +    public  static String defaultZkLbUserName = "phoenixuser";
    +    public  static String defaultZkLbPassword = "Xsjdhxsd";
    +    private static final Configuration configuration = HBaseConfiguration.create();
    +
    +    public LoadBalancerConfiguration() {}
    +
    +    public String getQueryServerBasePath(){
    +        return "/"+configuration.get(clusterName, defaultClusterName);
    +    }
    +
    +    public String getServiceName(){
    +        return configuration.get(serviceName, defaultServiceName);
    +    }
    +
    +    public String getZkConnectString(){
    +        return String.format("%s:%s",configuration.get(zkQuorum,"localhost"),configuration.get(zkPort,"2183"));
    +    }
    +
    +    private String getZkLbUserName(){
    +        return configuration.get(zkLbUserName,defaultZkLbUserName);
    +    }
    +
    +    private String getZkLbPassword(){
    +        return configuration.get(zkLbPassword,defaultZkLbPassword);
    +    }
    +
    +    public List<ACL> getAcls() {
    +        ACL acl = new ACL();
    +        acl.setId(new Id("digest",getZkLbUserName()+":"+getZkLbPassword()));
    --- End diff --
    
    We definitely need to support `SASL` ACLs here. Password is nice to have (for "unsecure" environments), but it might also be nice to support not ACL for testing/dev setups.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: Loadbalancer

Posted by rahulsIOT <gi...@git.apache.org>.
Github user rahulsIOT commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r117436598
  
    --- Diff: phoenix-queryserver/pom.xml ---
    @@ -147,6 +147,14 @@
           <groupId>commons-logging</groupId>
           <artifactId>commons-logging</artifactId>
         </dependency>
    +    <dependency>
    +      <groupId>org.apache.phoenix</groupId>
    +      <artifactId>phoenix-queryserver-loadbalancer</artifactId>
    --- End diff --
    
    We just need to find a way to share LoadBalancer configurations such as base path, service name, acls username and password ( and there defaults) between PQS and Load balancer. Both these projects need these details to communicate to zookeeper. We cannot put the code in QueryService/QueryServiceOptions as phoenix-core is not part of phoenix-Load-Balancer module. Let me think of better way to abstract out the code so it would be used at both phoenix-queryserver and phoenix-load-balancer modules. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: Loadbalancer

Posted by rahulsIOT <gi...@git.apache.org>.
Github user rahulsIOT commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r117402547
  
    --- Diff: phoenix-queryserver/src/main/java/org/apache/phoenix/queryserver/register/ZookeeperRegistry.java ---
    @@ -0,0 +1,73 @@
    +/**
    + *
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.phoenix.queryserver.register;
    +
    +
    +import org.apache.commons.logging.Log;
    +import org.apache.commons.logging.LogFactory;
    +import org.apache.curator.framework.CuratorFramework;
    +import org.apache.curator.framework.CuratorFrameworkFactory;
    +import org.apache.curator.retry.ExponentialBackoffRetry;
    +import org.apache.curator.utils.CloseableUtils;
    +import org.apache.phoenix.loadbalancer.service.LoadBalancerConfiguration;
    +import org.apache.phoenix.loadbalancer.service.PhoenixQueryServerNode;
    +import org.apache.phoenix.queryserver.server.QueryServer;
    +import org.apache.zookeeper.CreateMode;
    +import org.apache.zookeeper.data.Stat;
    +import java.net.InetAddress;
    +import java.nio.charset.StandardCharsets;
    +
    +
    +public class ZookeeperRegistry implements Registry {
    +
    +    protected static final Log LOG = LogFactory.getLog(QueryServer.class);
    +    private CuratorFramework client;
    +
    +    public ZookeeperRegistry(){}
    +
    +    @Override
    +    public  void registerServer(LoadBalancerConfiguration loadBalancerConfiguration, int avaticaServerPort,
    +                                String zookeeperConnectString)
    +            throws Exception {
    +
    +        this.client = CuratorFrameworkFactory.newClient(zookeeperConnectString,
    +                new ExponentialBackoffRetry(1000,10));
    +        this.client.start();
    +        String host = InetAddress.getLocalHost().getHostName();
    +        String path = loadBalancerConfiguration.getFullPathToNode(host,String.valueOf(avaticaServerPort));
    +        PhoenixQueryServerNode phoenixQueryServerNode = new PhoenixQueryServerNode(host, String.valueOf(avaticaServerPort));
    +        String node = phoenixQueryServerNode.toJsonString();
    +
    +        this.client.create().creatingParentsIfNeeded().withMode(CreateMode.EPHEMERAL).forPath(path
    +                ,node.getBytes(StandardCharsets.UTF_8));
    +        Stat stat = this.client.setACL().withACL(loadBalancerConfiguration.getAcls()).forPath(path);
    +        if (stat != null) {
    +            LOG.info(" node created with right ACL");
    +        }
    +        else {
    +            LOG.error("could not create node with right ACL");
    --- End diff --
    
    ok. agreed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: PHOENIX-3654 Load Balancer for thin client

Posted by rahulsIOT <gi...@git.apache.org>.
Github user rahulsIOT commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r122086038
  
    --- Diff: pom.xml ---
    @@ -953,6 +961,26 @@
             <artifactId>joda-time</artifactId>
             <version>${jodatime.version}</version>
           </dependency>
    +      <dependency>
    +        <groupId>org.apache.curator</groupId>
    +        <artifactId>curator-framework</artifactId>
    +        <version>${curator.version}</version>
    +      </dependency>
    +      <dependency>
    +        <groupId>org.apache.curator</groupId>
    +        <artifactId>curator-x-discovery</artifactId>
    --- End diff --
    
    removed


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: Loadbalancer

Posted by rahulsIOT <gi...@git.apache.org>.
Github user rahulsIOT commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r117377225
  
    --- Diff: pom.xml ---
    @@ -953,6 +962,30 @@
             <artifactId>joda-time</artifactId>
             <version>${jodatime.version}</version>
           </dependency>
    +      <dependency>
    +        <groupId>org.apache.curator</groupId>
    +        <artifactId>curator-framework</artifactId>
    +        <version>${curator.version}</version>
    +      </dependency>
    +      <dependency>
    +        <groupId>org.apache.curator</groupId>
    +        <artifactId>curator-x-discovery</artifactId>
    +        <version>${curator.version}</version>
    +      </dependency>
    +      <dependency>
    +        <groupId>org.apache.curator</groupId>
    +        <artifactId>curator-test</artifactId>
    +        <version>${curator.version}</version>
    +      </dependency>
    +      <dependency>
    +        <groupId>org.apache.phoenix</groupId>
    +        <artifactId>phoenix-core</artifactId>
    --- End diff --
    
    removed the phoenix-core.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: Loadbalancer

Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r109748036
  
    --- Diff: phoenix-load-balancer/src/main/java/org/apache/phoenix/loadbalancer/service/LoadBalancer.java ---
    @@ -0,0 +1,165 @@
    +/**
    + *
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.phoenix.loadbalancer.service;
    +
    +import com.google.common.base.Preconditions;
    +import com.google.common.collect.Lists;
    +import org.apache.commons.logging.Log;
    +import org.apache.commons.logging.LogFactory;
    +import org.apache.curator.framework.CuratorFramework;
    +import org.apache.curator.framework.CuratorFrameworkFactory;
    +import org.apache.curator.framework.api.UnhandledErrorListener;
    +import org.apache.curator.framework.recipes.cache.ChildData;
    +import org.apache.curator.framework.recipes.cache.PathChildrenCache;
    +import org.apache.curator.framework.recipes.cache.PathChildrenCacheEvent;
    +import org.apache.curator.framework.recipes.cache.PathChildrenCacheListener;
    +import org.apache.curator.framework.state.ConnectionState;
    +import org.apache.curator.framework.state.ConnectionStateListener;
    +import org.apache.curator.retry.ExponentialBackoffRetry;
    +import org.apache.curator.utils.CloseableUtils;
    +import org.apache.phoenix.loadbalancer.exception.NoPhoenixQueryServerRegisteredException;
    +
    +import java.nio.charset.StandardCharsets;
    +import java.util.ArrayList;
    +import java.util.concurrent.ThreadLocalRandom;
    +import java.io.Closeable;
    +import java.util.List;
    +
    +
    +public class LoadBalancer {
    +
    +    private static final LoadBalancerConfiguration CONFIG = new LoadBalancerConfiguration();
    +    private static CuratorFramework curaFramework = null;
    +    protected static final Log LOG = LogFactory.getLog(LoadBalancer.class);
    +    private static PathChildrenCache   cache = null;
    +    private static final LoadBalancer loadBalancer = new LoadBalancer();
    +
    +    private LoadBalancer()  {
    +        ConnectionStateListener connectionStateListener = null;
    +        UnhandledErrorListener unhandledErrorListener = null;
    +        List<Closeable> closeAbles = Lists.newArrayList();
    +        try  {
    +
    +            curaFramework = CuratorFrameworkFactory.newClient(getZkConnectString(),
    +                    new ExponentialBackoffRetry(1000, 3));
    +            curaFramework.start();
    +            curaFramework.setACL().withACL(CONFIG.getAcls());
    +            connectionStateListener = getConnectionStateListener();
    +            curaFramework.getConnectionStateListenable()
    +                    .addListener(connectionStateListener);
    +            unhandledErrorListener = getUnhandledErrorListener();
    +            curaFramework.getUnhandledErrorListenable()
    +                    .addListener(unhandledErrorListener);
    +            cache = new PathChildrenCache(curaFramework, CONFIG.getParentPath(), true);
    +            cache.start(PathChildrenCache.StartMode.BUILD_INITIAL_CACHE);
    +            closeAbles.add(cache);
    +            closeAbles.add(curaFramework);
    +        }catch(Exception ex){
    +            LOG.error("Exception while creating a zookeeper clients and cache",ex);
    +            if ((curaFramework != null) && (connectionStateListener != null)){
    +                curaFramework.getConnectionStateListenable()
    +                        .removeListener(connectionStateListener);
    +            }
    +            if ((curaFramework != null) && (unhandledErrorListener != null)){
    +                curaFramework.getUnhandledErrorListenable()
    +                        .removeListener(unhandledErrorListener);
    +            }
    +            for (Closeable closeable : closeAbles) {
    +                CloseableUtils.closeQuietly(closeable);
    +            }
    +        }
    +    }
    +
    +    /**
    +     * Return Singleton Load Balancer every single time.
    +     * @return LoadBalancer
    +     */
    +    public static LoadBalancer getLoadBalancer() {
    +        return loadBalancer;
    +    }
    +
    +    /**
    +     * return the location of Phoenix Query Server
    +     * in form of String. The method return a random PQS server
    +     * from the cache.
    +     * @return - PQS Location ( host:port)
    +     * @throws Exception
    +     */
    +    public PhoenixQueryServerNode getSingleServiceLocation()  throws Exception{
    +        List<PhoenixQueryServerNode> childNodes = conductSanityCheckAndReturn();
    +        // get an random connect string
    +        int i = ThreadLocalRandom.current().nextInt(0, childNodes.size());
    +        return childNodes.get(i);
    +    }
    +
    +    /**
    +     * return all the location of Phoenix Query Server from cache
    +     * @return - PQS Locations ( host1:port1,host2:port2)
    +     * @throws Exception
    +     */
    +    public List<PhoenixQueryServerNode> getAllServiceLocation()  throws Exception{
    +        return conductSanityCheckAndReturn();
    +    }
    +
    +    private List<PhoenixQueryServerNode> conductSanityCheckAndReturn() throws Exception{
    +        Preconditions.checkNotNull(curaFramework
    +                ," curator framework in not initialized ");
    +        Preconditions.checkNotNull(cache," cache value is not initialized");
    +        boolean connected = curaFramework.getZookeeperClient().isConnected();
    +        if (!connected) {
    +            String message = " Zookeeper seems to be down. The data is stale ";
    +            Exception exception =
    +                    new Exception(message);
    --- End diff --
    
    Throw a named exception here.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: Loadbalancer

Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r109748193
  
    --- Diff: phoenix-load-balancer/src/main/java/org/apache/phoenix/loadbalancer/service/LoadBalancer.java ---
    @@ -0,0 +1,165 @@
    +/**
    + *
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.phoenix.loadbalancer.service;
    +
    +import com.google.common.base.Preconditions;
    +import com.google.common.collect.Lists;
    +import org.apache.commons.logging.Log;
    +import org.apache.commons.logging.LogFactory;
    +import org.apache.curator.framework.CuratorFramework;
    +import org.apache.curator.framework.CuratorFrameworkFactory;
    +import org.apache.curator.framework.api.UnhandledErrorListener;
    +import org.apache.curator.framework.recipes.cache.ChildData;
    +import org.apache.curator.framework.recipes.cache.PathChildrenCache;
    +import org.apache.curator.framework.recipes.cache.PathChildrenCacheEvent;
    +import org.apache.curator.framework.recipes.cache.PathChildrenCacheListener;
    +import org.apache.curator.framework.state.ConnectionState;
    +import org.apache.curator.framework.state.ConnectionStateListener;
    +import org.apache.curator.retry.ExponentialBackoffRetry;
    +import org.apache.curator.utils.CloseableUtils;
    +import org.apache.phoenix.loadbalancer.exception.NoPhoenixQueryServerRegisteredException;
    +
    +import java.nio.charset.StandardCharsets;
    +import java.util.ArrayList;
    +import java.util.concurrent.ThreadLocalRandom;
    +import java.io.Closeable;
    +import java.util.List;
    +
    +
    +public class LoadBalancer {
    +
    +    private static final LoadBalancerConfiguration CONFIG = new LoadBalancerConfiguration();
    +    private static CuratorFramework curaFramework = null;
    +    protected static final Log LOG = LogFactory.getLog(LoadBalancer.class);
    +    private static PathChildrenCache   cache = null;
    +    private static final LoadBalancer loadBalancer = new LoadBalancer();
    +
    +    private LoadBalancer()  {
    +        ConnectionStateListener connectionStateListener = null;
    +        UnhandledErrorListener unhandledErrorListener = null;
    +        List<Closeable> closeAbles = Lists.newArrayList();
    +        try  {
    +
    +            curaFramework = CuratorFrameworkFactory.newClient(getZkConnectString(),
    +                    new ExponentialBackoffRetry(1000, 3));
    +            curaFramework.start();
    +            curaFramework.setACL().withACL(CONFIG.getAcls());
    +            connectionStateListener = getConnectionStateListener();
    +            curaFramework.getConnectionStateListenable()
    +                    .addListener(connectionStateListener);
    +            unhandledErrorListener = getUnhandledErrorListener();
    +            curaFramework.getUnhandledErrorListenable()
    +                    .addListener(unhandledErrorListener);
    +            cache = new PathChildrenCache(curaFramework, CONFIG.getParentPath(), true);
    +            cache.start(PathChildrenCache.StartMode.BUILD_INITIAL_CACHE);
    +            closeAbles.add(cache);
    +            closeAbles.add(curaFramework);
    +        }catch(Exception ex){
    +            LOG.error("Exception while creating a zookeeper clients and cache",ex);
    +            if ((curaFramework != null) && (connectionStateListener != null)){
    +                curaFramework.getConnectionStateListenable()
    +                        .removeListener(connectionStateListener);
    +            }
    +            if ((curaFramework != null) && (unhandledErrorListener != null)){
    +                curaFramework.getUnhandledErrorListenable()
    +                        .removeListener(unhandledErrorListener);
    +            }
    +            for (Closeable closeable : closeAbles) {
    +                CloseableUtils.closeQuietly(closeable);
    +            }
    +        }
    +    }
    +
    +    /**
    +     * Return Singleton Load Balancer every single time.
    +     * @return LoadBalancer
    +     */
    +    public static LoadBalancer getLoadBalancer() {
    +        return loadBalancer;
    +    }
    +
    +    /**
    +     * return the location of Phoenix Query Server
    +     * in form of String. The method return a random PQS server
    +     * from the cache.
    +     * @return - PQS Location ( host:port)
    +     * @throws Exception
    +     */
    +    public PhoenixQueryServerNode getSingleServiceLocation()  throws Exception{
    +        List<PhoenixQueryServerNode> childNodes = conductSanityCheckAndReturn();
    +        // get an random connect string
    +        int i = ThreadLocalRandom.current().nextInt(0, childNodes.size());
    +        return childNodes.get(i);
    +    }
    +
    +    /**
    +     * return all the location of Phoenix Query Server from cache
    +     * @return - PQS Locations ( host1:port1,host2:port2)
    +     * @throws Exception
    +     */
    +    public List<PhoenixQueryServerNode> getAllServiceLocation()  throws Exception{
    +        return conductSanityCheckAndReturn();
    +    }
    +
    +    private List<PhoenixQueryServerNode> conductSanityCheckAndReturn() throws Exception{
    +        Preconditions.checkNotNull(curaFramework
    +                ," curator framework in not initialized ");
    +        Preconditions.checkNotNull(cache," cache value is not initialized");
    +        boolean connected = curaFramework.getZookeeperClient().isConnected();
    +        if (!connected) {
    +            String message = " Zookeeper seems to be down. The data is stale ";
    +            Exception exception =
    +                    new Exception(message);
    +            LOG.error(message, exception);
    +            throw exception;
    +        }
    +        List<String> currentNodes = curaFramework.getChildren().forPath(CONFIG.getParentPath());
    +        List<PhoenixQueryServerNode> returnNodes = new ArrayList<>();
    +        for(String node:currentNodes) {
    +            byte[] bytes = curaFramework.getData().forPath(CONFIG.getParentPath() + "/" + node);
    +            returnNodes.add(PhoenixQueryServerNode.fromJsonString(new String(
    --- End diff --
    
    Probably want to try/catch around each node? You wouldn't want one bad entry to prevent access to all other good entries.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix issue #236: PHOENIX-3654 Load Balancer for thin client

Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on the issue:

    https://github.com/apache/phoenix/pull/236
  
    > The LoadBalancer.getLoadBalancer().getSingleServiceLocation() throw exception that client needs to catch. Usually it shows that there is some issue with connecting to zookeeper or there is not PQS registered with the zookeeper.
    
    Also thinking about this after the review comment about configuring the digest auth ACL for ZK, we'll need to get together documentation for this feature. This is another detail to add to the list of things to cover :)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: PHOENIX-3654 Load Balancer for thin client

Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r120395552
  
    --- Diff: pom.xml ---
    @@ -953,6 +961,26 @@
             <artifactId>joda-time</artifactId>
             <version>${jodatime.version}</version>
           </dependency>
    +      <dependency>
    +        <groupId>org.apache.curator</groupId>
    +        <artifactId>curator-framework</artifactId>
    +        <version>${curator.version}</version>
    +      </dependency>
    +      <dependency>
    +        <groupId>org.apache.curator</groupId>
    +        <artifactId>curator-x-discovery</artifactId>
    --- End diff --
    
    This appears to be unused.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: Loadbalancer

Posted by rahulsIOT <gi...@git.apache.org>.
Github user rahulsIOT commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r116869437
  
    --- Diff: phoenix-load-balancer/src/main/java/org/apache/phoenix/loadbalancer/service/PhoenixQueryServerNode.java ---
    @@ -0,0 +1,79 @@
    +/**
    + *
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.phoenix.loadbalancer.service;
    +
    +import org.codehaus.jackson.annotate.JsonProperty;
    +import org.codehaus.jackson.map.ObjectMapper;
    +import org.codehaus.jackson.map.annotate.JsonRootName;
    +
    +import java.io.IOException;
    +import java.nio.charset.StandardCharsets;
    +
    +/**
    + * Immutable class for defining the server location for
    + * Phoenix query instance. This data is stored as Node data
    + * in zookeeper
    + */
    +public class PhoenixQueryServerNode {
    +
    +    public void setHost(String host) {
    +        this.host = host;
    +    }
    +
    +    public void setPort(String port) {
    +        this.port = port;
    +    }
    +
    +    private String host;
    +    private String port;
    --- End diff --
    
    Josh. The benefit of this could be that we can pass more information such as measure of load on PQS ( cpu, load averages etc)  to load balancer. Load balancer can make more intelligent decision on where to route the next request. But, I think within this  PhoenixQueryServerNode.java class, it is good idea to use HostAndPort class from guava. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: PHOENIX-3654 Load Balancer for thin client

Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r122323098
  
    --- Diff: phoenix-queryserver/src/main/java/org/apache/phoenix/queryserver/server/QueryServer.java ---
    @@ -233,16 +240,29 @@ public int run(String[] args) throws Exception {
           // Build and start the HttpServer
           server = builder.build();
           server.start();
    +      registerToServiceProvider(hostname);
           runningLatch.countDown();
           server.join();
           return 0;
         } catch (Throwable t) {
           LOG.fatal("Unrecoverable service error. Shutting down.", t);
           this.t = t;
           return -1;
    +    } finally {
    +      deRegister();
         }
       }
     
    +  private void registerToServiceProvider(String hostName) throws Exception {
    +      PqsZookeeperConf pqsZookeeperConf = new PqsZookeeperConfImpl(getConf());
    --- End diff --
    
    Here's an example from Avatica on how it uses ServiceLoader to pull Metrics backend systems:
    
    A [factory interface is defined in one module](https://github.com/apache/calcite-avatica/blob/master/metrics/src/main/java/org/apache/calcite/avatica/metrics/MetricsSystemFactory.java), an [implementation is defined in a different module](https://github.com/apache/calcite-avatica/blob/master/metrics-dropwizardmetrics3/src/main/java/org/apache/calcite/avatica/metrics/dropwizard3/DropwizardMetricsSystemFactory.java), a [services file is created stating that the implementation is present for the interface](https://github.com/apache/calcite-avatica/blob/master/metrics-dropwizardmetrics3/src/main/resources/META-INF/services/org.apache.calcite.avatica.metrics.MetricsSystemFactory), the Avatica server [loads an implementation of the interface using ServiceLoader](https://github.com/apache/calcite-avatica/blob/master/server/src/main/java/org/apache/calcite/avatica/server/HandlerFactory.java#L114)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: PHOENIX-3654 Load Balancer for thin client

Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r130726475
  
    --- Diff: phoenix-core/src/main/java/org/apache/phoenix/query/QueryServicesOptions.java ---
    @@ -284,14 +288,18 @@
         public static final int DEFAULT_COLUMN_ENCODED_BYTES = QualifierEncodingScheme.TWO_BYTE_QUALIFIERS.getSerializedMetadataValue();
         public static final String DEFAULT_IMMUTABLE_STORAGE_SCHEME = ImmutableStorageScheme.SINGLE_CELL_ARRAY_WITH_OFFSETS.toString();
         public static final String DEFAULT_MULTITENANT_IMMUTABLE_STORAGE_SCHEME = ImmutableStorageScheme.ONE_CELL_PER_COLUMN.toString();
    +    public  final static String DEFAULT_PHOENIX_QUERY_SERVER_CLUSTER_BASE_PATH = "/phoenix";
    +    public  final static String DEFAULT_PHOENIX_QUERY_SERVER_SERVICE_NAME = "queryserver";
    +    public  final static String DEFAULT_PHOENIX_QUERY_SERVER_ZK_ACL_USERNAME = "phoenixuser";
    +    public  final static String DEFAULT_PHOENIX_QUERY_SERVER_ZK_ACL_PASSWORD = "Xsjdhxsd";
    --- End diff --
    
    > I replied saying that we can close this PR and open another ticket, where I will fix the SASL ACLs.
    
    Implementing SASL acls is one thing -- my above comment was about not forgetting to document the lack of initial support (much less the feature itself).
    
    We should make sure that a documentation issue gets filed, not that you build something that rots because no one knows how to use it!


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: PHOENIX-3654 Load Balancer for thin client

Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r120393196
  
    --- Diff: phoenix-queryserver/src/main/java/org/apache/phoenix/queryserver/register/ZookeeperRegistry.java ---
    @@ -0,0 +1,72 @@
    +/**
    + *
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.phoenix.queryserver.register;
    +
    +
    +import com.google.common.net.HostAndPort;
    +import org.apache.commons.logging.Log;
    +import org.apache.commons.logging.LogFactory;
    +import org.apache.curator.framework.CuratorFramework;
    +import org.apache.curator.framework.CuratorFrameworkFactory;
    +import org.apache.curator.retry.ExponentialBackoffRetry;
    +import org.apache.curator.utils.CloseableUtils;
    +import org.apache.phoenix.loadbalancer.service.PqsZookeeperConf;
    +import org.apache.phoenix.queryserver.server.QueryServer;
    +import org.apache.zookeeper.CreateMode;
    +import org.apache.zookeeper.data.Stat;
    +
    +import java.nio.charset.StandardCharsets;
    +
    +
    +public class ZookeeperRegistry implements Registry {
    --- End diff --
    
    Again, shouldn't this be in the loadbalancer module?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: PHOENIX-3654 Load Balancer for thin client

Posted by rahulsIOT <gi...@git.apache.org>.
Github user rahulsIOT commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r122048516
  
    --- Diff: phoenix-queryserver/src/main/java/org/apache/phoenix/loadbalancer/service/PqsZookeeperConfImpl.java ---
    @@ -0,0 +1,97 @@
    +/**
    + *
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +
    +package org.apache.phoenix.loadbalancer.service;
    +
    +import com.google.common.net.HostAndPort;
    +import org.apache.hadoop.conf.Configuration;
    +import org.apache.hadoop.hbase.HBaseConfiguration;
    +import org.apache.phoenix.query.QueryServices;
    +import org.apache.phoenix.query.QueryServicesOptions;
    +import org.apache.zookeeper.ZooDefs;
    +import org.apache.zookeeper.data.ACL;
    +import org.apache.zookeeper.data.Id;
    +
    +import java.util.Arrays;
    +import java.util.List;
    +
    +
    +
    +public class PqsZookeeperConfImpl implements PqsZookeeperConf {
    --- End diff --
    
    Loadbalancer has dependency in query server module. Now , if we move this module in Load balancer, the query server cannot use this module without putting  a dependency on it. If you can articulate how you prefer the module dependencies to be laid out, I can look into refractoring the code. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: PHOENIX-3654 Load Balancer for thin client

Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r120392825
  
    --- Diff: phoenix-queryserver/src/main/java/org/apache/phoenix/loadbalancer/service/PqsZookeeperConfImpl.java ---
    @@ -0,0 +1,97 @@
    +/**
    + *
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +
    +package org.apache.phoenix.loadbalancer.service;
    +
    +import com.google.common.net.HostAndPort;
    +import org.apache.hadoop.conf.Configuration;
    +import org.apache.hadoop.hbase.HBaseConfiguration;
    +import org.apache.phoenix.query.QueryServices;
    +import org.apache.phoenix.query.QueryServicesOptions;
    +import org.apache.zookeeper.ZooDefs;
    +import org.apache.zookeeper.data.ACL;
    +import org.apache.zookeeper.data.Id;
    +
    +import java.util.Arrays;
    +import java.util.List;
    +
    +
    +
    +public class PqsZookeeperConfImpl implements PqsZookeeperConf {
    --- End diff --
    
    This implementation should be in phoenix-loadbalancer, no?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix issue #236: PHOENIX-3654 Load Balancer for thin client

Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on the issue:

    https://github.com/apache/phoenix/pull/236
  
    Thanks for the fixes in f042ca0, @rahulsIOT.
    
    Github is telling me that you have some conflicts that need resolving. If you can do that and file the JIRA case about firming up the ACL support, I'll try to test+merge this today before the week ramps up.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: Loadbalancer

Posted by rahulsIOT <gi...@git.apache.org>.
Github user rahulsIOT commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r117371944
  
    --- Diff: phoenix-load-balancer/src/main/java/org/apache/phoenix/loadbalancer/service/LoadBalancerConfiguration.java ---
    @@ -0,0 +1,91 @@
    +/**
    + *
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.phoenix.loadbalancer.service;
    +
    +
    +
    +import org.apache.hadoop.conf.Configuration;
    +import org.apache.hadoop.hbase.HBaseConfiguration;
    +import org.apache.zookeeper.ZooDefs;
    +import org.apache.zookeeper.data.ACL;
    +import org.apache.zookeeper.data.Id;
    +import java.util.Arrays;
    +import java.util.List;
    +
    +/**
    + * Basic Configuration specific to Load Balancer only.
    + * The instance of {@link LoadBalancerConfiguration} is
    + * used as singleton so that we do not instantiate the class
    + * many times.
    + */
    +public class LoadBalancerConfiguration {
    +
    +    public  final static String clusterName = "phoenix.queryserver.base.path";
    +    public  final static String serviceName = "phoenix.queryserver.service.name";
    +    public  final static String defaultClusterName = "phoenix";
    +    public  final static String defaultServiceName = "queryserver";
    +    public  static String zkQuorum = "hbase.zookeeper.quorum";
    +    public  static String zkPort = "hbase.zookeeper.property.clientPort";
    +    public  static String zkLbUserName = "phoenix.queryserver.zookeeper.acl.username";
    +    public  static String zkLbPassword = "phoenix.queryserver.zookeeper.acl.password";
    +    public  static String defaultZkLbUserName = "phoenixuser";
    +    public  static String defaultZkLbPassword = "Xsjdhxsd";
    +    private static final Configuration configuration = HBaseConfiguration.create();
    +
    +    public LoadBalancerConfiguration() {}
    +
    +    public String getQueryServerBasePath(){
    +        return "/"+configuration.get(clusterName, defaultClusterName);
    +    }
    +
    +    public String getServiceName(){
    +        return configuration.get(serviceName, defaultServiceName);
    +    }
    +
    +    public String getZkConnectString(){
    +        return String.format("%s:%s",configuration.get(zkQuorum,"localhost"),configuration.get(zkPort,"2183"));
    +    }
    +
    +    private String getZkLbUserName(){
    +        return configuration.get(zkLbUserName,defaultZkLbUserName);
    +    }
    +
    +    private String getZkLbPassword(){
    +        return configuration.get(zkLbPassword,defaultZkLbPassword);
    +    }
    +
    +    public List<ACL> getAcls() {
    +        ACL acl = new ACL();
    +        acl.setId(new Id("digest",getZkLbUserName()+":"+getZkLbPassword()));
    --- End diff --
    
    Agreed. Let us move this to another jira, which I will log and work on after Loadbalancer patch is accepted. Let me know if that make sense ? 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: PHOENIX-3654 Load Balancer for thin client

Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r129068996
  
    --- Diff: phoenix-load-balancer/src/main/java/org/apache/phoenix/queryserver/register/ZookeeperRegistry.java ---
    @@ -0,0 +1,72 @@
    +/**
    + *
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.phoenix.queryserver.register;
    +
    +
    +import com.google.common.net.HostAndPort;
    +import org.apache.commons.logging.Log;
    +import org.apache.commons.logging.LogFactory;
    +import org.apache.curator.framework.CuratorFramework;
    +import org.apache.curator.framework.CuratorFrameworkFactory;
    +import org.apache.curator.retry.ExponentialBackoffRetry;
    +import org.apache.curator.utils.CloseableUtils;
    +import org.apache.phoenix.loadbalancer.service.LoadBalanceZookeeperConf;
    +import org.apache.phoenix.queryserver.server.QueryServer;
    +import org.apache.zookeeper.CreateMode;
    +import org.apache.zookeeper.data.Stat;
    +
    +import java.nio.charset.StandardCharsets;
    +
    +
    +public class ZookeeperRegistry implements Registry {
    +
    +    protected static final Log LOG = LogFactory.getLog(QueryServer.class);
    +    private CuratorFramework client;
    +
    +    public ZookeeperRegistry(){}
    +
    +    @Override
    +    public  void registerServer(LoadBalanceZookeeperConf configuration, int pqsPort,
    +                                String zookeeperConnectString, String pqsHost)
    +            throws Exception {
    +
    +        this.client = CuratorFrameworkFactory.newClient(zookeeperConnectString,
    +                new ExponentialBackoffRetry(1000,10));
    +        this.client.start();
    +        HostAndPort hostAndPort = HostAndPort.fromParts(pqsHost,pqsPort);
    +        String path = configuration.getFullPathToNode(hostAndPort);
    +        String node = hostAndPort.toString();
    +        this.client.create().creatingParentsIfNeeded().withMode(CreateMode.EPHEMERAL).forPath(path
    +                ,node.getBytes(StandardCharsets.UTF_8));
    +        Stat stat = this.client.setACL().withACL(configuration.getAcls()).forPath(path);
    +        if (stat != null) {
    +            LOG.info(" node created with right ACL");
    +        }
    +        else {
    +            LOG.error("could not create node with right ACL. So, system would exit now.");
    +            System.exit(-1);
    --- End diff --
    
    Calling `System.exit(-1)` is pretty brutal for anyone trying to integrate with your software :)
    
    Throwing a RuntimeException would be nicer.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: PHOENIX-3654 Load Balancer for thin client

Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r129068605
  
    --- Diff: phoenix-load-balancer/pom.xml ---
    @@ -0,0 +1,84 @@
    +<?xml version='1.0'?>
    +<!--
    +
    + Licensed to the Apache Software Foundation (ASF) under one
    + or more contributor license agreements.  See the NOTICE file
    + distributed with this work for additional information
    + regarding copyright ownership.  The ASF licenses this file
    + to you under the Apache License, Version 2.0 (the
    + "License"); you may not use this file except in compliance
    + with the License.  You may obtain a copy of the License at
    +
    +   http://www.apache.org/licenses/LICENSE-2.0
    +
    + Unless required by applicable law or agreed to in writing,
    + software distributed under the License is distributed on an
    + "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + KIND, either express or implied.  See the License for the
    + specific language governing permissions and limitations
    + under the License.
    +
    +-->
    +
    +<project xmlns="http://maven.apache.org/POM/4.0.0"
    +         xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
    +         xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
    +  <modelVersion>4.0.0</modelVersion>
    +  <parent>
    +    <groupId>org.apache.phoenix</groupId>
    +    <artifactId>phoenix</artifactId>
    +    <version>4.11.0-HBase-1.3-SNAPSHOT</version>
    +  </parent>
    +  <artifactId>phoenix-load-balancer</artifactId>
    +  <name>Phoenix Load Balancer</name>
    +  <description>A Load balancer which routes calls to Phoenix Query Server</description>
    +
    +  <dependencies>
    +    <dependency>
    +      <groupId>org.apache.hbase</groupId>
    +      <artifactId>hbase-common</artifactId>
    --- End diff --
    
    It looks like you're using a bunch of dependencies transitively. This will work for now, but is brittle in the long term.
    
    We should make sure that this module depends directly on all of the dependencies that it uses instead of relying on another dependency bringing them in. Can do this later though.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: PHOENIX-3654 Load Balancer for thin client

Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r120396712
  
    --- Diff: phoenix-queryserver/src/main/java/org/apache/phoenix/loadbalancer/service/PqsZookeeperConf.java ---
    @@ -0,0 +1,42 @@
    +/**
    + *
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.phoenix.loadbalancer.service;
    +
    +import com.google.common.net.HostAndPort;
    +import org.apache.zookeeper.data.ACL;
    +
    +import java.util.List;
    +
    +
    +public interface PqsZookeeperConf {
    --- End diff --
    
    Nit: Suggest "LoadBalanceZookeeperConf" instead of "PqsZookeeperConf".


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix issue #236: PHOENIX-3654 Load Balancer for thin client

Posted by rahulsIOT <gi...@git.apache.org>.
Github user rahulsIOT commented on the issue:

    https://github.com/apache/phoenix/pull/236
  
    Thin client can call " LoadBalancer.getLoadBalancer().getSingleServiceLocation() " and get hold of host and port for PQS location. It can user DriverManager.getConnection to get connection object. The LoadBalancer.getLoadBalancer().getSingleServiceLocation() throw exception that client needs to catch. Usually it shows that there is some issue with connecting to zookeeper or there is not PQS registered with the zookeeper. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: Loadbalancer

Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r109747859
  
    --- Diff: phoenix-load-balancer/src/main/java/org/apache/phoenix/loadbalancer/service/LoadBalancer.java ---
    @@ -0,0 +1,165 @@
    +/**
    + *
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.phoenix.loadbalancer.service;
    +
    +import com.google.common.base.Preconditions;
    +import com.google.common.collect.Lists;
    +import org.apache.commons.logging.Log;
    +import org.apache.commons.logging.LogFactory;
    +import org.apache.curator.framework.CuratorFramework;
    +import org.apache.curator.framework.CuratorFrameworkFactory;
    +import org.apache.curator.framework.api.UnhandledErrorListener;
    +import org.apache.curator.framework.recipes.cache.ChildData;
    +import org.apache.curator.framework.recipes.cache.PathChildrenCache;
    +import org.apache.curator.framework.recipes.cache.PathChildrenCacheEvent;
    +import org.apache.curator.framework.recipes.cache.PathChildrenCacheListener;
    +import org.apache.curator.framework.state.ConnectionState;
    +import org.apache.curator.framework.state.ConnectionStateListener;
    +import org.apache.curator.retry.ExponentialBackoffRetry;
    +import org.apache.curator.utils.CloseableUtils;
    +import org.apache.phoenix.loadbalancer.exception.NoPhoenixQueryServerRegisteredException;
    +
    +import java.nio.charset.StandardCharsets;
    +import java.util.ArrayList;
    +import java.util.concurrent.ThreadLocalRandom;
    +import java.io.Closeable;
    +import java.util.List;
    +
    +
    +public class LoadBalancer {
    +
    +    private static final LoadBalancerConfiguration CONFIG = new LoadBalancerConfiguration();
    +    private static CuratorFramework curaFramework = null;
    +    protected static final Log LOG = LogFactory.getLog(LoadBalancer.class);
    +    private static PathChildrenCache   cache = null;
    +    private static final LoadBalancer loadBalancer = new LoadBalancer();
    +
    +    private LoadBalancer()  {
    +        ConnectionStateListener connectionStateListener = null;
    +        UnhandledErrorListener unhandledErrorListener = null;
    +        List<Closeable> closeAbles = Lists.newArrayList();
    +        try  {
    +
    +            curaFramework = CuratorFrameworkFactory.newClient(getZkConnectString(),
    --- End diff --
    
    Move this into a `start()` method. Doing this all in the constructor just leads to problems.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: Loadbalancer

Posted by rahulsIOT <gi...@git.apache.org>.
Github user rahulsIOT commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r117436975
  
    --- Diff: phoenix-load-balancer/src/main/java/org/apache/phoenix/loadbalancer/service/LoadBalancer.java ---
    @@ -0,0 +1,165 @@
    +/**
    + *
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.phoenix.loadbalancer.service;
    +
    +import com.google.common.base.Preconditions;
    +import com.google.common.collect.Lists;
    +import org.apache.commons.logging.Log;
    +import org.apache.commons.logging.LogFactory;
    +import org.apache.curator.framework.CuratorFramework;
    +import org.apache.curator.framework.CuratorFrameworkFactory;
    +import org.apache.curator.framework.api.UnhandledErrorListener;
    +import org.apache.curator.framework.recipes.cache.ChildData;
    +import org.apache.curator.framework.recipes.cache.PathChildrenCache;
    +import org.apache.curator.framework.recipes.cache.PathChildrenCacheEvent;
    +import org.apache.curator.framework.recipes.cache.PathChildrenCacheListener;
    +import org.apache.curator.framework.state.ConnectionState;
    +import org.apache.curator.framework.state.ConnectionStateListener;
    +import org.apache.curator.retry.ExponentialBackoffRetry;
    +import org.apache.curator.utils.CloseableUtils;
    +import org.apache.phoenix.loadbalancer.exception.NoPhoenixQueryServerRegisteredException;
    +
    +import java.nio.charset.StandardCharsets;
    +import java.util.ArrayList;
    +import java.util.concurrent.ThreadLocalRandom;
    +import java.io.Closeable;
    +import java.util.List;
    +
    +
    +public class LoadBalancer {
    +
    +    private static final LoadBalancerConfiguration CONFIG = new LoadBalancerConfiguration();
    +    private static CuratorFramework curaFramework = null;
    +    protected static final Log LOG = LogFactory.getLog(LoadBalancer.class);
    +    private static PathChildrenCache   cache = null;
    +    private static final LoadBalancer loadBalancer = new LoadBalancer();
    +
    +    private LoadBalancer()  {
    +        ConnectionStateListener connectionStateListener = null;
    +        UnhandledErrorListener unhandledErrorListener = null;
    +        List<Closeable> closeAbles = Lists.newArrayList();
    +        try  {
    +
    +            curaFramework = CuratorFrameworkFactory.newClient(getZkConnectString(),
    --- End diff --
    
    done.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: Loadbalancer

Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r109750216
  
    --- Diff: pom.xml ---
    @@ -127,6 +128,9 @@
         <!-- Set default encoding so multi-byte tests work correctly on the Mac -->
         <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
         <project.reporting.outputEncoding>UTF-8</project.reporting.outputEncoding>
    +
    --- End diff --
    
    nit: unnecessary spaces


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: Loadbalancer

Posted by rahulsIOT <gi...@git.apache.org>.
Github user rahulsIOT commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r117405732
  
    --- Diff: phoenix-queryserver/src/main/java/org/apache/phoenix/queryserver/register/Registry.java ---
    @@ -0,0 +1,31 @@
    +/**
    + *
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.phoenix.queryserver.register;
    +
    +
    +import org.apache.phoenix.loadbalancer.service.LoadBalancerConfiguration;
    +
    +public interface  Registry  {
    +
    +    public void deRegisterTheServer() throws Exception;
    --- End diff --
    
    No really. it should just close the curator client connection and by that remove the emphemeral node. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: Loadbalancer

Posted by rahulsIOT <gi...@git.apache.org>.
Github user rahulsIOT commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r117377275
  
    --- Diff: pom.xml ---
    @@ -127,6 +128,9 @@
         <!-- Set default encoding so multi-byte tests work correctly on the Mac -->
         <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
         <project.reporting.outputEncoding>UTF-8</project.reporting.outputEncoding>
    +
    --- End diff --
    
    removed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: Loadbalancer

Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r109749485
  
    --- Diff: phoenix-queryserver/src/it/java/org/apache/phoenix/end2end/QueryServerBasicsIT.java ---
    @@ -162,4 +186,11 @@ public void smokeTest() throws Exception {
           }
         }
       }
    +
    +  @Test
    +  public void testIfRegisteredWithZookeeper() throws Exception {
    +    Stat stat = curatorFramework.checkExists().forPath(loadBalancerConfig.getQueryServerBasePath());
    --- End diff --
    
    Shouldn't this check for the child znode, not just the root path?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: PHOENIX-3654 Load Balancer for thin client

Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r129067645
  
    --- Diff: phoenix-core/src/main/java/org/apache/phoenix/query/QueryServicesOptions.java ---
    @@ -284,14 +288,18 @@
         public static final int DEFAULT_COLUMN_ENCODED_BYTES = QualifierEncodingScheme.TWO_BYTE_QUALIFIERS.getSerializedMetadataValue();
         public static final String DEFAULT_IMMUTABLE_STORAGE_SCHEME = ImmutableStorageScheme.SINGLE_CELL_ARRAY_WITH_OFFSETS.toString();
         public static final String DEFAULT_MULTITENANT_IMMUTABLE_STORAGE_SCHEME = ImmutableStorageScheme.ONE_CELL_PER_COLUMN.toString();
    +    public  final static String DEFAULT_PHOENIX_QUERY_SERVER_CLUSTER_BASE_PATH = "/phoenix";
    +    public  final static String DEFAULT_PHOENIX_QUERY_SERVER_SERVICE_NAME = "queryserver";
    +    public  final static String DEFAULT_PHOENIX_QUERY_SERVER_ZK_ACL_USERNAME = "phoenixuser";
    +    public  final static String DEFAULT_PHOENIX_QUERY_SERVER_ZK_ACL_PASSWORD = "Xsjdhxsd";
    --- End diff --
    
    I think we should just change this to be something simple like "phoenix/phoenix" instead of "phoenixuser/Xsjdhxsd" (the public random text is no-more secure than an easily remember-able string).
    
    We also need to document how users are expected to secure this (property to set in hbase-site.xml, make sure hbase-site.xml is not globally readable, etc). I'm seeing down below that there is not support for SASL ACLs -- that will also need to be documented as a limitation.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: PHOENIX-3654 Load Balancer for thin client

Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r120394867
  
    --- Diff: phoenix-queryserver/pom.xml ---
    @@ -147,6 +147,10 @@
           <groupId>commons-logging</groupId>
           <artifactId>commons-logging</artifactId>
         </dependency>
    +    <dependency>
    +      <groupId>org.apache.curator</groupId>
    +      <artifactId>curator-client</artifactId>
    --- End diff --
    
    Assuming you follow my suggestion about using service loader, the curator dependencies can be removed from this pom.xml.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: PHOENIX-3654 Load Balancer for thin client

Posted by rahulsIOT <gi...@git.apache.org>.
Github user rahulsIOT commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r130728526
  
    --- Diff: phoenix-core/src/main/java/org/apache/phoenix/query/QueryServicesOptions.java ---
    @@ -284,14 +288,18 @@
         public static final int DEFAULT_COLUMN_ENCODED_BYTES = QualifierEncodingScheme.TWO_BYTE_QUALIFIERS.getSerializedMetadataValue();
         public static final String DEFAULT_IMMUTABLE_STORAGE_SCHEME = ImmutableStorageScheme.SINGLE_CELL_ARRAY_WITH_OFFSETS.toString();
         public static final String DEFAULT_MULTITENANT_IMMUTABLE_STORAGE_SCHEME = ImmutableStorageScheme.ONE_CELL_PER_COLUMN.toString();
    +    public  final static String DEFAULT_PHOENIX_QUERY_SERVER_CLUSTER_BASE_PATH = "/phoenix";
    +    public  final static String DEFAULT_PHOENIX_QUERY_SERVER_SERVICE_NAME = "queryserver";
    +    public  final static String DEFAULT_PHOENIX_QUERY_SERVER_ZK_ACL_USERNAME = "phoenixuser";
    +    public  final static String DEFAULT_PHOENIX_QUERY_SERVER_ZK_ACL_PASSWORD = "Xsjdhxsd";
    --- End diff --
    
    ok. We need to file a doc bug with the details about LB and its limitations. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: Loadbalancer

Posted by rahulsIOT <gi...@git.apache.org>.
Github user rahulsIOT commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r117372183
  
    --- Diff: phoenix-load-balancer/src/main/java/org/apache/phoenix/loadbalancer/service/LoadBalancerConfiguration.java ---
    @@ -0,0 +1,91 @@
    +/**
    + *
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.phoenix.loadbalancer.service;
    +
    +
    +
    +import org.apache.hadoop.conf.Configuration;
    +import org.apache.hadoop.hbase.HBaseConfiguration;
    +import org.apache.zookeeper.ZooDefs;
    +import org.apache.zookeeper.data.ACL;
    +import org.apache.zookeeper.data.Id;
    +import java.util.Arrays;
    +import java.util.List;
    +
    +/**
    + * Basic Configuration specific to Load Balancer only.
    + * The instance of {@link LoadBalancerConfiguration} is
    + * used as singleton so that we do not instantiate the class
    + * many times.
    + */
    +public class LoadBalancerConfiguration {
    +
    +    public  final static String clusterName = "phoenix.queryserver.base.path";
    +    public  final static String serviceName = "phoenix.queryserver.service.name";
    +    public  final static String defaultClusterName = "phoenix";
    +    public  final static String defaultServiceName = "queryserver";
    +    public  static String zkQuorum = "hbase.zookeeper.quorum";
    +    public  static String zkPort = "hbase.zookeeper.property.clientPort";
    +    public  static String zkLbUserName = "phoenix.queryserver.zookeeper.acl.username";
    +    public  static String zkLbPassword = "phoenix.queryserver.zookeeper.acl.password";
    +    public  static String defaultZkLbUserName = "phoenixuser";
    +    public  static String defaultZkLbPassword = "Xsjdhxsd";
    +    private static final Configuration configuration = HBaseConfiguration.create();
    +
    +    public LoadBalancerConfiguration() {}
    +
    +    public String getQueryServerBasePath(){
    +        return "/"+configuration.get(clusterName, defaultClusterName);
    +    }
    +
    +    public String getServiceName(){
    +        return configuration.get(serviceName, defaultServiceName);
    +    }
    +
    +    public String getZkConnectString(){
    +        return String.format("%s:%s",configuration.get(zkQuorum,"localhost"),configuration.get(zkPort,"2183"));
    +    }
    +
    +    private String getZkLbUserName(){
    +        return configuration.get(zkLbUserName,defaultZkLbUserName);
    +    }
    +
    +    private String getZkLbPassword(){
    +        return configuration.get(zkLbPassword,defaultZkLbPassword);
    +    }
    +
    +    public List<ACL> getAcls() {
    +        ACL acl = new ACL();
    +        acl.setId(new Id("digest",getZkLbUserName()+":"+getZkLbPassword()));
    +        acl.setPerms(ZooDefs.Perms.READ);
    +        return Arrays.asList(acl);
    +    }
    +
    +    public String getParentPath() {
    +        String path = String.format("%s/%s",getQueryServerBasePath(),getServiceName());
    +        return path;
    +    }
    +
    +    public String getFullPathToNode(String host,String avaticaServerPort) {
    --- End diff --
    
    Changed this to Guava HostAndPort. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: Loadbalancer

Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r109748840
  
    --- Diff: phoenix-load-balancer/src/main/java/org/apache/phoenix/loadbalancer/service/LoadBalancerConfiguration.java ---
    @@ -0,0 +1,91 @@
    +/**
    + *
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.phoenix.loadbalancer.service;
    +
    +
    +
    +import org.apache.hadoop.conf.Configuration;
    +import org.apache.hadoop.hbase.HBaseConfiguration;
    +import org.apache.zookeeper.ZooDefs;
    +import org.apache.zookeeper.data.ACL;
    +import org.apache.zookeeper.data.Id;
    +import java.util.Arrays;
    +import java.util.List;
    +
    +/**
    + * Basic Configuration specific to Load Balancer only.
    + * The instance of {@link LoadBalancerConfiguration} is
    + * used as singleton so that we do not instantiate the class
    + * many times.
    + */
    +public class LoadBalancerConfiguration {
    +
    +    public  final static String clusterName = "phoenix.queryserver.base.path";
    +    public  final static String serviceName = "phoenix.queryserver.service.name";
    +    public  final static String defaultClusterName = "phoenix";
    +    public  final static String defaultServiceName = "queryserver";
    +    public  static String zkQuorum = "hbase.zookeeper.quorum";
    +    public  static String zkPort = "hbase.zookeeper.property.clientPort";
    +    public  static String zkLbUserName = "phoenix.queryserver.zookeeper.acl.username";
    +    public  static String zkLbPassword = "phoenix.queryserver.zookeeper.acl.password";
    +    public  static String defaultZkLbUserName = "phoenixuser";
    +    public  static String defaultZkLbPassword = "Xsjdhxsd";
    +    private static final Configuration configuration = HBaseConfiguration.create();
    +
    +    public LoadBalancerConfiguration() {}
    +
    +    public String getQueryServerBasePath(){
    +        return "/"+configuration.get(clusterName, defaultClusterName);
    +    }
    +
    +    public String getServiceName(){
    +        return configuration.get(serviceName, defaultServiceName);
    +    }
    +
    +    public String getZkConnectString(){
    +        return String.format("%s:%s",configuration.get(zkQuorum,"localhost"),configuration.get(zkPort,"2183"));
    --- End diff --
    
    Why default to 2183 and not the standard 2181?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: Loadbalancer

Posted by rahulsIOT <gi...@git.apache.org>.
Github user rahulsIOT commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r117437408
  
    --- Diff: phoenix-load-balancer/src/main/java/org/apache/phoenix/loadbalancer/service/LoadBalancer.java ---
    @@ -0,0 +1,165 @@
    +/**
    + *
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.phoenix.loadbalancer.service;
    +
    +import com.google.common.base.Preconditions;
    +import com.google.common.collect.Lists;
    +import org.apache.commons.logging.Log;
    +import org.apache.commons.logging.LogFactory;
    +import org.apache.curator.framework.CuratorFramework;
    +import org.apache.curator.framework.CuratorFrameworkFactory;
    +import org.apache.curator.framework.api.UnhandledErrorListener;
    +import org.apache.curator.framework.recipes.cache.ChildData;
    +import org.apache.curator.framework.recipes.cache.PathChildrenCache;
    +import org.apache.curator.framework.recipes.cache.PathChildrenCacheEvent;
    +import org.apache.curator.framework.recipes.cache.PathChildrenCacheListener;
    +import org.apache.curator.framework.state.ConnectionState;
    +import org.apache.curator.framework.state.ConnectionStateListener;
    +import org.apache.curator.retry.ExponentialBackoffRetry;
    +import org.apache.curator.utils.CloseableUtils;
    +import org.apache.phoenix.loadbalancer.exception.NoPhoenixQueryServerRegisteredException;
    +
    +import java.nio.charset.StandardCharsets;
    +import java.util.ArrayList;
    +import java.util.concurrent.ThreadLocalRandom;
    +import java.io.Closeable;
    +import java.util.List;
    +
    +
    +public class LoadBalancer {
    +
    +    private static final LoadBalancerConfiguration CONFIG = new LoadBalancerConfiguration();
    +    private static CuratorFramework curaFramework = null;
    +    protected static final Log LOG = LogFactory.getLog(LoadBalancer.class);
    +    private static PathChildrenCache   cache = null;
    +    private static final LoadBalancer loadBalancer = new LoadBalancer();
    +
    +    private LoadBalancer()  {
    +        ConnectionStateListener connectionStateListener = null;
    +        UnhandledErrorListener unhandledErrorListener = null;
    +        List<Closeable> closeAbles = Lists.newArrayList();
    +        try  {
    +
    +            curaFramework = CuratorFrameworkFactory.newClient(getZkConnectString(),
    +                    new ExponentialBackoffRetry(1000, 3));
    +            curaFramework.start();
    +            curaFramework.setACL().withACL(CONFIG.getAcls());
    +            connectionStateListener = getConnectionStateListener();
    +            curaFramework.getConnectionStateListenable()
    +                    .addListener(connectionStateListener);
    +            unhandledErrorListener = getUnhandledErrorListener();
    +            curaFramework.getUnhandledErrorListenable()
    +                    .addListener(unhandledErrorListener);
    +            cache = new PathChildrenCache(curaFramework, CONFIG.getParentPath(), true);
    +            cache.start(PathChildrenCache.StartMode.BUILD_INITIAL_CACHE);
    +            closeAbles.add(cache);
    +            closeAbles.add(curaFramework);
    +        }catch(Exception ex){
    +            LOG.error("Exception while creating a zookeeper clients and cache",ex);
    +            if ((curaFramework != null) && (connectionStateListener != null)){
    +                curaFramework.getConnectionStateListenable()
    +                        .removeListener(connectionStateListener);
    +            }
    +            if ((curaFramework != null) && (unhandledErrorListener != null)){
    +                curaFramework.getUnhandledErrorListenable()
    +                        .removeListener(unhandledErrorListener);
    +            }
    +            for (Closeable closeable : closeAbles) {
    +                CloseableUtils.closeQuietly(closeable);
    +            }
    +        }
    +    }
    +
    +    /**
    +     * Return Singleton Load Balancer every single time.
    +     * @return LoadBalancer
    +     */
    +    public static LoadBalancer getLoadBalancer() {
    +        return loadBalancer;
    +    }
    +
    +    /**
    +     * return the location of Phoenix Query Server
    +     * in form of String. The method return a random PQS server
    +     * from the cache.
    +     * @return - PQS Location ( host:port)
    +     * @throws Exception
    +     */
    +    public PhoenixQueryServerNode getSingleServiceLocation()  throws Exception{
    +        List<PhoenixQueryServerNode> childNodes = conductSanityCheckAndReturn();
    +        // get an random connect string
    +        int i = ThreadLocalRandom.current().nextInt(0, childNodes.size());
    +        return childNodes.get(i);
    +    }
    +
    +    /**
    +     * return all the location of Phoenix Query Server from cache
    +     * @return - PQS Locations ( host1:port1,host2:port2)
    +     * @throws Exception
    +     */
    +    public List<PhoenixQueryServerNode> getAllServiceLocation()  throws Exception{
    +        return conductSanityCheckAndReturn();
    +    }
    +
    +    private List<PhoenixQueryServerNode> conductSanityCheckAndReturn() throws Exception{
    +        Preconditions.checkNotNull(curaFramework
    +                ," curator framework in not initialized ");
    +        Preconditions.checkNotNull(cache," cache value is not initialized");
    +        boolean connected = curaFramework.getZookeeperClient().isConnected();
    +        if (!connected) {
    +            String message = " Zookeeper seems to be down. The data is stale ";
    +            Exception exception =
    +                    new Exception(message);
    +            LOG.error(message, exception);
    +            throw exception;
    +        }
    +        List<String> currentNodes = curaFramework.getChildren().forPath(CONFIG.getParentPath());
    +        List<PhoenixQueryServerNode> returnNodes = new ArrayList<>();
    +        for(String node:currentNodes) {
    +            byte[] bytes = curaFramework.getData().forPath(CONFIG.getParentPath() + "/" + node);
    +            returnNodes.add(PhoenixQueryServerNode.fromJsonString(new String(
    +                    bytes, StandardCharsets.UTF_8)));
    +        }
    +        return returnNodes;
    +    }
    +    private String getZkConnectString(){
    +        return CONFIG.getZkConnectString();
    +    }
    +
    +    private ConnectionStateListener getConnectionStateListener(){
    +        return new ConnectionStateListener() {
    +            @Override
    +            public void stateChanged(CuratorFramework client, ConnectionState newState) {
    +                if (!newState.isConnected()) {
    +                    LOG.error(" connection to zookeeper broken ");
    --- End diff --
    
    done.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: PHOENIX-3654 Load Balancer for thin client

Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:

    https://github.com/apache/phoenix/pull/236


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix issue #236: Loadbalancer

Posted by apurtell <gi...@git.apache.org>.
Github user apurtell commented on the issue:

    https://github.com/apache/phoenix/pull/236
  
    @rahulsIOT Please rename this issue "PHOENIX-3654 Load Balancer for thin client" and please squash all commits and set the commit message to "PHOENIX-3654 Load Balancer for thin client (Rahul Shrivastava)". 
    
    This will enable Apache's GitHub integration to automatically archive comments here on the JIRA. Thanks. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: PHOENIX-3654 Load Balancer for thin client

Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r129066492
  
    --- Diff: phoenix-core/src/main/java/org/apache/phoenix/query/QueryServices.java ---
    @@ -255,9 +255,16 @@
         public static final String DEFAULT_COLUMN_ENCODED_BYTES_ATRRIB  = "phoenix.default.column.encoded.bytes.attrib";
         public static final String DEFAULT_IMMUTABLE_STORAGE_SCHEME_ATTRIB  = "phoenix.default.immutable.storage.scheme";
         public static final String DEFAULT_MULTITENANT_IMMUTABLE_STORAGE_SCHEME_ATTRIB  = "phoenix.default.multitenant.immutable.storage.scheme";
    +
    +    public  final static String PHOENIX_QUERY_SERVER_CLUSTER_BASE_PATH = "phoenix.queryserver.base.path";
    +    public  final static String PHOENIX_QUERY_SERVER_SERVICE_NAME = "phoenix.queryserver.service.name";
    +    public  final static String PHOENIX_QUERY_SERVER_ZK_ACL_USERNAME = "phoenix.queryserver.zookeeper.acl.username";
    +    public  final static String PHOENIX_QUERY_SERVER_ZK_ACL_PASSWORD = "phoenix.queryserver.zookeeper.acl.password";
         public static final String STATS_COLLECTION_ENABLED = "phoenix.stats.collection.enabled";
         public static final String USE_STATS_FOR_PARALLELIZATION = "phoenix.use.stats.parallelization";
     
    +
    +
    --- End diff --
    
    nit: remove please.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix issue #236: Loadbalancer

Posted by apurtell <gi...@git.apache.org>.
Github user apurtell commented on the issue:

    https://github.com/apache/phoenix/pull/236
  
    Also please resolve indicated conflicts. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: Loadbalancer

Posted by rahulsIOT <gi...@git.apache.org>.
Github user rahulsIOT commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r116865790
  
    --- Diff: phoenix-load-balancer/pom.xml ---
    @@ -0,0 +1,58 @@
    +<?xml version='1.0'?>
    +<!--
    +
    + Licensed to the Apache Software Foundation (ASF) under one
    + or more contributor license agreements.  See the NOTICE file
    + distributed with this work for additional information
    + regarding copyright ownership.  The ASF licenses this file
    + to you under the Apache License, Version 2.0 (the
    + "License"); you may not use this file except in compliance
    + with the License.  You may obtain a copy of the License at
    +
    +   http://www.apache.org/licenses/LICENSE-2.0
    +
    + Unless required by applicable law or agreed to in writing,
    + software distributed under the License is distributed on an
    + "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + KIND, either express or implied.  See the License for the
    + specific language governing permissions and limitations
    + under the License.
    +
    +-->
    +
    +<project xmlns="http://maven.apache.org/POM/4.0.0"
    +         xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
    +         xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
    +  <modelVersion>4.0.0</modelVersion>
    +  <parent>
    +    <groupId>org.apache.phoenix</groupId>
    +    <artifactId>phoenix</artifactId>
    +    <version>4.10.0-HBase-1.2-SNAPSHOT</version>
    +  </parent>
    +  <artifactId>phoenix-queryserver-loadbalancer</artifactId>
    +  <name>Phoenix Load Balancer</name>
    +  <description>A Load balancer which routes calls to Phoenix Query Server</description>
    +
    +  <properties>
    +    <top.dir>${project.basedir}/..</top.dir>
    +    <shaded.package>org.apache.phoenix.shaded</shaded.package>
    --- End diff --
    
    done. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: Loadbalancer

Posted by rahulsIOT <gi...@git.apache.org>.
Github user rahulsIOT commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r117437336
  
    --- Diff: phoenix-load-balancer/src/main/java/org/apache/phoenix/loadbalancer/service/LoadBalancer.java ---
    @@ -0,0 +1,165 @@
    +/**
    + *
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.phoenix.loadbalancer.service;
    +
    +import com.google.common.base.Preconditions;
    +import com.google.common.collect.Lists;
    +import org.apache.commons.logging.Log;
    +import org.apache.commons.logging.LogFactory;
    +import org.apache.curator.framework.CuratorFramework;
    +import org.apache.curator.framework.CuratorFrameworkFactory;
    +import org.apache.curator.framework.api.UnhandledErrorListener;
    +import org.apache.curator.framework.recipes.cache.ChildData;
    +import org.apache.curator.framework.recipes.cache.PathChildrenCache;
    +import org.apache.curator.framework.recipes.cache.PathChildrenCacheEvent;
    +import org.apache.curator.framework.recipes.cache.PathChildrenCacheListener;
    +import org.apache.curator.framework.state.ConnectionState;
    +import org.apache.curator.framework.state.ConnectionStateListener;
    +import org.apache.curator.retry.ExponentialBackoffRetry;
    +import org.apache.curator.utils.CloseableUtils;
    +import org.apache.phoenix.loadbalancer.exception.NoPhoenixQueryServerRegisteredException;
    +
    +import java.nio.charset.StandardCharsets;
    +import java.util.ArrayList;
    +import java.util.concurrent.ThreadLocalRandom;
    +import java.io.Closeable;
    +import java.util.List;
    +
    +
    +public class LoadBalancer {
    +
    +    private static final LoadBalancerConfiguration CONFIG = new LoadBalancerConfiguration();
    +    private static CuratorFramework curaFramework = null;
    +    protected static final Log LOG = LogFactory.getLog(LoadBalancer.class);
    +    private static PathChildrenCache   cache = null;
    +    private static final LoadBalancer loadBalancer = new LoadBalancer();
    +
    +    private LoadBalancer()  {
    +        ConnectionStateListener connectionStateListener = null;
    +        UnhandledErrorListener unhandledErrorListener = null;
    +        List<Closeable> closeAbles = Lists.newArrayList();
    +        try  {
    +
    +            curaFramework = CuratorFrameworkFactory.newClient(getZkConnectString(),
    +                    new ExponentialBackoffRetry(1000, 3));
    +            curaFramework.start();
    +            curaFramework.setACL().withACL(CONFIG.getAcls());
    +            connectionStateListener = getConnectionStateListener();
    +            curaFramework.getConnectionStateListenable()
    +                    .addListener(connectionStateListener);
    +            unhandledErrorListener = getUnhandledErrorListener();
    +            curaFramework.getUnhandledErrorListenable()
    +                    .addListener(unhandledErrorListener);
    +            cache = new PathChildrenCache(curaFramework, CONFIG.getParentPath(), true);
    +            cache.start(PathChildrenCache.StartMode.BUILD_INITIAL_CACHE);
    +            closeAbles.add(cache);
    +            closeAbles.add(curaFramework);
    +        }catch(Exception ex){
    +            LOG.error("Exception while creating a zookeeper clients and cache",ex);
    +            if ((curaFramework != null) && (connectionStateListener != null)){
    +                curaFramework.getConnectionStateListenable()
    +                        .removeListener(connectionStateListener);
    +            }
    +            if ((curaFramework != null) && (unhandledErrorListener != null)){
    +                curaFramework.getUnhandledErrorListenable()
    +                        .removeListener(unhandledErrorListener);
    +            }
    +            for (Closeable closeable : closeAbles) {
    +                CloseableUtils.closeQuietly(closeable);
    +            }
    +        }
    +    }
    +
    +    /**
    +     * Return Singleton Load Balancer every single time.
    +     * @return LoadBalancer
    +     */
    +    public static LoadBalancer getLoadBalancer() {
    +        return loadBalancer;
    +    }
    +
    +    /**
    +     * return the location of Phoenix Query Server
    +     * in form of String. The method return a random PQS server
    +     * from the cache.
    +     * @return - PQS Location ( host:port)
    +     * @throws Exception
    +     */
    +    public PhoenixQueryServerNode getSingleServiceLocation()  throws Exception{
    +        List<PhoenixQueryServerNode> childNodes = conductSanityCheckAndReturn();
    +        // get an random connect string
    +        int i = ThreadLocalRandom.current().nextInt(0, childNodes.size());
    +        return childNodes.get(i);
    +    }
    +
    +    /**
    +     * return all the location of Phoenix Query Server from cache
    +     * @return - PQS Locations ( host1:port1,host2:port2)
    +     * @throws Exception
    +     */
    +    public List<PhoenixQueryServerNode> getAllServiceLocation()  throws Exception{
    +        return conductSanityCheckAndReturn();
    +    }
    +
    +    private List<PhoenixQueryServerNode> conductSanityCheckAndReturn() throws Exception{
    +        Preconditions.checkNotNull(curaFramework
    +                ," curator framework in not initialized ");
    +        Preconditions.checkNotNull(cache," cache value is not initialized");
    +        boolean connected = curaFramework.getZookeeperClient().isConnected();
    +        if (!connected) {
    +            String message = " Zookeeper seems to be down. The data is stale ";
    +            Exception exception =
    +                    new Exception(message);
    --- End diff --
    
    Now it throws java.net.ConnectionException


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix issue #236: PHOENIX-3654 Load Balancer for thin client

Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on the issue:

    https://github.com/apache/phoenix/pull/236
  
    Thanks, Rahul. I'll add this to my list to try to merge in so we can keep moving forward!


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: PHOENIX-3654 Load Balancer for thin client

Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r120392963
  
    --- Diff: phoenix-queryserver/src/main/java/org/apache/phoenix/loadbalancer/service/PqsZookeeperConfImpl.java ---
    @@ -0,0 +1,97 @@
    +/**
    + *
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +
    +package org.apache.phoenix.loadbalancer.service;
    +
    +import com.google.common.net.HostAndPort;
    +import org.apache.hadoop.conf.Configuration;
    +import org.apache.hadoop.hbase.HBaseConfiguration;
    +import org.apache.phoenix.query.QueryServices;
    +import org.apache.phoenix.query.QueryServicesOptions;
    +import org.apache.zookeeper.ZooDefs;
    +import org.apache.zookeeper.data.ACL;
    +import org.apache.zookeeper.data.Id;
    +
    +import java.util.Arrays;
    +import java.util.List;
    +
    +
    +
    +public class PqsZookeeperConfImpl implements PqsZookeeperConf {
    +
    +        private final Configuration configuration;
    +
    +        public PqsZookeeperConfImpl() {
    +           this.configuration = HBaseConfiguration.create();
    +        }
    +
    +        public PqsZookeeperConfImpl(Configuration configuration) {
    +            this.configuration = configuration;
    +        }
    +
    +        @Override
    +        public String getQueryServerBasePath(){
    +            return "/"+configuration.get(QueryServices.PHOENIX_QUERY_SERVER_CLUSTER_BASE_PATH,
    --- End diff --
    
    I think pre-pending the leading '/' is confusing. The user can just provide the full path in ZK.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: Loadbalancer

Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r109747132
  
    --- Diff: phoenix-load-balancer/src/main/java/org/apache/phoenix/loadbalancer/service/PhoenixQueryServerNode.java ---
    @@ -0,0 +1,79 @@
    +/**
    + *
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.phoenix.loadbalancer.service;
    +
    +import org.codehaus.jackson.annotate.JsonProperty;
    +import org.codehaus.jackson.map.ObjectMapper;
    +import org.codehaus.jackson.map.annotate.JsonRootName;
    +
    +import java.io.IOException;
    +import java.nio.charset.StandardCharsets;
    +
    +/**
    + * Immutable class for defining the server location for
    + * Phoenix query instance. This data is stored as Node data
    + * in zookeeper
    + */
    +public class PhoenixQueryServerNode {
    +
    +    public void setHost(String host) {
    +        this.host = host;
    +    }
    +
    +    public void setPort(String port) {
    +        this.port = port;
    +    }
    +
    +    private String host;
    +    private String port;
    --- End diff --
    
    Guava has a nice HostAndPort class which covers a lot of edge cases. That would be good to use internally here.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: PHOENIX-3654 Load Balancer for thin client

Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r120394548
  
    --- Diff: phoenix-queryserver/src/main/java/org/apache/phoenix/queryserver/server/QueryServer.java ---
    @@ -233,16 +240,29 @@ public int run(String[] args) throws Exception {
           // Build and start the HttpServer
           server = builder.build();
           server.start();
    +      registerToServiceProvider(hostname);
           runningLatch.countDown();
           server.join();
           return 0;
         } catch (Throwable t) {
           LOG.fatal("Unrecoverable service error. Shutting down.", t);
           this.t = t;
           return -1;
    +    } finally {
    +      deRegister();
         }
       }
     
    +  private void registerToServiceProvider(String hostName) throws Exception {
    +      PqsZookeeperConf pqsZookeeperConf = new PqsZookeeperConfImpl(getConf());
    --- End diff --
    
    Oh, I see now. You're instantiating these implementations directly.
    
    I think you should use Java's ServiceLoader to properly separate properly separate interface from implementation across the queryserver and loadbalancer modules. Here in queryserver, you would just refer to PqsZookeeperConf and Registry (but the concrete implementations would be wired up by ServiceLoader as defined by the META-INF/services/... files you create). 
    
    If you're not familiar: https://docs.oracle.com/javase/tutorial/ext/basics/spi.html. Should be pretty straightforward. e.g. this is also how the PhoenixDriver class is loaded for JDBC (so there's example of what to do in phoenix-core).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: Loadbalancer

Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r109750030
  
    --- Diff: phoenix-queryserver/src/main/java/org/apache/phoenix/queryserver/register/ZookeeperRegistry.java ---
    @@ -0,0 +1,73 @@
    +/**
    + *
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.phoenix.queryserver.register;
    +
    +
    +import org.apache.commons.logging.Log;
    +import org.apache.commons.logging.LogFactory;
    +import org.apache.curator.framework.CuratorFramework;
    +import org.apache.curator.framework.CuratorFrameworkFactory;
    +import org.apache.curator.retry.ExponentialBackoffRetry;
    +import org.apache.curator.utils.CloseableUtils;
    +import org.apache.phoenix.loadbalancer.service.LoadBalancerConfiguration;
    +import org.apache.phoenix.loadbalancer.service.PhoenixQueryServerNode;
    +import org.apache.phoenix.queryserver.server.QueryServer;
    +import org.apache.zookeeper.CreateMode;
    +import org.apache.zookeeper.data.Stat;
    +import java.net.InetAddress;
    +import java.nio.charset.StandardCharsets;
    +
    +
    +public class ZookeeperRegistry implements Registry {
    +
    +    protected static final Log LOG = LogFactory.getLog(QueryServer.class);
    +    private CuratorFramework client;
    +
    +    public ZookeeperRegistry(){}
    +
    +    @Override
    +    public  void registerServer(LoadBalancerConfiguration loadBalancerConfiguration, int avaticaServerPort,
    +                                String zookeeperConnectString)
    +            throws Exception {
    +
    +        this.client = CuratorFrameworkFactory.newClient(zookeeperConnectString,
    +                new ExponentialBackoffRetry(1000,10));
    +        this.client.start();
    +        String host = InetAddress.getLocalHost().getHostName();
    --- End diff --
    
    I think it makes sense to have the caller pass this info in. Or, provide some information object on the `QueryServer` instance.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: PHOENIX-3654 Load Balancer for thin client

Posted by rahulsIOT <gi...@git.apache.org>.
Github user rahulsIOT commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r122048729
  
    --- Diff: phoenix-queryserver/src/main/java/org/apache/phoenix/loadbalancer/service/PqsZookeeperConfImpl.java ---
    @@ -0,0 +1,97 @@
    +/**
    + *
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +
    +package org.apache.phoenix.loadbalancer.service;
    +
    +import com.google.common.net.HostAndPort;
    +import org.apache.hadoop.conf.Configuration;
    +import org.apache.hadoop.hbase.HBaseConfiguration;
    +import org.apache.phoenix.query.QueryServices;
    +import org.apache.phoenix.query.QueryServicesOptions;
    +import org.apache.zookeeper.ZooDefs;
    +import org.apache.zookeeper.data.ACL;
    +import org.apache.zookeeper.data.Id;
    +
    +import java.util.Arrays;
    +import java.util.List;
    +
    +
    +
    +public class PqsZookeeperConfImpl implements PqsZookeeperConf {
    --- End diff --
    
    nevermind, saw your comment below. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: PHOENIX-3654 Load Balancer for thin client

Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r122508226
  
    --- Diff: phoenix-queryserver/src/main/java/org/apache/phoenix/queryserver/server/QueryServer.java ---
    @@ -233,16 +240,29 @@ public int run(String[] args) throws Exception {
           // Build and start the HttpServer
           server = builder.build();
           server.start();
    +      registerToServiceProvider(hostname);
           runningLatch.countDown();
           server.join();
           return 0;
         } catch (Throwable t) {
           LOG.fatal("Unrecoverable service error. Shutting down.", t);
           this.t = t;
           return -1;
    +    } finally {
    +      deRegister();
         }
       }
     
    +  private void registerToServiceProvider(String hostName) throws Exception {
    +      PqsZookeeperConf pqsZookeeperConf = new PqsZookeeperConfImpl(getConf());
    --- End diff --
    
    Move any IT exercising loadbalancer functionality to the loadbalancer module :)
    
    Tests in the queryserver module would have no load balancer implementation. PQS should pass gracefully in this case (e.g. not try to register with the loadbalancer because it would be null).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: PHOENIX-3654 Load Balancer for thin client

Posted by rahulsIOT <gi...@git.apache.org>.
Github user rahulsIOT commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r130725729
  
    --- Diff: phoenix-core/src/main/java/org/apache/phoenix/query/QueryServicesOptions.java ---
    @@ -284,14 +288,18 @@
         public static final int DEFAULT_COLUMN_ENCODED_BYTES = QualifierEncodingScheme.TWO_BYTE_QUALIFIERS.getSerializedMetadataValue();
         public static final String DEFAULT_IMMUTABLE_STORAGE_SCHEME = ImmutableStorageScheme.SINGLE_CELL_ARRAY_WITH_OFFSETS.toString();
         public static final String DEFAULT_MULTITENANT_IMMUTABLE_STORAGE_SCHEME = ImmutableStorageScheme.ONE_CELL_PER_COLUMN.toString();
    +    public  final static String DEFAULT_PHOENIX_QUERY_SERVER_CLUSTER_BASE_PATH = "/phoenix";
    +    public  final static String DEFAULT_PHOENIX_QUERY_SERVER_SERVICE_NAME = "queryserver";
    +    public  final static String DEFAULT_PHOENIX_QUERY_SERVER_ZK_ACL_USERNAME = "phoenixuser";
    +    public  final static String DEFAULT_PHOENIX_QUERY_SERVER_ZK_ACL_PASSWORD = "Xsjdhxsd";
    --- End diff --
    
    I think you mentioned SASL ACLs before,and I replied saying that we can close this PR and open another ticket, where I will fix the SASL ACLs. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: Loadbalancer

Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r109750317
  
    --- Diff: pom.xml ---
    @@ -953,6 +962,30 @@
             <artifactId>joda-time</artifactId>
             <version>${jodatime.version}</version>
           </dependency>
    +      <dependency>
    +        <groupId>org.apache.curator</groupId>
    +        <artifactId>curator-framework</artifactId>
    +        <version>${curator.version}</version>
    +      </dependency>
    +      <dependency>
    +        <groupId>org.apache.curator</groupId>
    +        <artifactId>curator-x-discovery</artifactId>
    +        <version>${curator.version}</version>
    +      </dependency>
    +      <dependency>
    +        <groupId>org.apache.curator</groupId>
    +        <artifactId>curator-test</artifactId>
    +        <version>${curator.version}</version>
    +      </dependency>
    +      <dependency>
    +        <groupId>org.apache.phoenix</groupId>
    +        <artifactId>phoenix-core</artifactId>
    --- End diff --
    
    Unnecessary to include phoenix-core here..


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: Loadbalancer

Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r109746794
  
    --- Diff: phoenix-load-balancer/pom.xml ---
    @@ -0,0 +1,58 @@
    +<?xml version='1.0'?>
    +<!--
    +
    + Licensed to the Apache Software Foundation (ASF) under one
    + or more contributor license agreements.  See the NOTICE file
    + distributed with this work for additional information
    + regarding copyright ownership.  The ASF licenses this file
    + to you under the Apache License, Version 2.0 (the
    + "License"); you may not use this file except in compliance
    + with the License.  You may obtain a copy of the License at
    +
    +   http://www.apache.org/licenses/LICENSE-2.0
    +
    + Unless required by applicable law or agreed to in writing,
    + software distributed under the License is distributed on an
    + "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + KIND, either express or implied.  See the License for the
    + specific language governing permissions and limitations
    + under the License.
    +
    +-->
    +
    +<project xmlns="http://maven.apache.org/POM/4.0.0"
    +         xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
    +         xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
    +  <modelVersion>4.0.0</modelVersion>
    +  <parent>
    +    <groupId>org.apache.phoenix</groupId>
    +    <artifactId>phoenix</artifactId>
    +    <version>4.10.0-HBase-1.2-SNAPSHOT</version>
    +  </parent>
    +  <artifactId>phoenix-queryserver-loadbalancer</artifactId>
    +  <name>Phoenix Load Balancer</name>
    +  <description>A Load balancer which routes calls to Phoenix Query Server</description>
    +
    +  <properties>
    +    <top.dir>${project.basedir}/..</top.dir>
    +    <shaded.package>org.apache.phoenix.shaded</shaded.package>
    --- End diff --
    
    Unused


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: PHOENIX-3654 Load Balancer for thin client

Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r123116460
  
    --- Diff: phoenix-load-balancer/src/main/java/org/apache/phoenix/loadbalancer/service/LoadBalancer.java ---
    @@ -0,0 +1,170 @@
    +/**
    + *
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.phoenix.loadbalancer.service;
    +
    +import com.google.common.base.Preconditions;
    +import com.google.common.collect.Lists;
    +import com.google.common.net.HostAndPort;
    +import org.apache.commons.logging.Log;
    +import org.apache.commons.logging.LogFactory;
    +import org.apache.curator.framework.CuratorFramework;
    +import org.apache.curator.framework.CuratorFrameworkFactory;
    +import org.apache.curator.framework.api.UnhandledErrorListener;
    +import org.apache.curator.framework.recipes.cache.PathChildrenCache;
    +import org.apache.curator.framework.state.ConnectionState;
    +import org.apache.curator.framework.state.ConnectionStateListener;
    +import org.apache.curator.retry.ExponentialBackoffRetry;
    +import org.apache.curator.utils.CloseableUtils;
    +import org.apache.hadoop.hbase.HBaseConfiguration;
    +
    +import java.net.ConnectException;
    +import java.util.ArrayList;
    +import java.util.concurrent.ThreadLocalRandom;
    +import java.io.Closeable;
    +import java.util.List;
    +
    +
    +public class LoadBalancer {
    +
    +    private static final LoadBalanceZookeeperConf CONFIG = new LoadBalanceZookeeperConfImpl(HBaseConfiguration.create());
    +    private static CuratorFramework curaFramework = null;
    +    protected static final Log LOG = LogFactory.getLog(LoadBalancer.class);
    +    private static PathChildrenCache   cache = null;
    +    private static final LoadBalancer loadBalancer = new LoadBalancer();
    +    private ConnectionStateListener connectionStateListener = null;
    +    private UnhandledErrorListener unhandledErrorListener = null;
    +    private List<Closeable> closeAbles = Lists.newArrayList();
    +
    +    private LoadBalancer()  {
    +        try  {
    +            start();
    +        }catch(Exception ex){
    +            LOG.error("Exception while creating a zookeeper clients and cache",ex);
    +            if ((curaFramework != null) && (connectionStateListener != null)){
    +                curaFramework.getConnectionStateListenable()
    +                        .removeListener(connectionStateListener);
    +            }
    +            if ((curaFramework != null) && (unhandledErrorListener != null)){
    +                curaFramework.getUnhandledErrorListenable()
    +                        .removeListener(unhandledErrorListener);
    +            }
    +            for (Closeable closeable : closeAbles) {
    +                CloseableUtils.closeQuietly(closeable);
    +            }
    +        }
    +    }
    +
    +
    +
    +    /**
    +     * Return Singleton Load Balancer every single time.
    +     * @return LoadBalancer
    +     */
    +    public static LoadBalancer getLoadBalancer() {
    +        return loadBalancer;
    +    }
    +
    +    /**
    +     * return the location of Phoenix Query Server
    --- End diff --
    
    Javadoc could use some updating to reflect the use of HostAndPort and alert users that they will see an exception if there are no registered PQS instances.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: PHOENIX-3654 Load Balancer for thin client

Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r129067998
  
    --- Diff: phoenix-core/src/main/java/org/apache/phoenix/query/QueryServices.java ---
    @@ -255,9 +255,16 @@
         public static final String DEFAULT_COLUMN_ENCODED_BYTES_ATRRIB  = "phoenix.default.column.encoded.bytes.attrib";
         public static final String DEFAULT_IMMUTABLE_STORAGE_SCHEME_ATTRIB  = "phoenix.default.immutable.storage.scheme";
         public static final String DEFAULT_MULTITENANT_IMMUTABLE_STORAGE_SCHEME_ATTRIB  = "phoenix.default.multitenant.immutable.storage.scheme";
    +
    +    public  final static String PHOENIX_QUERY_SERVER_CLUSTER_BASE_PATH = "phoenix.queryserver.base.path";
    +    public  final static String PHOENIX_QUERY_SERVER_SERVICE_NAME = "phoenix.queryserver.service.name";
    +    public  final static String PHOENIX_QUERY_SERVER_ZK_ACL_USERNAME = "phoenix.queryserver.zookeeper.acl.username";
    +    public  final static String PHOENIX_QUERY_SERVER_ZK_ACL_PASSWORD = "phoenix.queryserver.zookeeper.acl.password";
    --- End diff --
    
    Nit: please match the style "public static final" not "public  final static" (emphasis also on the two spaces)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: PHOENIX-3654 Load Balancer for thin client

Posted by rahulsIOT <gi...@git.apache.org>.
Github user rahulsIOT commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r122505939
  
    --- Diff: phoenix-queryserver/src/main/java/org/apache/phoenix/queryserver/server/QueryServer.java ---
    @@ -233,16 +240,29 @@ public int run(String[] args) throws Exception {
           // Build and start the HttpServer
           server = builder.build();
           server.start();
    +      registerToServiceProvider(hostname);
           runningLatch.countDown();
           server.join();
           return 0;
         } catch (Throwable t) {
           LOG.fatal("Unrecoverable service error. Shutting down.", t);
           this.t = t;
           return -1;
    +    } finally {
    +      deRegister();
         }
       }
     
    +  private void registerToServiceProvider(String hostName) throws Exception {
    +      PqsZookeeperConf pqsZookeeperConf = new PqsZookeeperConfImpl(getConf());
    --- End diff --
    
    Question: How to make the IT test pass for Queryserver? In the IT tests, the queryserver loads up and registers itself to a ZKTestingServer . This uses curator client which is part of Registry class implementation. Now, this implementation is part of loadbalancer module. One way is to provide a mocked out implementation of Registry and LoadBalancerConf in the queryserver module. Do you think something else from your experience ?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: Loadbalancer

Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r117391613
  
    --- Diff: phoenix-load-balancer/src/main/java/org/apache/phoenix/loadbalancer/service/PhoenixQueryServerNode.java ---
    @@ -0,0 +1,79 @@
    +/**
    + *
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.phoenix.loadbalancer.service;
    +
    +import org.codehaus.jackson.annotate.JsonProperty;
    +import org.codehaus.jackson.map.ObjectMapper;
    +import org.codehaus.jackson.map.annotate.JsonRootName;
    +
    +import java.io.IOException;
    +import java.nio.charset.StandardCharsets;
    +
    +/**
    + * Immutable class for defining the server location for
    + * Phoenix query instance. This data is stored as Node data
    + * in zookeeper
    + */
    +public class PhoenixQueryServerNode {
    +
    +    public void setHost(String host) {
    +        this.host = host;
    +    }
    +
    +    public void setPort(String port) {
    +        this.port = port;
    +    }
    +
    +    private String host;
    +    private String port;
    --- End diff --
    
    Yeah, you can still collapse the two string fields into one HostAndPort..


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #236: Loadbalancer

Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/236#discussion_r117372190
  
    --- Diff: phoenix-load-balancer/src/main/java/org/apache/phoenix/loadbalancer/service/LoadBalancerConfiguration.java ---
    @@ -0,0 +1,91 @@
    +/**
    + *
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.phoenix.loadbalancer.service;
    +
    +
    +
    +import org.apache.hadoop.conf.Configuration;
    +import org.apache.hadoop.hbase.HBaseConfiguration;
    +import org.apache.zookeeper.ZooDefs;
    +import org.apache.zookeeper.data.ACL;
    +import org.apache.zookeeper.data.Id;
    +import java.util.Arrays;
    +import java.util.List;
    +
    +/**
    + * Basic Configuration specific to Load Balancer only.
    + * The instance of {@link LoadBalancerConfiguration} is
    + * used as singleton so that we do not instantiate the class
    + * many times.
    + */
    +public class LoadBalancerConfiguration {
    +
    +    public  final static String clusterName = "phoenix.queryserver.base.path";
    +    public  final static String serviceName = "phoenix.queryserver.service.name";
    +    public  final static String defaultClusterName = "phoenix";
    +    public  final static String defaultServiceName = "queryserver";
    +    public  static String zkQuorum = "hbase.zookeeper.quorum";
    +    public  static String zkPort = "hbase.zookeeper.property.clientPort";
    +    public  static String zkLbUserName = "phoenix.queryserver.zookeeper.acl.username";
    +    public  static String zkLbPassword = "phoenix.queryserver.zookeeper.acl.password";
    +    public  static String defaultZkLbUserName = "phoenixuser";
    +    public  static String defaultZkLbPassword = "Xsjdhxsd";
    +    private static final Configuration configuration = HBaseConfiguration.create();
    +
    +    public LoadBalancerConfiguration() {}
    +
    +    public String getQueryServerBasePath(){
    +        return "/"+configuration.get(clusterName, defaultClusterName);
    +    }
    +
    +    public String getServiceName(){
    +        return configuration.get(serviceName, defaultServiceName);
    +    }
    +
    +    public String getZkConnectString(){
    +        return String.format("%s:%s",configuration.get(zkQuorum,"localhost"),configuration.get(zkPort,"2183"));
    +    }
    +
    +    private String getZkLbUserName(){
    +        return configuration.get(zkLbUserName,defaultZkLbUserName);
    +    }
    +
    +    private String getZkLbPassword(){
    +        return configuration.get(zkLbPassword,defaultZkLbPassword);
    +    }
    +
    +    public List<ACL> getAcls() {
    +        ACL acl = new ACL();
    +        acl.setId(new Id("digest",getZkLbUserName()+":"+getZkLbPassword()));
    --- End diff --
    
    Sure. Will just need to make sure documentation is clear if this feature gets out in a release before you get around to the sasl acl support.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---