[GitHub] [hive] nrg4878 commented on a diff in pull request #3388: HIVE-26045: Detect timed out connections for providers and auto-reconnect

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

nrg4878 commented on code in PR #3388:
URL: https://github.com/apache/hive/pull/3388#discussion_r957956349


##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/dataconnector/jdbc/AbstractJDBCConnectorProvider.java:
##########
@@ -129,8 +129,20 @@ protected Connection getConnection() {
     throw new RuntimeException("unexpected type for connection handle");
   }
 
+  protected boolean isOpen() {
+    if (handle == null) {
+      return false;
+    }
+    try {
+      return ((Connection)handle).isClosed();
+    } catch (SQLException e) {
+      LOG.warn("Could not validate jdbc connection to"+ jdbcUrl, e);
+    }
+    return false;
+  }
+
   @Override public void close() {
-    if (isOpen) {

Review Comment:
   looks like this boolean is never being used.



##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/dataconnector/jdbc/AbstractJDBCConnectorProvider.java:
##########
@@ -129,8 +129,20 @@ protected Connection getConnection() {
     throw new RuntimeException("unexpected type for connection handle");
   }
 
+  protected boolean isOpen() {
+    if (handle == null) {
+      return false;
+    }
+    try {
+      return ((Connection)handle).isClosed();
+    } catch (SQLException e) {
+      LOG.warn("Could not validate jdbc connection to"+ jdbcUrl, e);

Review Comment:
   nit: need a space after "to" and before the jdbcUrl
   so "to " + jdbcUrl



##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/dataconnector/jdbc/AbstractJDBCConnectorProvider.java:
##########
@@ -107,8 +108,7 @@ public AbstractJDBCConnectorProvider(String dbName, DataConnector dataConn, Stri
 
   @Override public void open() throws ConnectException {
     try {
-      handle = DriverManager.getConnection(jdbcUrl, username, password);
-      isOpen = true;
+      handle = DriverManager.getDriver(jdbcUrl).connect(jdbcUrl, getConnectionProperties());

Review Comment:
   /whats the difference between DriverManager.getConnection vs DriverManager.getDriver().connect() 



##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/dataconnector/jdbc/MySQLConnectorProvider.java:
##########
@@ -27,14 +27,27 @@
 import java.sql.ResultSet;
 import java.sql.SQLException;
 import java.sql.Statement;
+import java.util.Properties;
 
 public class MySQLConnectorProvider extends AbstractJDBCConnectorProvider {
   private static Logger LOG = LoggerFactory.getLogger(MySQLConnectorProvider.class);
 
   private static final String DRIVER_CLASS = "com.mysql.jdbc.Driver";
 
+  public static final String JDBC_MYSQL_CONFIG_PREFIX = "hive.sql.mysql";
+  public static final String JDBC_MYSQL_AUTO_RECONNECT = JDBC_MYSQL_CONFIG_PREFIX + ".auto.reconnect";

Review Comment:
   what are these for? is this only supported for MySQL ?



##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/dataconnector/jdbc/AbstractJDBCConnectorProvider.java:
##########
@@ -129,8 +129,20 @@ protected Connection getConnection() {
     throw new RuntimeException("unexpected type for connection handle");
   }
 
+  protected boolean isOpen() {
+    if (handle == null) {
+      return false;
+    }
+    try {
+      return ((Connection)handle).isClosed();

Review Comment:
   also isValid() might be better than isClosed(). I dont know if isClosed() returns true if close() has not been called on the connection. isValid() seems to test the health of the connection



##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/dataconnector/jdbc/AbstractJDBCConnectorProvider.java:
##########
@@ -129,8 +129,20 @@ protected Connection getConnection() {
     throw new RuntimeException("unexpected type for connection handle");
   }
 
+  protected boolean isOpen() {
+    if (handle == null) {
+      return false;
+    }
+    try {
+      return ((Connection)handle).isClosed();

Review Comment:
   while it should never happen but its always a good idea to check the type via instanceof before casting it to avoid exceptions. So may be add a check?



-- 
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: gitbox-unsubscribe@hive.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org