> 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.
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 - Alejandro ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9327/#review16235 ----------------------------------------------------------- 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 > >
