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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
