[GitHub] [phoenix-queryserver] stoty commented on a change in pull request #38: PHOENIX-5771 Make standalone queryserver assembly useful again

[GitBox <gi...@apache.org>]

stoty commented on a change in pull request #38:
URL: https://github.com/apache/phoenix-queryserver/pull/38#discussion_r441320101



##########
File path: bin/queryserver.py
##########
@@ -121,12 +121,10 @@
 
 # The command is run through subprocess so environment variables are automatically inherited
 java_cmd = '%(java)s -cp ' + hbase_conf_dir + os.pathsep + hadoop_conf_dir + os.pathsep + \
-    phoenix_utils.phoenix_client_jar + os.pathsep + \
-    phoenix_utils.phoenix_loadbalancer_jar + os.pathsep + \
-    phoenix_utils.phoenix_queryserver_jar + os.pathsep + \
-    phoenix_utils.phoenix_queryserver_classpath + \
+    phoenix_queryserver_utils.phoenix_client_jar + os.pathsep + \
+    phoenix_queryserver_utils.phoenix_queryserver_jar + os.pathsep + \

Review comment:
       The lib directory used to contain guava, which is now shaded in, so it's not needed.
   It also contains sqlline, which is used by sqlline-thin, but not by queryserver.
   
   As for the load-balancer, you are right that this setup doesn't allow for a working load-balancer. However, it wouldn't work even if the load-balancer jar was included in the classpath. The load-balancer depends on (unshaded) zookeeper, and unshaded zookeeper is generally not available on the PQS classpath anyway (it may have been available in some old phoenix-client version, but currently both master and 4.x shades it).
   
   Loading the load-balancer as an external service is nice in theory, but since it's compiled separately from phoenix-client, the dependency conflicts that it introduces are hard to solve. 
   
   If I were to do it, I'd:
   
   - Refactor/Simplify loadbalancer, so that doesn't need Hadoop and Hbase deps
     * Pass the Hbase Config object as in Iterable to registerServer
     * Ditch the config singleton/cache, it not useful in this context
     * Preferably ditch Curator as well
   - create a shaded loadbalancer jar with ZK (and curator) shaded into it
   - Then prepare the startup script for it, and document how to add this to PQS as an optional dependency
   




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org