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 2022/07/27 03:05:11 UTC
[GitHub] [hive] belugabehr opened a new pull request, #3478: HIVE-22417: Remove stringifyException from MetaStore
belugabehr opened a new pull request, #3478:
URL: https://github.com/apache/hive/pull/3478
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://cwiki.apache.org/confluence/display/Hive/HowToContribute
2. Ensure that you have created an issue on the Hive project JIRA: https://issues.apache.org/jira/projects/HIVE/summary
3. Ensure you have added or run the appropriate tests for your PR:
4. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP]HIVE-XXXXX: Your PR title ...'.
5. Be sure to keep the PR description updated to reflect all changes.
6. Please write your PR title to summarize what this PR proposes.
7. If possible, provide a concise example to reproduce the issue for a faster review.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description, screenshot and/or a reproducable example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Hive versions or within the unreleased branches such as master.
If no, write 'No'.
-->
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
-->
--
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] belugabehr commented on a diff in pull request #3478: HIVE-22417: Remove stringifyException from MetaStore
Posted by GitBox <gi...@apache.org>.
belugabehr commented on code in PR #3478:
URL: https://github.com/apache/hive/pull/3478#discussion_r932668308
##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/ReplChangeManager.java:
##########
@@ -205,7 +205,7 @@ private ReplChangeManager(Configuration conf) throws MetaException {
inited = true;
}
} catch (IOException e) {
- throw new MetaException(StringUtils.stringifyException(e));
+ throw new MetaException(e.getMessage());
Review Comment:
Also, I see I did this log-and-throw in a couple of other places. Good catch. Thanks. Will update.
--
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 #3478: HIVE-22417: Remove stringifyException from MetaStore
Posted by GitBox <gi...@apache.org>.
zabetak commented on code in PR #3478:
URL: https://github.com/apache/hive/pull/3478#discussion_r932413646
##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/ReplChangeManager.java:
##########
@@ -205,7 +205,7 @@ private ReplChangeManager(Configuration conf) throws MetaException {
inited = true;
}
} catch (IOException e) {
- throw new MetaException(StringUtils.stringifyException(e));
+ throw new MetaException(e.getMessage());
Review Comment:
Should we `LOG.error("..", e)` before throwing to avoid losing the complete stacktrace?
##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/ReplChangeManager.java:
##########
@@ -378,32 +378,28 @@ static Path getCMPath(Configuration conf, String name, String checkSum, String c
* @return Corresponding FileInfo object
*/
public static FileInfo getFileInfo(Path src, String checksumString, String srcCMRootURI, String subDir,
- Configuration conf) throws MetaException {
- try {
- FileSystem srcFs = src.getFileSystem(conf);
- if (checksumString == null) {
- return new FileInfo(srcFs, src, subDir);
- }
+ Configuration conf) throws IOException {
+ FileSystem srcFs = src.getFileSystem(conf);
+ if (checksumString == null) {
+ return new FileInfo(srcFs, src, subDir);
+ }
- Path cmPath = getCMPath(conf, src.getName(), checksumString, srcCMRootURI);
- if (!srcFs.exists(src)) {
- return new FileInfo(srcFs, src, cmPath, checksumString, false, subDir);
- }
+ Path cmPath = getCMPath(conf, src.getName(), checksumString, srcCMRootURI);
+ if (!srcFs.exists(src)) {
+ return new FileInfo(srcFs, src, cmPath, checksumString, false, subDir);
+ }
- String currentChecksumString;
- try {
- currentChecksumString = checksumFor(src, srcFs);
- } catch (IOException ex) {
- // If the file is missing or getting modified, then refer CM path
- return new FileInfo(srcFs, src, cmPath, checksumString, false, subDir);
- }
- if ((currentChecksumString == null) || checksumString.equals(currentChecksumString)) {
- return new FileInfo(srcFs, src, cmPath, checksumString, true, subDir);
- } else {
- return new FileInfo(srcFs, src, cmPath, checksumString, false, subDir);
- }
- } catch (IOException e) {
- throw new MetaException(StringUtils.stringifyException(e));
Review Comment:
Dropping the `MetaException` from this method seems like a good idea to me but do we understand why it was added in the first place?
--
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] belugabehr commented on a diff in pull request #3478: HIVE-22417: Remove stringifyException from MetaStore
Posted by GitBox <gi...@apache.org>.
belugabehr commented on code in PR #3478:
URL: https://github.com/apache/hive/pull/3478#discussion_r932663972
##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/ReplChangeManager.java:
##########
@@ -378,32 +378,28 @@ static Path getCMPath(Configuration conf, String name, String checkSum, String c
* @return Corresponding FileInfo object
*/
public static FileInfo getFileInfo(Path src, String checksumString, String srcCMRootURI, String subDir,
- Configuration conf) throws MetaException {
- try {
- FileSystem srcFs = src.getFileSystem(conf);
- if (checksumString == null) {
- return new FileInfo(srcFs, src, subDir);
- }
+ Configuration conf) throws IOException {
+ FileSystem srcFs = src.getFileSystem(conf);
+ if (checksumString == null) {
+ return new FileInfo(srcFs, src, subDir);
+ }
- Path cmPath = getCMPath(conf, src.getName(), checksumString, srcCMRootURI);
- if (!srcFs.exists(src)) {
- return new FileInfo(srcFs, src, cmPath, checksumString, false, subDir);
- }
+ Path cmPath = getCMPath(conf, src.getName(), checksumString, srcCMRootURI);
+ if (!srcFs.exists(src)) {
+ return new FileInfo(srcFs, src, cmPath, checksumString, false, subDir);
+ }
- String currentChecksumString;
- try {
- currentChecksumString = checksumFor(src, srcFs);
- } catch (IOException ex) {
- // If the file is missing or getting modified, then refer CM path
- return new FileInfo(srcFs, src, cmPath, checksumString, false, subDir);
- }
- if ((currentChecksumString == null) || checksumString.equals(currentChecksumString)) {
- return new FileInfo(srcFs, src, cmPath, checksumString, true, subDir);
- } else {
- return new FileInfo(srcFs, src, cmPath, checksumString, false, subDir);
- }
- } catch (IOException e) {
- throw new MetaException(StringUtils.stringifyException(e));
Review Comment:
This comes from [HIVE-15525](https://issues.apache.org/jira/browse/HIVE-15525). I'm do not understand why this particular decision was made.
--
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] belugabehr merged pull request #3478: HIVE-22417: Remove stringifyException from MetaStore
Posted by GitBox <gi...@apache.org>.
belugabehr merged PR #3478:
URL: https://github.com/apache/hive/pull/3478
--
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 #3478: HIVE-22417: Remove stringifyException from MetaStore
Posted by GitBox <gi...@apache.org>.
zabetak commented on code in PR #3478:
URL: https://github.com/apache/hive/pull/3478#discussion_r933296697
##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/ReplChangeManager.java:
##########
@@ -205,7 +205,7 @@ private ReplChangeManager(Configuration conf) throws MetaException {
inited = true;
}
} catch (IOException e) {
- throw new MetaException(StringUtils.stringifyException(e));
+ throw new MetaException(e.getMessage());
Review Comment:
I agree that "Log-and-Throw" is problematic but losing the cause (or part of the stack trace) is equally bad (maybe worse).
I was thinking a bit more about this and came up with the following idea:
```java
throw MetaStoreUtils.newMetaException("Failed to create ReplChangeManager", e);
```
Using the above snippet we can avoid the "Log-and-Throw" anti-pattern and also retain the original cause and stack trace without plumbing everything into the exception message. If ever this error reaches the client it will have a meaningful message pointing the problem without the redundant stack trace.
One problem that I see with my proposal (but also with the proposed changes in this PR to deprecate `StringUtils.stringifyException`) is that if the `MetaException` propagates all the way up to the Thrift processor (i.e., nobody catches it) then we will have no clue about the real cause; there will be no log events and the error reaching the client will only contain the message (no cause neither stacktrace). Despite this problem, I still believe that the pattern of putting the whole stacktrace as a message in another exception is a bad practice and should not be used.
Moreover, I am not in favor of sending complete stack traces from server to client. I know that there were attempts to explicitly do this (e.g., HIVE-3626, HIVE-26345, etc) in order to give more information to the user but I would advise against. Doing this can leak sensitive information and also goes against the usual guidelines of throwing exceptions appropriate to the abstraction (Effective Java, Item 61: Throw exceptions appropriate to the abstraction).
Long story short, I think it is worth pushing this PR forward possibly incorporating the proposal outlined above. WDYT @belugabehr ?
##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/ReplChangeManager.java:
##########
@@ -378,32 +378,28 @@ static Path getCMPath(Configuration conf, String name, String checkSum, String c
* @return Corresponding FileInfo object
*/
public static FileInfo getFileInfo(Path src, String checksumString, String srcCMRootURI, String subDir,
- Configuration conf) throws MetaException {
- try {
- FileSystem srcFs = src.getFileSystem(conf);
- if (checksumString == null) {
- return new FileInfo(srcFs, src, subDir);
- }
+ Configuration conf) throws IOException {
+ FileSystem srcFs = src.getFileSystem(conf);
+ if (checksumString == null) {
+ return new FileInfo(srcFs, src, subDir);
+ }
- Path cmPath = getCMPath(conf, src.getName(), checksumString, srcCMRootURI);
- if (!srcFs.exists(src)) {
- return new FileInfo(srcFs, src, cmPath, checksumString, false, subDir);
- }
+ Path cmPath = getCMPath(conf, src.getName(), checksumString, srcCMRootURI);
+ if (!srcFs.exists(src)) {
+ return new FileInfo(srcFs, src, cmPath, checksumString, false, subDir);
+ }
- String currentChecksumString;
- try {
- currentChecksumString = checksumFor(src, srcFs);
- } catch (IOException ex) {
- // If the file is missing or getting modified, then refer CM path
- return new FileInfo(srcFs, src, cmPath, checksumString, false, subDir);
- }
- if ((currentChecksumString == null) || checksumString.equals(currentChecksumString)) {
- return new FileInfo(srcFs, src, cmPath, checksumString, true, subDir);
- } else {
- return new FileInfo(srcFs, src, cmPath, checksumString, false, subDir);
- }
- } catch (IOException e) {
- throw new MetaException(StringUtils.stringifyException(e));
Review Comment:
The decision of throwing `MetaException` in HIVE-15525 was either an oversight or an attempt to keep this method inline with other methods in this class. Ideally, the only place that should throw `MetaException` should be the `HMSHandler` so removing it from here is a net positive change from my perspective.
Moreover, I checked the immediate callers of this method and it seems completely safe to drop the `MetaException` and propagate the `IOException` since it will not affect the behavior of HMS.
--
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 #3478: HIVE-22417: Remove stringifyException from MetaStore
Posted by GitBox <gi...@apache.org>.
zabetak commented on code in PR #3478:
URL: https://github.com/apache/hive/pull/3478#discussion_r933784608
##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/ReplChangeManager.java:
##########
@@ -205,7 +205,7 @@ private ReplChangeManager(Configuration conf) throws MetaException {
inited = true;
}
} catch (IOException e) {
- throw new MetaException(StringUtils.stringifyException(e));
+ throw new MetaException(e.getMessage());
Review Comment:
I am happy that we are on the same page :)
The first thing that came to my mind is doing this:
```java
throw new MetaException("Failed to create ReplChangeManager", e)
```
but it is not possible since MetaException class is generated by Thrift and is missing the relevant constructor.
The next best thing was:
```java
throw (MetaException) new MetaException("Failed to create ReplChangeManager").initCause(e);
```
but it is quite verbose so it feels natural to create a util and that's how I think `MetaStoreUtils.newMetaException` was introduced.
Personally, I would prefer to use `MetaStoreUtils.newMetaException` here but I understand that this is a bit subjective so I do not insist. The points you raised make perfect sense so I am leaving the final decision up to you. You have my `+1` on whatever you pick.
--
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] belugabehr commented on a diff in pull request #3478: HIVE-22417: Remove stringifyException from MetaStore
Posted by GitBox <gi...@apache.org>.
belugabehr commented on code in PR #3478:
URL: https://github.com/apache/hive/pull/3478#discussion_r932663609
##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/ReplChangeManager.java:
##########
@@ -205,7 +205,7 @@ private ReplChangeManager(Configuration conf) throws MetaException {
inited = true;
}
} catch (IOException e) {
- throw new MetaException(StringUtils.stringifyException(e));
+ throw new MetaException(e.getMessage());
Review Comment:
Ya, this ideally should just throw something other than a MetaException, but it wasn't easy to fix like the other location. Log-and-Throw is a bad practice; the code should do one or the other, not both. However, if you would like, I can add a log along side the throw. Log-and-throw is probably a better compromise.
The idea in general though is that MetaException is an Exception that gets passed back to the client via the Thrift API. It should not be part of any of the core Hive code,... Hive should simply log the exception (`error`) at the Thrift service shim and throw MetaException back 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] belugabehr commented on a diff in pull request #3478: HIVE-22417: Remove stringifyException from MetaStore
Posted by GitBox <gi...@apache.org>.
belugabehr commented on code in PR #3478:
URL: https://github.com/apache/hive/pull/3478#discussion_r933556524
##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/ReplChangeManager.java:
##########
@@ -205,7 +205,7 @@ private ReplChangeManager(Configuration conf) throws MetaException {
inited = true;
}
} catch (IOException e) {
- throw new MetaException(StringUtils.stringifyException(e));
+ throw new MetaException(e.getMessage());
Review Comment:
I completely agree with everything you have put forward here. Checkout out. HIVE-25126.
I do not think however it is such a burden to log-and-throw for now,... with the end goal of removing it completely. I think adding a utility work-around like this is not ideal in an open-source project because it becomes yet another little thing everyone needs to be aware of, whereas log-and-throw is a much more natural (albeit anti-pattern) behavior.
So to summarize my position:
Yes. Should remove all references to MetaException within the core Hive product. It should only exists as a log-and-throw at the top-level service level. It should contain a helpful message for the client in order to respond appropriately to the error condition, and the details (i.e., complete stacktrace) should be logged on the server. This will making debugging much more consistent. There have been many times I have been stumped because the stack trace prints on the client side, and just a small message (or nothing at all) appears in the server logs. It's also confusing because I'm never sure at first if the stacktrace presented on the client side occurred within the server or within the client itself.
Yes. Sending internal stack traces to external clients is a security risk
Yes. `StringUtils.stringifyException` is duplicative of what SLF4J offers and should be removed completely.
Yes. I think we should push through this PR and I can do some more of this work by the same parameters discussed here. Just waiting on your `+1` :)
--
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