> On Feb. 7, 2013, 1:14 a.m., Alejandro Abdelnur wrote: > > 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() > > > > Rohini Palaniswamy wrote: > * HCATLauncherURIHandler why creation of partition is not supported? > - It does not logically make sense. Unlike mkdir, when you add a > partition, the data needs to be there. So it does not make sense to have > create for hcat in prepare block. > * 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. > - Thread safety does not matter. It will be the same value that will > get overwritten. The dependency type for a table does not change. caching is > just for performance. > * 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? > - We did ask for an API to get the JMS information. But hcat team was > not able to provide it. That is the reason we had to do the mapping as a > configuration in oozie. > * 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. > - When we get the dependency type we do not have the full uri (only > uri template with unsubstituted variables). So can't get the topic name based > on the db and table name. So we decide based on the availability of jms > mapping. Anyways there is always the fallback of direct hcat check, even if > JMS notifications are not happening. Current hcat implementation sends > notifications for all tables if JMS is configured. So we can assume for now, > that if the hcat server is publishing notifications, all tables in that > server have topics. > * 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. > - How is a race condition possible with the putIfAbsent logic? It > will always be one UGI returned for a user at any time. > * HCatAccessorService, initializeMappingRules(), why not do directly > String[] connections = conf.getStrings(JMS_CONNECTIONS_PROPERTIES)? > - I guess that is because it does not work if the configuration > spans multiple lines or has white spaces. Just to allow better readability > since the jndi connect strings are going to be long. > * 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. > - We can do a guava based cache later if there are issues with this. > I am not seriously worried about race conditions as the fallback of direct > hcat checks will take care of anything missed. > * URIHandlerService, getAuthorityWithScheme() why not just do a new > URI(uri) and then extract getScheme() + "://" + getAuthority() > - This is used when we work on the uri templates which has > unsubstituted variables to determine the type of the dependency. new URI() > will throw syntax error. So need to remove the path manually before doing new > URI(). > > Will address the rest. > > Alejandro Abdelnur wrote: > on ' Thread safety does not matter....', actually it does, if you are > putting a value and somebody else is ready another value things will break. > We need to make it thread safe. > > on ' When we get the dependency...', got it, but if the topic does not > exist we should throw an exception instead continuing silently. > > on 'How is a race condition possible...', the get and putIfAbsent are 2 > diff methods, you could have 2 threads doing the same in an interleave way. > the Guava Cache get() avoids that having a single method, get() that performs > both. > > on 'I guess that is because it does not work if the config spans ...', > actually it does work, we do that in multiple places already, ie parsing the > ELService definitions. > > on 'WE can do a guava base cache later...', same reason on my 3rd comment > here. This must be fixed, or the cache remove completely. > > thx
Convinced Tucu that there are no thread safety issues with the patch and the logic takes care or all race conditions. - Rohini ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9327/#review16235 ----------------------------------------------------------- On Feb. 7, 2013, 4:03 p.m., Rohini Palaniswamy wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/9327/ > ----------------------------------------------------------- > > (Updated Feb. 7, 2013, 4:03 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 > 1443061 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/action/hadoop/LauncherException.java > PRE-CREATION > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/action/hadoop/LauncherMapper.java > 1443061 > > 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 > 1443061 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/client/rest/JsonCoordinatorJob.java > 1443061 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/command/coord/CoordActionInputCheckXCommand.java > 1443061 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/command/coord/CoordCommandUtils.java > 1443061 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/command/coord/CoordPushDependencyCheckXCommand.java > 1443061 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/coord/CoordELFunctions.java > 1443061 > > 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 > 1443061 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/dependency/DependencyType.java > 1443061 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/dependency/FSURIContext.java > 1443061 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/dependency/FSURIHandler.java > 1443061 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/dependency/HCatURIContext.java > 1443061 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/dependency/HCatURIHandler.java > 1443061 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/dependency/URIContext.java > 1443061 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/dependency/URIHandler.java > 1443061 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/dependency/URIHandlerException.java > PRE-CREATION > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/dependency/cache/SimpleHCatDependencyCache.java > 1443061 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/jms/ConnectionContext.java > 1443061 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/jms/DefaultConnectionContext.java > 1443061 > > 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 > 1443061 > > 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/HadoopAccessorException.java > 1443061 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/service/HadoopAccessorService.java > 1443061 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/service/JMSAccessorService.java > 1443061 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/service/MetaDataAccessorException.java > 1443061 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/service/PartitionDependencyManagerService.java > 1443061 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/service/RecoveryService.java > 1443061 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/service/URIAccessorException.java > 1443061 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/service/URIHandlerService.java > 1443061 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/service/UserGroupInformationService.java > 1443061 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/util/XLog.java > 1443061 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/resources/oozie-default.xml > 1443061 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/test/java/org/apache/oozie/action/hadoop/TestFSPrepareActions.java > 1443061 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/test/java/org/apache/oozie/action/hadoop/TestHCatPrepareActions.java > 1443061 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java > 1443061 > > 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 > 1443061 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/test/java/org/apache/oozie/action/hadoop/TestMapReduceActionExecutor.java > 1443061 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/test/java/org/apache/oozie/action/hadoop/TestPrepareActionsDriver.java > 1443061 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/test/java/org/apache/oozie/action/hadoop/TestShellActionExecutor.java > 1443061 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/test/java/org/apache/oozie/command/coord/TestCoordPushDependencyCheckXCommand.java > 1443061 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/test/java/org/apache/oozie/dependency/TestFSURIHandler.java > 1443061 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/test/java/org/apache/oozie/dependency/TestHCatURIHandler.java > 1443061 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/test/java/org/apache/oozie/dependency/TestURIHandlerService.java > 1443061 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/test/java/org/apache/oozie/jms/TestHCatMessageHandler.java > 1443061 > > 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 > 1443061 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/test/java/org/apache/oozie/service/TestRecoveryService.java > 1443061 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/test/java/org/apache/oozie/test/XTestCase.java > 1443061 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/tests/pig/src/test/java/org/apache/oozie/action/hadoop/TestPigActionExecutor.java > 1443061 > > Diff: https://reviews.apache.org/r/9327/diff/ > > > Testing > ------- > > > Thanks, > > Rohini Palaniswamy > >
