HIVE-26323: Expose real exception information to the client in JdbcSerDe.java

[zhangbutao]

What changes were proposed in this pull request?

Throw real exception massage to client in method initialize of JdbcSerDe.java

Why are the changes needed?

Display friendly execption massage to the client user.
Please see the details in: https://issues.apache.org/jira/browse/HIVE-26323

Does this PR introduce any user-facing change?

No.

How was this patch tested?

mvn test -Dtest=TestNegativeLlapLocalCliDriver -Dqfile=jdbc_table_create_with_wrong_password.q -Dtest.output.overwrite=true
mvn test -Dtest=TestNegativeLlapLocalCliDriver -Dqfile=jdbc_table_create_with_wrong_url.q -Dtest.output.overwrite=true

[zhangbutao]

@zabetak @belugabehr Would you like to have a look? Thanks in advance.

[zabetak]

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.

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?

[zhangbutao]

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

hive/jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/JdbcSerDe.java

Lines 133 to 134 in a63f4b3

	} catch (Exception e) {
	throw new SerDeException("Caught exception while initializing the SqlSerDe", e);

hive/service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java

Lines 245 to 246 in a63f4b3

	if (e instanceof CommandProcessorException) {
	throw toSQLException("Error while compiling statement", (CommandProcessorException)e);

hive/service/src/java/org/apache/hive/service/cli/operation/Operation.java

Lines 365 to 368 in a63f4b3

	protected HiveSQLException toSQLException(String prefix, CommandProcessorException e) {
	HiveSQLException ex =
	new HiveSQLException(prefix + ": " + e.getMessage(), e.getSqlState(), e.getResponseCode());
	if (e.getCause() != null) {

[zabetak]

The method toSQLException indeed uses the message from CommandProcessorException so in this case "Caught exception while initializing the SqlSerDe" but it also restores the cause which contains the original message. Usually when an exception is printed the stack trace contains all causes along with their messages so that's why I am not still sure why we lose the original message.

[zhangbutao]

Actually, we can get all exception stack trace which contains all causes along with their messages from hiveserver2 log, and we can find partial real exception message from hiveserver2 log.
But client user(beeline) can not get real exception message, because CommandProcessorException only return message "Caught exception while initializing the SqlSerDe"

[zabetak]

It seems that this is a problem of the client (Beeline) and not a problem of the server (HS2).

A stacktrace can be composed of many nested exceptions one causing the other. It doesn't seem like a very good idea to me to just concatenate the messages when propagating the exception from one abstraction to the other.

throw new SerDeException("Caught exception while initializing the SqlSerDe: " + e.getMessage(), e);

The snippet above will make the exception in the HS2 logs more verbose than necessary since it will essentially have duplicate information.

Even the current version of the code contains duplicate information:

throw new SerDeException("Caught exception while initializing the SqlSerDe", e);

The stacktrace shows that this exception is thrown from inside a catch block so mentioning "Caught exception while" does not add additional info but just states the obvious. Moreover, the stack trace also shows that we are inside the initialize method, in JdbcSerDe class so putting "initializing the SqlSerDe" is redundant and possibly misleading cause there is no SqlSerDe class.

What I want to highlight is that exceptions are for developers and not for end users. It is very important to provide to the end user meaningful error messages but we should be mindful on how we achieve this result.

[zabetak]

Would it be possible to write unit tests using derby which are more lightweight than those starting up docker containers?

[zhangbutao]

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.

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Feel free to reach out on the dev@hive.apache.org list if the patch is in need of reviews.