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: [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]
