[GitHub] [phoenix] dbwong commented on a change in pull request #1280: PHOENIX-6523: Support for HRpc connection protocol through JDBC URL

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

dbwong commented on a change in pull request #1280:
URL: https://github.com/apache/phoenix/pull/1280#discussion_r685659569



##########
File path: phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriver.java
##########
@@ -44,14 +30,22 @@
 import org.apache.phoenix.query.ConnectionQueryServices;
 import org.apache.phoenix.query.HBaseFactoryProvider;
 import org.apache.phoenix.query.QueryServices;
+import org.apache.phoenix.thirdparty.com.google.common.collect.ImmutableMap;
+import org.apache.phoenix.thirdparty.com.google.common.collect.Maps;
 import org.apache.phoenix.util.PhoenixRuntime;
 import org.apache.phoenix.util.PropertiesUtil;
 import org.apache.phoenix.util.ReadOnlyProps;
 import org.apache.phoenix.util.SQLCloseable;
 import org.slf4j.LoggerFactory;
 
-import org.apache.phoenix.thirdparty.com.google.common.collect.ImmutableMap;
-import org.apache.phoenix.thirdparty.com.google.common.collect.Maps;
+import javax.annotation.concurrent.Immutable;
+import java.io.IOException;
+import java.sql.*;

Review comment:
       We do not use import * in phoenix code guidelines, you can use our eclipse template or similar.

##########
File path: phoenix-core/src/test/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriverTest.java
##########
@@ -18,21 +18,20 @@
 
 package org.apache.phoenix.jdbc;
 
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertTrue;
-
-import java.sql.Driver;
-import java.sql.DriverManager;
-import java.sql.SQLException;
-
 import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.hbase.HConstants;
 import org.apache.phoenix.exception.SQLExceptionCode;
 import org.apache.phoenix.jdbc.PhoenixEmbeddedDriver.ConnectionInfo;
 import org.apache.phoenix.query.HBaseFactoryProvider;
+import org.apache.phoenix.util.ReadOnlyProps;
 import org.junit.Test;
 
+import java.sql.Driver;
+import java.sql.DriverManager;
+import java.sql.SQLException;
+
+import static org.junit.Assert.*;

Review comment:
       * import again

##########
File path: phoenix-core/src/main/java/org/apache/phoenix/query/QueryServices.java
##########
@@ -56,6 +56,9 @@
             "phoenix.query.server.orderBy.spooling.enabled";
     public static final String HBASE_CLIENT_KEYTAB = "hbase.myclient.keytab";
     public static final String HBASE_CLIENT_PRINCIPAL = "hbase.myclient.principal";
+
+    public static final String HBASE_MASTERS = "hbase.masters";

Review comment:
       Is this defined already in a publicly facing hbase module?  If so can we directly include it via hbase compat or similar?

##########
File path: phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriver.java
##########
@@ -481,16 +514,20 @@ private Configuration getConfiguration(ReadOnlyProps props, Properties info, Str
             }
             return config;
         }
-        
+
         private final Integer port;
         private final String rootNode;
         private final String zookeeperQuorum;
         private final boolean isConnectionless;
         private final String principal;
         private final String keytab;
         private final User user;
-        
+
         public ConnectionInfo(String zookeeperQuorum, Integer port, String rootNode, String principal, String keytab) {
+            this(zookeeperQuorum, port, rootNode, principal, keytab, null);
+        }
+
+        public ConnectionInfo(String zookeeperQuorum, Integer port, String rootNode, String principal, String keytab, String bootstrap) {

Review comment:
       Can we consider a parsed Enum or was there a reason you used a String for bootstrap?




-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org

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