----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/14274/#review26342 -----------------------------------------------------------
trunk/src/org/apache/pig/PigConstants.java <https://reviews.apache.org/r/14274/#comment51440> Would you mind replacing all the "pig.temp.dir" strings in the code base? I see 4 more matches besides what you're substituting in this patch. TestAvroStorage.java 197: pigServerLocal.getPigContext().getProperties().setProperty("pig.temp.dir", TMP_DIR); PigConstants.java 53: public static final String PIG_TEMP_DIR = "pig.temp.dir"; TestMultiQueryLocal.java 73: myPig.getPigContext().getProperties().setProperty("pig.temp.dir", "build/test/tmp/"); TestPigServer.java 731: out.println("pig.temp.dir=/opt/temp"); trunk/src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/JobControlCompiler.java <https://reviews.apache.org/r/14274/#comment51434> Perhaps use this utility function instead: http://commons.apache.org/proper/commons-io/apidocs/org/apache/commons/io/FilenameUtils.html#getName(java.lang.String) trunk/src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/JobControlCompiler.java <https://reviews.apache.org/r/14274/#comment51431> Why not return immediately when jarPath is found and remove the boolean flag? i.e. for (FileStatus stat : statuses) { jarPath = stat.getPath(); if(jarPath.getName().equals(filename)) { return jarPath; } } ... trunk/src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/JobControlCompiler.java <https://reviews.apache.org/r/14274/#comment51433> Can you move this line to after the if block, or replace the pathOnDfs? trunk/src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/JobControlCompiler.java <https://reviews.apache.org/r/14274/#comment51432> Perhaps use this utility function instead: http://commons.apache.org/proper/commons-io/apidocs/org/apache/commons/io/FilenameUtils.html#getName(java.lang.String) trunk/src/org/apache/pig/impl/PigContext.java <https://reviews.apache.org/r/14274/#comment51436> Looks like we can simply use Set<String> instead of Map<Sting, URL> for extraJarOriginalPaths, no? trunk/test/org/apache/pig/test/TestJobControlCompiler.java <https://reviews.apache.org/r/14274/#comment51437> Delete? trunk/test/org/apache/pig/test/TestJobControlCompiler.java <https://reviews.apache.org/r/14274/#comment51438> Not used. Delete? trunk/test/org/apache/pig/test/TestJobControlCompiler.java <https://reviews.apache.org/r/14274/#comment51439> The following line is missing in the RB diff but it's in the attached the patch: properties.setProperty(PigConstants.PIG_SHARED_CACHE_ENABLED_KEY, "true"); Just pointing it out. - Cheolsoo Park On Sept. 21, 2013, 1:21 a.m., Aniket Mokashi wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/14274/ > ----------------------------------------------------------- > > (Updated Sept. 21, 2013, 1:21 a.m.) > > > Review request for pig, Cheolsoo Park, DanielWX DanielWX, Dmitriy Ryaboy, > Julien Le Dem, and Rohini Palaniswamy. > > > Bugs: PIG-2672 > https://issues.apache.org/jira/browse/PIG-2672 > > > Repository: pig > > > Description > ------- > > added jar.cache.location option > > > Diffs > ----- > > trunk/src/org/apache/pig/PigConstants.java 1525188 > > trunk/src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/JobControlCompiler.java > 1525188 > trunk/src/org/apache/pig/impl/PigContext.java 1525188 > trunk/src/org/apache/pig/impl/io/FileLocalizer.java 1525188 > trunk/test/org/apache/pig/test/TestJobControlCompiler.java 1525188 > > Diff: https://reviews.apache.org/r/14274/diff/ > > > Testing > ------- > > > Thanks, > > Aniket Mokashi > >
