Home | About | Sematext search-lucene.com search-hadoop.com
 Search Hadoop and all its subprojects:

Switch to Threaded View
Hive >> mail # dev >> Review Request 14180: HIVE-4531:  [WebHCat] Collecting task logs to hdfs


Copy link to this message
-
Re: Review Request 14180: HIVE-4531: [WebHCat] Collecting task logs to hdfs


> 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
>