> On Nov. 3, 2014, 8:20 p.m., Rohini Palaniswamy wrote: > > core/src/main/java/org/apache/oozie/service/ShareLibService.java, line 604 > > <https://reviews.apache.org/r/27118/diff/2/?file=745099#file745099line604> > > > > 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
Incase of symlink, else will never execute because key will always prefix with oozie. > On Nov. 3, 2014, 8:20 p.m., Rohini Palaniswamy wrote: > > core/src/main/java/org/apache/oozie/service/ShareLibService.java, lines > > 761-762 > > <https://reviews.apache.org/r/27118/diff/2/?file=745099#file745099line761> > > > > null and empty check for values() not required. We don't want to populate empty list to instrumentation. > On Nov. 3, 2014, 8:20 p.m., Rohini Palaniswamy wrote: > > core/src/main/java/org/apache/oozie/service/ShareLibService.java, line 772 > > <https://reviews.apache.org/r/27118/diff/2/?file=745099#file745099line772> > > > > Have the code in try catch. Log any exception and return "Error > > retrieving symlink information". Not needed, we ate not verifying link. Just populating list from map. > On Nov. 3, 2014, 8:20 p.m., Rohini Palaniswamy wrote: > > core/src/main/java/org/apache/oozie/service/ShareLibService.java, lines > > 352-353 > > <https://reviews.apache.org/r/27118/diff/2/?file=745099#file745099line352> > > > > 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. Here is full code. synchronized (ShareLibService.class) { Map<String, List<Path>> tempShareLibMap = new HashMap<String, List<Path>>(shareLibMap); Map<String, Map<Path, Path>> tmpSymlinkMapping = new HashMap<String, Map<Path, Path>>(symlinkMapping); LOG.info(MessageFormat.format("Symlink target for [{0}] is changed, was [{1}], now [{2}]", shareLibKey, path, fileSystem.getSymLinkTarget(path))); loadShareLibMetaFile(tempShareLibMap, tmpSymlinkMapping, sharelibMappingFile, shareLibKey); shareLibMap = tempShareLibMap; symlinkMapping = tmpSymlinkMapping; return; } We are generating tmpSymlinkMapping from symlinkMapping and updating the particular key. > On Nov. 3, 2014, 8:20 p.m., Rohini Palaniswamy wrote: > > core/src/main/java/org/apache/oozie/service/ShareLibService.java, line 282 > > <https://reviews.apache.org/r/27118/diff/2/?file=745099#file745099line282> > > > > Doesn't fs.isFile(rootDir) work? Why is the wrapping into URI required? fs.isFile(rootDir) doesn't work with linkname. URI wrapping works for filename with linkname. I have added that code only for filename with linkname, otherwise fs.listStatus() works fine for filename. - Purshotam ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27118/#review59617 ----------------------------------------------------------- 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 > >
