zabetak commented on code in PR #3938:
URL: https://github.com/apache/hive/pull/3938#discussion_r1083931923
##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/RetryingMetaStoreClient.java:
##########
@@ -264,15 +264,18 @@ public Object run() throws MetaException {
return ret;
}
- private static boolean isRecoverableMetaException(MetaException e) {
- String m = e.getMessage();
- if (m == null) {
+ public static boolean isRecoverableMessage(String exceptionMsg) {
Review Comment:
Calling this method from the Server(Handler) adds additional coupling
between Client and Server thus it is debatable if the change has a net positive
outcome. I slightly prefer the implementation as it is right now; I find it a
bit more straightforward to understand/document.
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/RetryingHMSHandler.java:
##########
@@ -203,11 +203,12 @@ public Result invokeInternal(final Object proxy, final
Method method, final Obje
}
}
- if (retryCount >= retryLimit) {
+ Throwable rootCause = ExceptionUtils.getRootCause(caughtException);
+ String errorMessage = ExceptionUtils.getMessage(caughtException) +
+ (rootCause == null ? "" : ("\nRoot cause: " + rootCause));
+ if (!RetryingMetaStoreClient.isRecoverableMessage(errorMessage) ||
retryCount >= retryLimit) {
LOG.error("HMSHandler Fatal error: " +
ExceptionUtils.getStackTrace(caughtException));
- MetaException me = new MetaException(caughtException.toString());
- me.initCause(caughtException);
Review Comment:
Removing the call to `initCause` destroys the exception provenance that is
usually very helpful to get the full picture that led to an exception. Is there
a particular reason that stack-traces till here are not worth
keeping/propagating?
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/RetryingHMSHandler.java:
##########
@@ -203,11 +203,12 @@ public Result invokeInternal(final Object proxy, final
Method method, final Obje
}
}
- if (retryCount >= retryLimit) {
+ Throwable rootCause = ExceptionUtils.getRootCause(caughtException);
+ String errorMessage = ExceptionUtils.getMessage(caughtException) +
Review Comment:
Concatenating exception messages is a pattern that I tend to avoid. I find
the stacktraces using this pattern hard to read and analyse since it leads to
message duplication and repetition.
From a developer standpoint, I agree that the first exception (`rootCause`)
is usually more interesting than the last (`caughtException`) to debug and
resolve the problem but users/clients shouldn't have to worry with such details
[1].
[1] Effective Java, Item 73: Throw exceptions appropriate to the abstraction
--
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-unsubscr...@hive.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscr...@hive.apache.org
For additional commands, e-mail: gitbox-h...@hive.apache.org
