You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@hive.apache.org by GitBox <gi...@apache.org> on 2023/01/11 17:53:16 UTC
[GitHub] [hive] wecharyu opened a new pull request, #3938: HIVE-26935: Expose root cause of MetaException to client sides
wecharyu opened a new pull request, #3938:
URL: https://github.com/apache/hive/pull/3938
### What changes were proposed in this pull request?
We are trying to expose the root cause of MetaException in the message:
1. Refactor the MetaException message in RetryingHMSHandler with following format:
```sh
MetaException(message:One or more instances could not be deleted
Root cause: java.sql.SQLIntegrityConstraintViolationException: Cannot delete or update a parent row)
```
2. Check if the exception can be retry in HMS server.
### Why are the changes needed?
1. Expose root cause for user troubleshooting.
2. Root cause in message can help skip some unnecessary retry in both client and server sides.
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Add Unit test.
--
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
[GitHub] [hive] wecharyu commented on a diff in pull request #3938: HIVE-26935: Expose root cause of MetaException to client sides
Posted by "wecharyu (via GitHub)" <gi...@apache.org>.
wecharyu commented on code in PR #3938:
URL: https://github.com/apache/hive/pull/3938#discussion_r1089755691
##########
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:
OK, I will remove this behavior.
--
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
[GitHub] [hive] wecharyu commented on a diff in pull request #3938: HIVE-26935: Expose root cause of MetaException to client sides
Posted by "wecharyu (via GitHub)" <gi...@apache.org>.
wecharyu commented on code in PR #3938:
URL: https://github.com/apache/hive/pull/3938#discussion_r1086177278
##########
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:
Make sense. How about encapsulating it as a util function?
--
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
[GitHub] [hive] zabetak commented on a diff in pull request #3938: HIVE-26935: Expose root cause of MetaException to client sides
Posted by "zabetak (via GitHub)" <gi...@apache.org>.
zabetak commented on code in PR #3938:
URL: https://github.com/apache/hive/pull/3938#discussion_r1087810684
##########
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:
Users are not necessarily developers; throwing exceptions back to them should be the norm. Consider for instance some of the most popular DBMS (Postgres, Oracle, MSSQL, etc); I don't think anyone is exposing exceptions to the user.
This is a minor comment not really blocking but I wanted to mention that propagating exceptions to the client is not the ideal solution.
--
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
[GitHub] [hive] wecharyu commented on a diff in pull request #3938: HIVE-26935: Expose root cause of MetaException to client sides
Posted by "wecharyu (via GitHub)" <gi...@apache.org>.
wecharyu commented on code in PR #3938:
URL: https://github.com/apache/hive/pull/3938#discussion_r1089770003
##########
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:
In the prod environment, HMS may be transparent to users. And since we will pass the exception messages to the client, why not pass the most useful messages?
--
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
[GitHub] [hive] wecharyu commented on pull request #3938: HIVE-26935: Expose root cause of MetaException to client sides
Posted by GitBox <gi...@apache.org>.
wecharyu commented on PR #3938:
URL: https://github.com/apache/hive/pull/3938#issuecomment-1381931269
@dengzhhu653 @zabetak Could you please help review this PR?
--
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
[GitHub] [hive] zabetak commented on a diff in pull request #3938: HIVE-26935: Expose root cause of MetaException to client sides
Posted by "zabetak (via GitHub)" <gi...@apache.org>.
zabetak commented on code in PR #3938:
URL: https://github.com/apache/hive/pull/3938#discussion_r1087799467
##########
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:
I know but the exception is more useful for the server than it is for the client thus it should be complete. The people that will debug the problem are looking in the server logs not in the client logs.
--
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
[GitHub] [hive] sonarcloud[bot] commented on pull request #3938: HIVE-26935: Expose root cause of MetaException to client sides
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #3938:
URL: https://github.com/apache/hive/pull/3938#issuecomment-1381969337
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=3938)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3938&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3938&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3938&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3938&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3938&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3938&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3938&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3938&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3938&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3938&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3938&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3938&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=3938&metric=coverage&view=list) No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=3938&metric=duplicated_lines_density&view=list) No Duplication information
--
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
[GitHub] [hive] zabetak commented on a diff in pull request #3938: HIVE-26935: Expose root cause of MetaException to client sides
Posted by "zabetak (via GitHub)" <gi...@apache.org>.
zabetak commented on code in PR #3938:
URL: https://github.com/apache/hive/pull/3938#discussion_r1087810684
##########
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:
Users are not necessarily developers; throwing exceptions back to them shouldn't be the norm. Consider for instance some of the most popular DBMS (Postgres, Oracle, MSSQL, etc); I don't think anyone is exposing exceptions to the user.
This is a minor comment not really blocking but I wanted to mention that propagating exceptions to the client is not the ideal solution.
--
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
[GitHub] [hive] wecharyu commented on a diff in pull request #3938: HIVE-26935: Expose root cause of MetaException to client sides
Posted by "wecharyu (via GitHub)" <gi...@apache.org>.
wecharyu commented on code in PR #3938:
URL: https://github.com/apache/hive/pull/3938#discussion_r1089759237
##########
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:
I think the server logs can get the full stack in this line:
https://github.com/apache/hive/blob/d8f9a977cf37afcb2b5cdc7b1ea9e57e26908bc3/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/RetryingHMSHandler.java#L207
The thrown `me` does not log in server actually.
--
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
[GitHub] [hive] zabetak closed pull request #3938: HIVE-26935: Expose root cause of MetaException to client sides
Posted by "zabetak (via GitHub)" <gi...@apache.org>.
zabetak closed pull request #3938: HIVE-26935: Expose root cause of MetaException to client sides
URL: https://github.com/apache/hive/pull/3938
--
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
[GitHub] [hive] wecharyu commented on a diff in pull request #3938: HIVE-26935: Expose root cause of MetaException to client sides
Posted by "wecharyu (via GitHub)" <gi...@apache.org>.
wecharyu commented on code in PR #3938:
URL: https://github.com/apache/hive/pull/3938#discussion_r1086186793
##########
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:
In this case, the `caughtException` is always `JDOException`. We should be able to get the deep cause and should not have the duplication message. Also I think the root cause can help users understand why this exception happens.
--
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
[GitHub] [hive] sonarcloud[bot] commented on pull request #3938: HIVE-26935: Expose root cause of MetaException to client sides
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #3938:
URL: https://github.com/apache/hive/pull/3938#issuecomment-1379353775
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=3938)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3938&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3938&resolved=false&types=BUG) [1 Bug](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3938&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3938&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3938&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3938&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3938&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3938&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3938&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3938&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3938&resolved=false&types=CODE_SMELL) [1 Code Smell](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3938&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=3938&metric=coverage&view=list) No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=3938&metric=duplicated_lines_density&view=list) No Duplication information
--
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
[GitHub] [hive] zabetak commented on a diff in pull request #3938: HIVE-26935: Expose root cause of MetaException to client sides
Posted by "zabetak (via GitHub)" <gi...@apache.org>.
zabetak commented on code in PR #3938:
URL: https://github.com/apache/hive/pull/3938#discussion_r1090412818
##########
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:
This is an improvement given the current situation and that's why I think its worth merging. My comments are more forward looking on things that we could improve in the future.
--
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
[GitHub] [hive] wecharyu commented on a diff in pull request #3938: HIVE-26935: Expose root cause of MetaException to client sides
Posted by "wecharyu (via GitHub)" <gi...@apache.org>.
wecharyu commented on code in PR #3938:
URL: https://github.com/apache/hive/pull/3938#discussion_r1086181889
##########
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:
The thrown `MetaException` will only be received by client, but the init cause will not be transmitted to the client.
--
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
[GitHub] [hive] zabetak commented on a diff in pull request #3938: HIVE-26935: Expose root cause of MetaException to client sides
Posted by "zabetak (via GitHub)" <gi...@apache.org>.
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-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
[GitHub] [hive] zabetak commented on a diff in pull request #3938: HIVE-26935: Expose root cause of MetaException to client sides
Posted by "zabetak (via GitHub)" <gi...@apache.org>.
zabetak commented on code in PR #3938:
URL: https://github.com/apache/hive/pull/3938#discussion_r1087803841
##########
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:
I would rather leave things as they are right now as far as it concerns this method. If we want to change the behavior of the server let's discuss under a dedicated JIRA.
--
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
[GitHub] [hive] zabetak commented on a diff in pull request #3938: HIVE-26935: Expose root cause of MetaException to client sides
Posted by "zabetak (via GitHub)" <gi...@apache.org>.
zabetak commented on code in PR #3938:
URL: https://github.com/apache/hive/pull/3938#discussion_r1090441719
##########
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:
If this change was somewhere deeper in the metastore code then I would probably keep the code as is.
In this case though, we are just before returning the exception to the client, I finally agree with you that using `initCause` is a bit useless.
--
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