> 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()
> >
* 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.
- Rohini
-----------------------------------------------------------
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
>
>
