zabetak commented on code in PR #3506:
URL: https://github.com/apache/hive/pull/3506#discussion_r951498152
##########
accumulo-handler/src/test/org/apache/hadoop/hive/accumulo/predicate/TestAccumuloPredicateHandler.java:
##########
@@ -386,7 +386,7 @@ public void testPushdownComparisonOptNotSupported() {
} catch (RuntimeException e) {
assertTrue(e.getMessage().contains("Unexpected residual predicate:
field1 is not null"));
} catch (Exception e) {
- fail(StringUtils.stringifyException(e));
+ fail(e.getMessage());
}
Review Comment:
Remove unused `import org.apache.hadoop.util.StringUtils;`
##########
accumulo-handler/src/java/org/apache/hadoop/hive/accumulo/AccumuloStorageHandler.java:
##########
@@ -369,11 +369,7 @@ public void preCreateTable(Table table) throws
MetaException {
tableOpts.create(idxTable);
}
}
- } catch (AccumuloSecurityException e) {
- throw new MetaException(StringUtils.stringifyException(e));
- } catch (TableExistsException e) {
- throw new MetaException(StringUtils.stringifyException(e));
- } catch (AccumuloException e) {
+ } catch (AccumuloSecurityException | TableExistsException |
AccumuloException e) {
Review Comment:
In a previous PR (https://github.com/apache/hive/pull/3478) we opted to use
the LOG + throw pattern as an alternative of removing `stringifyException`
acknowledging advantages and disadvantages of it. Why we don't want to do the
same here?
##########
accumulo-handler/src/test/org/apache/hadoop/hive/accumulo/predicate/TestAccumuloPredicateHandler.java:
##########
@@ -386,7 +386,7 @@ public void testPushdownComparisonOptNotSupported() {
} catch (RuntimeException e) {
assertTrue(e.getMessage().contains("Unexpected residual predicate:
field1 is not null"));
} catch (Exception e) {
- fail(StringUtils.stringifyException(e));
+ fail(e.getMessage());
}
Review Comment:
It seems we don't really need this catch.
##########
accumulo-handler/src/test/org/apache/hadoop/hive/accumulo/predicate/TestAccumuloPredicateHandler.java:
##########
@@ -475,7 +474,7 @@ public void testIgnoreIteratorPushdown() throws
TooManyAccumuloColumnsException
List<IteratorSetting> iterators = handler.getIterators(conf,
columnMapper);
assertEquals(iterators.size(), 0);
} catch (Exception e) {
- fail(StringUtils.stringifyException(e));
+ fail(e.getMessage());
}
Review Comment:
Same comment as before.
##########
druid-handler/src/test/org/apache/hadoop/hive/druid/QTestDruidSerDe.java:
##########
@@ -96,7 +96,7 @@ public class QTestDruidSerDe extends DruidSerDe {
DruidStorageHandlerUtils.JSON_MAPPER.readValue(RESPONSE, new
TypeReference<List<SegmentAnalysis>>() {
});
} catch (Exception e) {
- throw new SerDeException(StringUtils.stringifyException(e));
+ throw new SerDeException(e);
Review Comment:
Remove unused `import org.apache.hadoop.util.StringUtils;`
##########
accumulo-handler/src/java/org/apache/hadoop/hive/accumulo/mr/HiveAccumuloTableInputFormat.java:
##########
@@ -175,15 +175,8 @@ public InputSplit[] getSplits(JobConf jobConf, int
numSplits) throws IOException
}
return hiveSplits;
- } catch (AccumuloException e) {
- log.error("Could not configure AccumuloInputFormat", e);
- throw new IOException(StringUtils.stringifyException(e));
- } catch (AccumuloSecurityException e) {
- log.error("Could not configure AccumuloInputFormat", e);
- throw new IOException(StringUtils.stringifyException(e));
- } catch (SerDeException e) {
- log.error("Could not configure AccumuloInputFormat", e);
- throw new IOException(StringUtils.stringifyException(e));
+ } catch (AccumuloException | AccumuloSecurityException | SerDeException e)
{
+ throw new IOException("Could not configure AccumuloInputFormat", e);
Review Comment:
Remove unused `import org.apache.hadoop.util.StringUtils;`
##########
accumulo-handler/src/test/org/apache/hadoop/hive/accumulo/predicate/TestAccumuloPredicateHandler.java:
##########
@@ -423,7 +423,6 @@ public void testIteratorIgnoreRowIDFields() {
List<IteratorSetting> iterators = handler.getIterators(conf,
columnMapper);
assertEquals(iterators.size(), 0);
} catch (SerDeException e) {
- StringUtils.stringifyException(e);
}
Review Comment:
Ignoring the exception seems wrong. Most likely the test should fail in case
of exception. Can we fix this as part of 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: [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]
