----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/14180/#review26246 -----------------------------------------------------------
trunk/hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/JobIDParser.java <https://reviews.apache.org/r/14180/#comment51284> could this class have a JobIDParser(String statusdir, Configuration conf) c'tor so that subclasses can call super(statusdir, conf) and have both member vars private (final) ? trunk/hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/JobIDParser.java <https://reviews.apache.org/r/14180/#comment51311> openStatusFile() can be private trunk/hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/JobIDParser.java <https://reviews.apache.org/r/14180/#comment51312> this can be made private as well (see comment in TestJobIDParser) trunk/hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/JobIDParser.java <https://reviews.apache.org/r/14180/#comment51309> finally() ? trunk/hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/LogRetriever.java <https://reviews.apache.org/r/14180/#comment51298> THis needs some javadoc that explains the what the class is doing, at least the directory structure as explained in the HIVE-4531 description may also be useful to explain why it's parsing JSPs instead of calling some API, etc trunk/hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/LogRetriever.java <https://reviews.apache.org/r/14180/#comment51291> Would JavaDoc comments at method level be better than // ? trunk/hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/LogRetriever.java <https://reviews.apache.org/r/14180/#comment51286> This class is only used from TempletonControllerJob. Can it be moved to the same package and be made package scoped? trunk/hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/LogRetriever.java <https://reviews.apache.org/r/14180/#comment51288> can all 5 member variables be private? They are not used outside the class trunk/hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/LogRetriever.java <https://reviews.apache.org/r/14180/#comment51287> could this be static class (since it's basically a struct) trunk/hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/LogRetriever.java <https://reviews.apache.org/r/14180/#comment51300> could the valid values for status & type be 'enum's for readability? trunk/hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/LogRetriever.java <https://reviews.apache.org/r/14180/#comment51289> I'd include the actual value 'jobType' in the error msg - will make debugging easier trunk/hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/LogRetriever.java <https://reviews.apache.org/r/14180/#comment51294> finally { if(listWriter != null ) listWriter.close() } would be better trunk/hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/LogRetriever.java <https://reviews.apache.org/r/14180/#comment51296> if this exception is caught, the writer will be closed, but the loop will continue (and I assume fail trying to write to list.txt) trunk/hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/LogRetriever.java <https://reviews.apache.org/r/14180/#comment51295> it'd be clearer if 'job' was 'jobID' trunk/hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/LogRetriever.java <https://reviews.apache.org/r/14180/#comment51290> ArrayList<String> trunk/hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/LogRetriever.java <https://reviews.apache.org/r/14180/#comment51299> connection not closed trunk/hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/LogRetriever.java <https://reviews.apache.org/r/14180/#comment51293> 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 trunk/hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/LogRetriever.java <https://reviews.apache.org/r/14180/#comment51292> shouldn't the connection be closed? trunk/hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/LogRetriever.java <https://reviews.apache.org/r/14180/#comment51297> close() should be called from finally{} trunk/hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/LogRetriever.java <https://reviews.apache.org/r/14180/#comment51301> close connection trunk/hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/LogRetriever.java <https://reviews.apache.org/r/14180/#comment51302> finally{} ? trunk/hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/Server.java <https://reviews.apache.org/r/14180/#comment51303> javaDoc for 'enablelog' is missing trunk/hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/tool/TempletonControllerJob.java <https://reviews.apache.org/r/14180/#comment51304> is there a job_id or some other piece of data that could be added to this log message to indicate on behalf of which job this done? trunk/hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/tool/TempletonControllerJob.java <https://reviews.apache.org/r/14180/#comment51305> 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... trunk/hcatalog/webhcat/svr/src/test/java/org/apache/hive/hcatalog/templeton/TestJobIDParser.java <https://reviews.apache.org/r/14180/#comment51310> I'm still not quite clear why the tests can't be written like: @Test public void myTestPig() throws IOException { PigJobIDParser pigJobIDParser = new PigJobIDParser("../../src/test/data/status/pig/", new Configuration()); List<String> jobs = pigJobIDParser.parseJobID(); Assert.assertEquals("Expected 1 job, found: " + jobs.size(), jobs.size(), 1); } in which case JobIDParser#findJobID() can be private. - Eugene Koifman 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 > >
