hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Daniel Dai" <dai...@gmail.com>
Subject Re: Review Request 14180: HIVE-4531: [WebHCat] Collecting task logs to hdfs
Date Fri, 20 Sep 2013 21:49:46 GMT


> On Sept. 18, 2013, 11:49 p.m., Eugene Koifman wrote:
> > trunk/hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/LogRetriever.java,
line 129
> > <https://reviews.apache.org/r/14180/diff/1/?file=353367#file353367line129>
> >
> >     finally {
> >     if(listWriter != null ) listWriter.close()
> >     }
> >     would be better

Actually I shall not close the listWriter in this case.


> On Sept. 18, 2013, 11:49 p.m., Eugene Koifman wrote:
> > trunk/hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/LogRetriever.java,
line 199
> > <https://reviews.apache.org/r/14180/diff/1/?file=353367#file353367line199>
> >
> >     ArrayList<String>

Cannot create a generic array of ArrayList<String>


> On Sept. 18, 2013, 11:49 p.m., Eugene Koifman wrote:
> > trunk/hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/LogRetriever.java,
line 204
> > <https://reviews.apache.org/r/14180/diff/1/?file=353367#file353367line204>
> >
> >     connection not closed

There is no close method in URLConnection. Close the underlining inputstream should be enough
according to the docs.


> On Sept. 18, 2013, 11:49 p.m., Eugene Koifman wrote:
> > trunk/hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/LogRetriever.java,
line 227
> > <https://reviews.apache.org/r/14180/diff/1/?file=353367#file353367line227>
> >
> >     shouldn't the connection be closed?

See previous comment.


> On Sept. 18, 2013, 11:49 p.m., Eugene Koifman wrote:
> > trunk/hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/LogRetriever.java,
line 340
> > <https://reviews.apache.org/r/14180/diff/1/?file=353367#file353367line340>
> >
> >     close connection

See previous comment.


> On Sept. 18, 2013, 11:49 p.m., Eugene Koifman wrote:
> > trunk/hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/LogRetriever.java,
line 221
> > <https://reviews.apache.org/r/14180/diff/1/?file=353367#file353367line221>
> >
> >     it seems better that this method use a try/catch(IOException)/finally and handle
cleaning up resources here, rather than make every caller do this - all they do is write the
stack trace to System.err

This is consistent with other similar methods:getCompletedAttempts, getFailedAttempts, etc.
One awkward thing I don't like handling it in the method is: You will put method body in try/catch
block, and in finally, you need to close the file within another try/catch block:
finally {
    if (writer!=null) {
    try {
        writer.close();
    catch (IOException e) {
    }
}


> On Sept. 18, 2013, 11:49 p.m., Eugene Koifman wrote:
> > trunk/hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/tool/TempletonControllerJob.java,
line 294
> > <https://reviews.apache.org/r/14180/diff/1/?file=353372#file353372line294>
> >
> >     why is this necessary?  
> >     There are 2 Watchers created in separate threads in this class.  Both use System.err
to log error messages.  If one closes 'err' while the other gets an error right after - it
will be a problem.  I think this writer.close() creates a race condition...

I don't realize there are two watcher. I don't remember why I add that, probably because one
issue I see. I am fine to remove it and keep a close eye on that for a while.


- Daniel


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14180/#review26246
-----------------------------------------------------------


On Sept. 18, 2013, 3:20 p.m., Daniel Dai wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14180/
> -----------------------------------------------------------
> 
> (Updated Sept. 18, 2013, 3:20 p.m.)
> 
> 
> Review request for hive.
> 
> 
> Bugs: HIVE-4531
>     https://issues.apache.org/jira/browse/HIVE-4531
> 
> 
> Repository: hive
> 
> 
> Description
> -------
> 
> SEE HIVE-4531.
> 
> 
> Diffs
> -----
> 
>   trunk/hcatalog/src/docs/src/documentation/content/xdocs/hive.xml 1524447 
>   trunk/hcatalog/src/docs/src/documentation/content/xdocs/mapreducejar.xml 1524447 
>   trunk/hcatalog/src/docs/src/documentation/content/xdocs/mapreducestreaming.xml 1524447

>   trunk/hcatalog/src/docs/src/documentation/content/xdocs/pig.xml 1524447 
>   trunk/hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/HiveDelegator.java
1524447 
>   trunk/hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/HiveJobIDParser.java
PRE-CREATION 
>   trunk/hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/JarDelegator.java
1524447 
>   trunk/hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/JarJobIDParser.java
PRE-CREATION 
>   trunk/hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/JobIDParser.java
PRE-CREATION 
>   trunk/hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/LauncherDelegator.java
1524447 
>   trunk/hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/LogRetriever.java
PRE-CREATION 
>   trunk/hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/PigDelegator.java
1524447 
>   trunk/hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/PigJobIDParser.java
PRE-CREATION 
>   trunk/hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/Server.java
1524447 
>   trunk/hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/StreamingDelegator.java
1524447 
>   trunk/hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/tool/TempletonControllerJob.java
1524447 
>   trunk/hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/tool/TempletonUtils.java
1524447 
>   trunk/hcatalog/webhcat/svr/src/test/data/status/hive/stderr PRE-CREATION 
>   trunk/hcatalog/webhcat/svr/src/test/data/status/jar/stderr PRE-CREATION 
>   trunk/hcatalog/webhcat/svr/src/test/data/status/pig/stderr PRE-CREATION 
>   trunk/hcatalog/webhcat/svr/src/test/data/status/streaming/stderr PRE-CREATION 
>   trunk/hcatalog/webhcat/svr/src/test/java/org/apache/hive/hcatalog/templeton/TestJobIDParser.java
PRE-CREATION 
>   trunk/hcatalog/webhcat/svr/src/test/java/org/apache/hive/hcatalog/templeton/tool/TestTempletonUtils.java
1524447 
> 
> Diff: https://reviews.apache.org/r/14180/diff/
> 
> 
> Testing
> -------
> 
> WebHCat unit tests
> e2e tests in HIVE-5078 under both Linux/Windows
> 
> 
> Thanks,
> 
> Daniel Dai
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message