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