----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27118/#review59617 -----------------------------------------------------------
core/src/main/java/org/apache/oozie/service/ShareLibService.java <https://reviews.apache.org/r/27118/#comment100914> Does not throw URISyntaxException core/src/main/java/org/apache/oozie/service/ShareLibService.java <https://reviews.apache.org/r/27118/#comment100917> Doesn't fs.isFile(rootDir) work? Why is the wrapping into URI required? core/src/main/java/org/apache/oozie/service/ShareLibService.java <https://reviews.apache.org/r/27118/#comment100919> %s/symlink/Symlink/g has changed core/src/main/java/org/apache/oozie/service/ShareLibService.java <https://reviews.apache.org/r/27118/#comment100920> You need to update the particular key for sharelib name on the old map with value from new maps. You can't assign the new maps as they contain only value for the particular sharelib. core/src/main/java/org/apache/oozie/service/ShareLibService.java <https://reviews.apache.org/r/27118/#comment100935> tmpShareLibMap core/src/main/java/org/apache/oozie/service/ShareLibService.java <https://reviews.apache.org/r/27118/#comment100926> 1) Either sharelibKey or sharelibName for the map key in all the places in code. Now there is a mix of actionKey, sharelibName and sharelibKey. 2) Method javadoc also needs to include sharelibKey and need to mention only that particular sharelib will be loaded. core/src/main/java/org/apache/oozie/service/ShareLibService.java <https://reviews.apache.org/r/27118/#comment100936> Can we pass ..,key.substring(SHARE_LIB_CONF_PREFIX.length() + 1), prop.getProperty(key).split(",")) instead of prop to the method changing the method signature to ...String sharelibKey, String[] sharelibPaths)? Currently the new method assumes knowledge of internals of meta file. Would be cleaner to encapsulate that within this method. core/src/main/java/org/apache/oozie/service/ShareLibService.java <https://reviews.apache.org/r/27118/#comment100933> Can remove else block and just add "Loading sharelib for " + key message to if block. Else there will be lot of log spam when symlinks are updated core/src/main/java/org/apache/oozie/service/ShareLibService.java <https://reviews.apache.org/r/27118/#comment100934> Symlink core/src/main/java/org/apache/oozie/service/ShareLibService.java <https://reviews.apache.org/r/27118/#comment100929> null and empty check for values() not required. core/src/main/java/org/apache/oozie/service/ShareLibService.java <https://reviews.apache.org/r/27118/#comment100931> 1) Use entryset instead of keyset. 2) Null and empty checks not required. core/src/main/java/org/apache/oozie/service/ShareLibService.java <https://reviews.apache.org/r/27118/#comment100932> Have the code in try catch. Log any exception and return "Error retrieving symlink information". core/src/test/java/org/apache/oozie/service/TestShareLibService.java <https://reviews.apache.org/r/27118/#comment100922> formatting core/src/test/java/org/apache/oozie/service/TestShareLibService.java <https://reviews.apache.org/r/27118/#comment100925> createTestShareLibMetaFile - Rohini Palaniswamy On Oct. 31, 2014, 12:19 a.m., Purshotam Shah wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27118/ > ----------------------------------------------------------- > > (Updated Oct. 31, 2014, 12:19 a.m.) > > > Review request for oozie. > > > Bugs: OOZIE-2045 > https://issues.apache.org/jira/browse/OOZIE-2045 > > > Repository: oozie-git > > > Description > ------- > > OOZIE-2045 Symlink support for sharelib > > > Diffs > ----- > > core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java > 201cfa3 > core/src/main/java/org/apache/oozie/service/ShareLibService.java 63c5a41 > core/src/main/resources/oozie-default.xml 26eb7e0 > core/src/test/java/org/apache/oozie/service/TestShareLibService.java > 68cb357 > > hadooplibs/hadoop-utils-0.23/src/main/java/org/apache/oozie/hadoop/utils/HadoopShims.java > e69de29 > > hadooplibs/hadoop-utils-1/src/main/java/org/apache/oozie/hadoop/utils/HadoopShims.java > e69de29 > > hadooplibs/hadoop-utils-2/src/main/java/org/apache/oozie/hadoop/utils/HadoopShims.java > e69de29 > > hadooplibs/hadoop-utils-3/src/main/java/org/apache/oozie/hadoop/utils/HadoopShims.java > e69de29 > > Diff: https://reviews.apache.org/r/27118/diff/ > > > Testing > ------- > > UTC and manul testing > > > Thanks, > > Purshotam Shah > >
