zabetak commented on code in PR #3518:
URL: https://github.com/apache/hive/pull/3518#discussion_r951459418
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/ReduceRecordSource.java:
##########
@@ -373,8 +370,7 @@ public void next() throws HiveException {
try {
rowString = SerDeUtils.getJSONString(row, rowObjectInspector);
} catch (Exception e2) {
- rowString = "[Error getting row data with exception "
- + StringUtils.stringifyException(e2) + " ]";
+ l4j.trace("Error getting row data (tag={})", tag, e2);
Review Comment:
The refactoring in `ExecReducer` was a bit different. Maybe it makes sense
to keep things uniform.
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/ReduceRecordSource.java:
##########
@@ -502,8 +495,7 @@ private void processVectorGroup(BytesWritable keyWritable,
try {
rowString = batch.toString();
} catch (Exception e2) {
- rowString = "[Error getting row data with exception "
- + StringUtils.stringifyException(e2) + " ]";
+ l4j.error("Error getting row data (tag={})", tag, e2);
Review Comment:
The refactoring in `ExecReducer` is different.
Moreover, previously we used `l4j.trace` and here we use `l4j.error`. The
discussion in `HIVE-20644` indicates that maybe it is safer to log at trace
level when it comes to data. This is outside the scope of this PR but maybe
worth filing a JIRA case for this.
##########
ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDAFSum.java:
##########
@@ -286,11 +286,7 @@ public void iterate(AggregationBuffer agg, Object[]
parameters) throws HiveExcep
} catch (NumberFormatException e) {
if (!warned) {
warned = true;
- LOG.warn(getClass().getSimpleName() + " "
- + StringUtils.stringifyException(e));
- LOG
- .warn(getClass().getSimpleName()
- + " ignoring similar exceptions.");
+ LOG.warn("{}: ignoring similar exceptions",
getClass().getSimpleName(), e);
Review Comment:
Remove unused `import org.apache.hadoop.util.StringUtils;`
##########
ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDAFSum.java:
##########
@@ -286,11 +286,7 @@ public void iterate(AggregationBuffer agg, Object[]
parameters) throws HiveExcep
} catch (NumberFormatException e) {
if (!warned) {
warned = true;
- LOG.warn(getClass().getSimpleName() + " "
- + StringUtils.stringifyException(e));
- LOG
- .warn(getClass().getSimpleName()
- + " ignoring similar exceptions.");
+ LOG.warn("{}: ignoring similar exceptions",
getClass().getSimpleName(), e);
Review Comment:
The `getClass().getSimpleName()` is a bit useless/redundant for two reasons:
* The class name will appear in the stacktrace;
* The log configuration can be changed to include the class name if
necessary.
The comment applies to all changes in this class.
The proposed refactoring though simply retains the old behavior so this is
not a blocking comment.
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/ReduceRecordSource.java:
##########
@@ -398,15 +394,12 @@ private boolean pushRecordVector() {
processVectorGroup(keyWritable, valueWritables, tag);
return true;
- } catch (Throwable e) {
+ } catch (OutOfMemoryError oom) {
abort = true;
- if (e instanceof OutOfMemoryError) {
- // Don't create a new object if we are already out of memory
- throw (OutOfMemoryError) e;
- } else {
- l4j.error(StringUtils.stringifyException(e));
- throw new RuntimeException(e);
- }
+ // Do not create a new object if we are already out of memory
+ throw oom;
+ } catch (Throwable t) {
+ throw new RuntimeException(t);
Review Comment:
`abort = true;` is missing here. This is the only major review comment in
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]
