You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Kanna Karanam <ka...@microsoft.com> on 2012/07/05 21:50:40 UTC
Review Request: Resource Leak: Fix the File handle leak in EximUtil.java
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5777/
-----------------------------------------------------------
Review request for hive, Carl Steinbach, Edward Capriolo, and Ashutosh Chauhan.
Description
-------
1) Not closing the file handle EximUtil after reading the metadata from the file.
2) Nit: Get the path from URI to handle the Windows paths.
This addresses bug HIVE-3232.
https://issues.apache.org/jira/browse/HIVE-3232
Diffs
-----
http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/EximUtil.java 1357818
Diff: https://reviews.apache.org/r/5777/diff/
Testing
-------
Yes
Thanks,
Kanna Karanam
Re: Review Request: Resource Leak: Fix the File handle leak in EximUtil.java
Posted by Ashutosh Chauhan <ha...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5777/#review9021
-----------------------------------------------------------
Ship it!
Ship It!
http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/EximUtil.java
<https://reviews.apache.org/r/5777/#comment19184>
path here is used to construct URI object later. URI constructor javadocs says that path should either begin with '/' or should be empty, irrespective of OS. So, looks like check for path.startsWith('/') makes most sense here. As Kanna noted his changes make path to /D:/hive/etc from D:/hive/etc thus making URI constructor happy.
- Ashutosh Chauhan
On July 5, 2012, 7:50 p.m., Kanna Karanam wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5777/
> -----------------------------------------------------------
>
> (Updated July 5, 2012, 7:50 p.m.)
>
>
> Review request for hive, Carl Steinbach, Edward Capriolo, and Ashutosh Chauhan.
>
>
> Description
> -------
>
> 1) Not closing the file handle EximUtil after reading the metadata from the file.
> 2) Nit: Get the path from URI to handle the Windows paths.
>
>
> This addresses bug HIVE-3232.
> https://issues.apache.org/jira/browse/HIVE-3232
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/EximUtil.java 1357818
>
> Diff: https://reviews.apache.org/r/5777/diff/
>
>
> Testing
> -------
>
> Yes
>
>
> Thanks,
>
> Kanna Karanam
>
>
Re: Review Request: Resource Leak: Fix the File handle leak in EximUtil.java
Posted by Kanna Karanam <ka...@microsoft.com>.
> On July 5, 2012, 8:12 p.m., Carl Steinbach wrote:
> >
Thanks Carl. Please find the comments below. I tested these changes on Unix as well as on Windows and exim unit tests are passing.
> On July 5, 2012, 8:12 p.m., Carl Steinbach wrote:
> > http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/EximUtil.java, line 80
> > <https://reviews.apache.org/r/5777/diff/1/?file=119652#file119652line80>
> >
> > This check is looking for non-absolute paths, but on Windows it will always evaluate to true. I think this check should be replaced with a call to Path.isAbsolute().
I verified it on windows. The absolute path on Windows is something like "D:\OSS\Hive\etc". It detects this path and converts it into "/D:/OSS/Hive/etc" and allows us to create a valid URI.
In this case - File.IsAbsolute() returns true always.
> On July 5, 2012, 8:12 p.m., Carl Steinbach wrote:
> > http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/EximUtil.java, line 146
> > <https://reviews.apache.org/r/5777/diff/1/?file=119652#file119652line146>
> >
> > This check is looking for non-absolute paths, but on Windows it will always evaluate to true. I think this check should be replaced with a call to Path.isAbsolute().
I verified it on windows. The absolute path on Windows is something like "D:\OSS\Hive\etc". It detects this path and converts it into "/D:/OSS/Hive/etc" and allows us to create a valid URI.
In this case - File.IsAbsolute() returns true always.
> On July 5, 2012, 8:12 p.m., Carl Steinbach wrote:
> > http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/EximUtil.java, line 147
> > <https://reviews.apache.org/r/5777/diff/1/?file=119652#file119652line147>
> >
> > This logic is no-op unless we're running in testmode, which makes me worry that this may still be broken on windows if you run it by hand. Can you please check on this? Thanks.
I verified it on windows too. It is working.
- Kanna
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5777/#review8890
-----------------------------------------------------------
On July 5, 2012, 7:50 p.m., Kanna Karanam wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5777/
> -----------------------------------------------------------
>
> (Updated July 5, 2012, 7:50 p.m.)
>
>
> Review request for hive, Carl Steinbach, Edward Capriolo, and Ashutosh Chauhan.
>
>
> Description
> -------
>
> 1) Not closing the file handle EximUtil after reading the metadata from the file.
> 2) Nit: Get the path from URI to handle the Windows paths.
>
>
> This addresses bug HIVE-3232.
> https://issues.apache.org/jira/browse/HIVE-3232
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/EximUtil.java 1357818
>
> Diff: https://reviews.apache.org/r/5777/diff/
>
>
> Testing
> -------
>
> Yes
>
>
> Thanks,
>
> Kanna Karanam
>
>
Re: Review Request: Resource Leak: Fix the File handle leak in EximUtil.java
Posted by Carl Steinbach <ca...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5777/#review8890
-----------------------------------------------------------
http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/EximUtil.java
<https://reviews.apache.org/r/5777/#comment18935>
This check is looking for non-absolute paths, but on Windows it will always evaluate to true. I think this check should be replaced with a call to Path.isAbsolute().
http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/EximUtil.java
<https://reviews.apache.org/r/5777/#comment18933>
This check is looking for non-absolute paths, but on Windows it will always evaluate to true. I think this check should be replaced with a call to Path.isAbsolute().
http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/EximUtil.java
<https://reviews.apache.org/r/5777/#comment18934>
This logic is no-op unless we're running in testmode, which makes me worry that this may still be broken on windows if you run it by hand. Can you please check on this? Thanks.
- Carl Steinbach
On July 5, 2012, 7:50 p.m., Kanna Karanam wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5777/
> -----------------------------------------------------------
>
> (Updated July 5, 2012, 7:50 p.m.)
>
>
> Review request for hive, Carl Steinbach, Edward Capriolo, and Ashutosh Chauhan.
>
>
> Description
> -------
>
> 1) Not closing the file handle EximUtil after reading the metadata from the file.
> 2) Nit: Get the path from URI to handle the Windows paths.
>
>
> This addresses bug HIVE-3232.
> https://issues.apache.org/jira/browse/HIVE-3232
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/EximUtil.java 1357818
>
> Diff: https://reviews.apache.org/r/5777/diff/
>
>
> Testing
> -------
>
> Yes
>
>
> Thanks,
>
> Kanna Karanam
>
>