----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9327/#review16234 -----------------------------------------------------------
Second round: * URIAccessorException should be URIHandlerException. * URIAccessorException javadocs are incorrect, mention MetaDataAccessor exception. * LauncherURIHandler javadocs @throws use incorrect exception name * LauncherURIHandler getLauncherClassesToShip() rename to getClassesForLauncher() * We should make LauncherException a top level class as we are using it outside of LauncherMapper * HCATLauncherURIHandler why creation of partition is not supported? * FSURIHandler, classesToShip should be initialized in the init() * HCatURIHandler, classesToShip should be initialized in the init() * HCatURIHandler, getDependencyType(), registerForNotification() , getMetatStoreConnection() have multiple returns, they should have one. * HCatURIHandler, getDependencyType() modifies dependencyType which is a HashMap, that is not thread safe. Do we need do that optimization of caching things here, I'd say we always go to the hcatservice if avail. * HCatURIHandler, registerForNotification(), I guess this is more a HCAT question, but why the HCatClient.getMessageBusTopicName() method does not return the full JMS information including HOST:PORT/TOPIC, this would remove the need for the mapping, no? * HCatURIHandler, registerForNotification(), if there is no topic, shouldn't be the correct thing to fallback to PULL for this dependency? This means that the hcatservicie.registerForNotification() call would have to be done in the getDependencyType() in order to do the fallback if needed. * UserGroupInformationService, it seems the Guava Cache get() method would be a much better fit for this usecase as the getProxyUser() is still subject to a race condition. * ActionDependency, why the 2 lists are not of the same type? either URI or String? * ConnectionContext, why expose the getConnection() ? the getConnectionFactoryName() should not be part of the interface, this is an impl detail. * HCatAccessorService, initializeMappingRules(), why not do directly String[] connections = conf.getStrings(JMS_CONNECTIONS_PROPERTIES)? * HCatAccessorService is doing 4 times the creation of the key, why not have a method that does that once * JMSAccessorService, all the caching/reconnect logic, it is a bit complex, I'll get back to this one later. * SimpleHCatDepedencyCache, the more I look at this, the more I think using Guava cache get() will simplify the whole logic and will eliminate all race conditions. * URIHandlerService, getAuthorityWithScheme() why not just do a new URI(uri) and then extract getScheme() + "://" + getAuthority() - Alejandro Abdelnur On Feb. 6, 2013, 2:06 p.m., Rohini Palaniswamy wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/9327/ > ----------------------------------------------------------- > > (Updated Feb. 6, 2013, 2:06 p.m.) > > > Review request for oozie and Alejandro Abdelnur. > > > Description > ------- > > Addresses major chunk of review comments from OOZIE-1200. > > > This addresses bug OOZIE-1210. > https://issues.apache.org/jira/browse/OOZIE-1210 > > > Diffs > ----- > > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/action/hadoop/FSLauncherURIHandler.java > PRE-CREATION > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/action/hadoop/HCatLauncherURIHandler.java > PRE-CREATION > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java > 1442199 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/action/hadoop/LauncherMapper.java > 1442199 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/action/hadoop/LauncherURIHandler.java > PRE-CREATION > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/action/hadoop/LauncherURIHandlerFactory.java > PRE-CREATION > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/action/hadoop/PrepareActionsDriver.java > 1442199 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/client/rest/JsonCoordinatorJob.java > 1442199 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/command/coord/CoordActionInputCheckXCommand.java > 1442199 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/command/coord/CoordCommandUtils.java > 1442199 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/command/coord/CoordPushDependencyCheckXCommand.java > 1442199 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/coord/CoordELFunctions.java > 1442199 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/dependency/ActionDependency.java > PRE-CREATION > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/dependency/DependencyChecker.java > 1442199 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/dependency/DependencyType.java > 1442199 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/dependency/FSURIContext.java > 1442199 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/dependency/FSURIHandler.java > 1442199 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/dependency/HCatURIContext.java > 1442199 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/dependency/HCatURIHandler.java > 1442199 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/dependency/URIContext.java > 1442199 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/dependency/URIHandler.java > 1442199 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/dependency/cache/SimpleHCatDependencyCache.java > 1442199 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/jms/ConnectionContext.java > 1442199 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/jms/DefaultConnectionContext.java > 1442199 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/jms/JMSConnectionInfo.java > PRE-CREATION > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/jms/JMSExceptionListener.java > 1442199 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/service/HCatAccessorService.java > PRE-CREATION > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/service/HadoopAccessorService.java > 1442199 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/service/JMSAccessorService.java > 1442199 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/service/PartitionDependencyManagerService.java > 1442199 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/service/RecoveryService.java > 1442199 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/service/URIHandlerService.java > 1442199 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/service/UserGroupInformationService.java > 1442199 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/util/XLog.java > 1442199 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/resources/oozie-default.xml > 1442199 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/test/java/org/apache/oozie/action/hadoop/TestFSPrepareActions.java > 1442199 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/test/java/org/apache/oozie/action/hadoop/TestHCatPrepareActions.java > 1442199 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java > 1442199 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/test/java/org/apache/oozie/action/hadoop/TestLauncherFSURIHandler.java > PRE-CREATION > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/test/java/org/apache/oozie/action/hadoop/TestLauncherHCatURIHandler.java > PRE-CREATION > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/test/java/org/apache/oozie/action/hadoop/TestMapReduceActionError.java > 1442199 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/test/java/org/apache/oozie/action/hadoop/TestMapReduceActionExecutor.java > 1442199 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/test/java/org/apache/oozie/action/hadoop/TestPrepareActionsDriver.java > 1442199 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/test/java/org/apache/oozie/action/hadoop/TestShellActionExecutor.java > 1442199 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/test/java/org/apache/oozie/command/coord/TestCoordPushDependencyCheckXCommand.java > 1442199 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/test/java/org/apache/oozie/dependency/TestFSURIHandler.java > 1442199 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/test/java/org/apache/oozie/dependency/TestHCatURIHandler.java > 1442199 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/test/java/org/apache/oozie/jms/TestHCatMessageHandler.java > 1442199 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/test/java/org/apache/oozie/service/TestHCatAccessorService.java > PRE-CREATION > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/test/java/org/apache/oozie/service/TestJMSAccessorService.java > 1442199 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/test/java/org/apache/oozie/service/TestRecoveryService.java > 1442199 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/test/java/org/apache/oozie/test/XTestCase.java > 1442199 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/tests/pig/src/test/java/org/apache/oozie/action/hadoop/TestPigActionExecutor.java > 1442199 > > Diff: https://reviews.apache.org/r/9327/diff/ > > > Testing > ------- > > > Thanks, > > Rohini Palaniswamy > >
