You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Szehon Ho <sz...@cloudera.com> on 2013/10/26 04:21:23 UTC
Review Request 14970: HIVE-4723 DDLSemanticAnalyzer.addTablePartsOutputs
eats several exceptions
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14970/
-----------------------------------------------------------
Review request for hive.
Bugs: HIVE-4723
https://issues.apache.org/jira/browse/HIVE-4723
Repository: hive-git
Description
-------
Throwing the offending exception, that was swallowed. Also changing the underlying exception to use the standard error message.
Diffs
-----
ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java b0f124b
ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java 037191a
Diff: https://reviews.apache.org/r/14970/diff/
Testing
-------
Thanks,
Szehon Ho
Re: Review Request 14970: HIVE-4723 DDLSemanticAnalyzer.addTablePartsOutputs
eats several exceptions
Posted by Szehon Ho <sz...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14970/
-----------------------------------------------------------
(Updated Oct. 28, 2013, 10:30 p.m.)
Review request for hive.
Changes
-------
Adding the exception to the log. Verified by checking hive.log.
In my observation, the NPE for non-partitioned table use-case occurred at DDLSemanticAnalyzer 2951 : if (parts.isempty()), as parts is null after swallowing the exception.
Bugs: HIVE-4723
https://issues.apache.org/jira/browse/HIVE-4723
Repository: hive-git
Description
-------
Throwing the offending exception, that was swallowed. Also changing the underlying exception to use the standard error message.
Diffs (updated)
-----
ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java b0f124b
ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java 037191a
ql/src/test/results/clientnegative/alter_rename_partition_failure3.q.out 384fcbe
ql/src/test/results/clientnegative/touch2.q.out 045121a
Diff: https://reviews.apache.org/r/14970/diff/
Testing
-------
Thanks,
Szehon Ho
Re: Review Request 14970: HIVE-4723 DDLSemanticAnalyzer.addTablePartsOutputs
eats several exceptions
Posted by Szehon Ho <sz...@cloudera.com>.
> On Oct. 28, 2013, 9:33 p.m., Brock Noland wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java, line 2936
> > <https://reviews.apache.org/r/14970/diff/2/?file=372522#file372522line2936>
> >
> > Hi,
> >
> > Thank you for working on this! It's not actually being logged though.
> >
> > See
> >
> > LOG.error("Got HiveException during obtaining list of partitions");
> >
> > No exception is being logged. That is the reason I created this JIRA, not to throw a new exception.
Thanks, I got your point :)... I will add the exception to the log. The JIRA said "The error message was bad, hive ate an exception, and NPE'ed." thats why I thought that ate exception and NPE was the main problem.
I think there is some value in throwing a semantic exception with the correct error message if we get exception here, instead of allowing it to go forward, as it is a better exception to the end-user that way. Swallowing it will get either NPE down the line for non-partitioned table, or some exception of DDL Task. Or should we just keep the existing behavior? Thanks
- Szehon
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14970/#review27638
-----------------------------------------------------------
On Oct. 28, 2013, 8:40 p.m., Szehon Ho wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14970/
> -----------------------------------------------------------
>
> (Updated Oct. 28, 2013, 8:40 p.m.)
>
>
> Review request for hive.
>
>
> Bugs: HIVE-4723
> https://issues.apache.org/jira/browse/HIVE-4723
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Throwing the offending exception, that was swallowed. Also changing the underlying exception to use the standard error message.
>
>
> Diffs
> -----
>
> ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java b0f124b
> ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java 037191a
> ql/src/test/results/clientnegative/alter_rename_partition_failure3.q.out 384fcbe
> ql/src/test/results/clientnegative/touch2.q.out 045121a
>
> Diff: https://reviews.apache.org/r/14970/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Szehon Ho
>
>
Re: Review Request 14970: HIVE-4723 DDLSemanticAnalyzer.addTablePartsOutputs
eats several exceptions
Posted by Brock Noland <br...@cloudera.com>.
> On Oct. 28, 2013, 9:33 p.m., Brock Noland wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java, line 2936
> > <https://reviews.apache.org/r/14970/diff/2/?file=372522#file372522line2936>
> >
> > Hi,
> >
> > Thank you for working on this! It's not actually being logged though.
> >
> > See
> >
> > LOG.error("Got HiveException during obtaining list of partitions");
> >
> > No exception is being logged. That is the reason I created this JIRA, not to throw a new exception.
>
> Szehon Ho wrote:
> Thanks, I got your point :)... I will add the exception to the log. The JIRA said "The error message was bad, hive ate an exception, and NPE'ed." thats why I thought that ate exception and NPE was the main problem.
>
> I think there is some value in throwing a semantic exception with the correct error message if we get exception here, instead of allowing it to go forward, as it is a better exception to the end-user that way. Swallowing it will get either NPE down the line for non-partitioned table, or some exception of DDL Task. Or should we just keep the existing behavior? Thanks
Yeah actually we might want to throw. I had forgotten the original error. Let's let a test run go and see if anything fails.
- Brock
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14970/#review27638
-----------------------------------------------------------
On Oct. 28, 2013, 8:40 p.m., Szehon Ho wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14970/
> -----------------------------------------------------------
>
> (Updated Oct. 28, 2013, 8:40 p.m.)
>
>
> Review request for hive.
>
>
> Bugs: HIVE-4723
> https://issues.apache.org/jira/browse/HIVE-4723
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Throwing the offending exception, that was swallowed. Also changing the underlying exception to use the standard error message.
>
>
> Diffs
> -----
>
> ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java b0f124b
> ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java 037191a
> ql/src/test/results/clientnegative/alter_rename_partition_failure3.q.out 384fcbe
> ql/src/test/results/clientnegative/touch2.q.out 045121a
>
> Diff: https://reviews.apache.org/r/14970/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Szehon Ho
>
>
Re: Review Request 14970: HIVE-4723 DDLSemanticAnalyzer.addTablePartsOutputs
eats several exceptions
Posted by Brock Noland <br...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14970/#review27638
-----------------------------------------------------------
ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java
<https://reviews.apache.org/r/14970/#comment53691>
Hi,
Thank you for working on this! It's not actually being logged though.
See
LOG.error("Got HiveException during obtaining list of partitions");
No exception is being logged. That is the reason I created this JIRA, not to throw a new exception.
- Brock Noland
On Oct. 28, 2013, 8:40 p.m., Szehon Ho wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14970/
> -----------------------------------------------------------
>
> (Updated Oct. 28, 2013, 8:40 p.m.)
>
>
> Review request for hive.
>
>
> Bugs: HIVE-4723
> https://issues.apache.org/jira/browse/HIVE-4723
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Throwing the offending exception, that was swallowed. Also changing the underlying exception to use the standard error message.
>
>
> Diffs
> -----
>
> ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java b0f124b
> ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java 037191a
> ql/src/test/results/clientnegative/alter_rename_partition_failure3.q.out 384fcbe
> ql/src/test/results/clientnegative/touch2.q.out 045121a
>
> Diff: https://reviews.apache.org/r/14970/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Szehon Ho
>
>
Re: Review Request 14970: HIVE-4723 DDLSemanticAnalyzer.addTablePartsOutputs
eats several exceptions
Posted by Szehon Ho <sz...@cloudera.com>.
> On Oct. 28, 2013, 8:43 p.m., Brock Noland wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java, line 2936
> > <https://reviews.apache.org/r/14970/diff/2/?file=372522#file372522line2936>
> >
> > Is this what we want? I was assume we only wanted to log the exception at this point since it's not being logged.
Hi Brock, the exception is still being logged in the catch block, and was logged even before, that behavior did not change.
But the problem is, the logging catch block eats the exception, losing the information. Then, it causes a NPE down the line also in the same method (line 2949: if parts.isEmpty()).
I read the JIRA to be to throw the correct exception which today is being eaten and lost, and to not throw the misleading NPE. Or am I misunderstanding it? Thank you.
- Szehon
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14970/#review27634
-----------------------------------------------------------
On Oct. 28, 2013, 8:40 p.m., Szehon Ho wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14970/
> -----------------------------------------------------------
>
> (Updated Oct. 28, 2013, 8:40 p.m.)
>
>
> Review request for hive.
>
>
> Bugs: HIVE-4723
> https://issues.apache.org/jira/browse/HIVE-4723
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Throwing the offending exception, that was swallowed. Also changing the underlying exception to use the standard error message.
>
>
> Diffs
> -----
>
> ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java b0f124b
> ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java 037191a
> ql/src/test/results/clientnegative/alter_rename_partition_failure3.q.out 384fcbe
> ql/src/test/results/clientnegative/touch2.q.out 045121a
>
> Diff: https://reviews.apache.org/r/14970/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Szehon Ho
>
>
Re: Review Request 14970: HIVE-4723 DDLSemanticAnalyzer.addTablePartsOutputs
eats several exceptions
Posted by Brock Noland <br...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14970/#review27634
-----------------------------------------------------------
ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java
<https://reviews.apache.org/r/14970/#comment53685>
Is this what we want? I was assume we only wanted to log the exception at this point since it's not being logged.
- Brock Noland
On Oct. 28, 2013, 8:40 p.m., Szehon Ho wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14970/
> -----------------------------------------------------------
>
> (Updated Oct. 28, 2013, 8:40 p.m.)
>
>
> Review request for hive.
>
>
> Bugs: HIVE-4723
> https://issues.apache.org/jira/browse/HIVE-4723
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Throwing the offending exception, that was swallowed. Also changing the underlying exception to use the standard error message.
>
>
> Diffs
> -----
>
> ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java b0f124b
> ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java 037191a
> ql/src/test/results/clientnegative/alter_rename_partition_failure3.q.out 384fcbe
> ql/src/test/results/clientnegative/touch2.q.out 045121a
>
> Diff: https://reviews.apache.org/r/14970/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Szehon Ho
>
>
Re: Review Request 14970: HIVE-4723 DDLSemanticAnalyzer.addTablePartsOutputs
eats several exceptions
Posted by Szehon Ho <sz...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14970/
-----------------------------------------------------------
(Updated Oct. 28, 2013, 8:40 p.m.)
Review request for hive.
Changes
-------
There were two negative tests that expected the exception to be swallowed during semantic analysis, and for execution to continue and the DDL Task to fail. Changed the test output accordingly to only expect the exception during the semantic analysis and not proceed further.
Bugs: HIVE-4723
https://issues.apache.org/jira/browse/HIVE-4723
Repository: hive-git
Description
-------
Throwing the offending exception, that was swallowed. Also changing the underlying exception to use the standard error message.
Diffs (updated)
-----
ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java b0f124b
ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java 037191a
ql/src/test/results/clientnegative/alter_rename_partition_failure3.q.out 384fcbe
ql/src/test/results/clientnegative/touch2.q.out 045121a
Diff: https://reviews.apache.org/r/14970/diff/
Testing
-------
Thanks,
Szehon Ho