zhangbutao commented on code in PR #3364:
URL: https://github.com/apache/hive/pull/3364#discussion_r897501149
##########
jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/JdbcSerDe.java:
##########
@@ -131,7 +131,8 @@ public void initialize(Configuration configuration,
Properties tableProperties,
row = new ArrayList<>(hiveColumnNames.length);
}
} catch (Exception e) {
- throw new SerDeException("Caught exception while initializing the
SqlSerDe", e);
+ log.error("Caught exception while initializing the SqlSerDe", e);
+ throw new SerDeException(e);
Review Comment:
> Log and rethrow is usually an anti pattern and most of the time it will
lead into reporting the same problem multiple times.
>
> Also it is usually a good thing to wrap and rethrow an exception with a
message appropriate for the current abstraction.
Make sense. How about this change?
`throw new SerDeException("Caught exception while initializing the SqlSerDe:
" + e.getMessage(), e);`
> I am not sure that the code here is responsible for "losing" the real
exception. Have you checked higher or lower in the call hierarchy to see if
there is something wrong going on there?
I'm sure that's the root of the problem.
Method _**runQuery()**_ in SQLOperation.java will catch final exception and
rethrow _**toSQLException**_ which show the root exception message to the
client, and the exception messgae is retrieved from original exception in
JdbcSerDe using _**e.getMessage()**_. howerver, current JdbcSerDe code always
return exception message "**Caught exception while initializing the SqlSerDe**".
https://github.com/apache/hive/blob/a63f4b38c4ab9baf5d99f233fd3b49e8e52fb6c6/jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/JdbcSerDe.java#L133-L134
https://github.com/apache/hive/blob/a63f4b38c4ab9baf5d99f233fd3b49e8e52fb6c6/service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java#L245-L246
https://github.com/apache/hive/blob/a63f4b38c4ab9baf5d99f233fd3b49e8e52fb6c6/service/src/java/org/apache/hive/service/cli/operation/Operation.java#L365-L368
##########
ql/src/test/queries/clientnegative/jdbc_table_create_with_wrong_password.q:
##########
@@ -0,0 +1,12 @@
+--! qt:database:mariadb:q_test_country_table_with_schema.mariadb.sql
+
+-- Create jdbc table with wrong password(hive.sql.dbcp.password)
+CREATE EXTERNAL TABLE country_test1 (id int, name varchar(20))
Review Comment:
Good suggest. We already have the derby qtest which can verify this change,
**external_jdbc_negative.q**. But this test has been disabled as its flaky. Do
you think we need reopen this test? thanks.
--
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]
