----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18754/#review43416 -----------------------------------------------------------
On scale with lot of missing dependencies, this code will have serious problems. Iterating on all missing dependencies is going to take a long time and synchronized can block other calls. You need another data structure (reverse lookup map) which quickly gives the keys for a action id that you need to pick so that this method executes really quickly and does not hold other threads. core/src/main/java/org/apache/oozie/dependency/hcat/EhcacheHCatDependencyCache.java <https://reviews.apache.org/r/18754/#comment77535> Not required core/src/main/java/org/apache/oozie/dependency/hcat/EhcacheHCatDependencyCache.java <https://reviews.apache.org/r/18754/#comment77545> hcatURI will not be null as the if block already checks for it. You can remove the check core/src/main/java/org/apache/oozie/dependency/hcat/HCatDependencyCache.java <https://reviews.apache.org/r/18754/#comment77522> CoordActions. Can we be more specific and call it removeNonWaitingCoordActions? core/src/main/java/org/apache/oozie/dependency/hcat/SimpleHCatDependencyCache.java <https://reviews.apache.org/r/18754/#comment77549> synchronized(partKeyPatterns) core/src/main/java/org/apache/oozie/dependency/hcat/SimpleHCatDependencyCache.java <https://reviews.apache.org/r/18754/#comment77550> not required core/src/main/java/org/apache/oozie/dependency/hcat/SimpleHCatDependencyCache.java <https://reviews.apache.org/r/18754/#comment77551> Move to beginning of method core/src/main/java/org/apache/oozie/service/PartitionDependencyManagerService.java <https://reviews.apache.org/r/18754/#comment77530> Please get rid of this setting and add a new method to JobsConcurrencyService - isHighlyAvailableMode() which returns false for the single mode and true for ZKJobsConcurrencyService. It should be automatically turned on HA and turned off for non-HA core/src/main/java/org/apache/oozie/service/PartitionDependencyManagerService.java <https://reviews.apache.org/r/18754/#comment77531> purgeStaleMissingDependencies Method should be private and should be part of CachePurgeWorker inner class. core/src/test/java/org/apache/oozie/service/TestHAPartitionDependencyManagerService.java <https://reviews.apache.org/r/18754/#comment77538> Why wait for 30 seconds? - Rohini Palaniswamy On May 19, 2014, 5:07 a.m., Ryota Egashira wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/18754/ > ----------------------------------------------------------- > > (Updated May 19, 2014, 5:07 a.m.) > > > Review request for oozie. > > > Bugs: OOZIE-1492 > https://issues.apache.org/jira/browse/OOZIE-1492 > > > Repository: oozie-git > > > Description > ------- > > https://issues.apache.org/jira/browse/OOZIE-1492 > > > Diffs > ----- > > core/src/main/java/org/apache/oozie/CoordinatorActionBean.java 43d5103 > > core/src/main/java/org/apache/oozie/command/coord/CoordPushDependencyCheckXCommand.java > 2e5cd47 > > core/src/main/java/org/apache/oozie/dependency/hcat/EhcacheHCatDependencyCache.java > 6f127c4 > > core/src/main/java/org/apache/oozie/dependency/hcat/HCatDependencyCache.java > df3afd3 > > core/src/main/java/org/apache/oozie/dependency/hcat/SimpleHCatDependencyCache.java > e8e3ebc > > core/src/main/java/org/apache/oozie/executor/jpa/CoordActionQueryExecutor.java > f5304ca > > core/src/main/java/org/apache/oozie/service/PartitionDependencyManagerService.java > 985dcab > > core/src/test/java/org/apache/oozie/command/coord/TestCoordPushDependencyCheckXCommand.java > da09727 > > core/src/test/java/org/apache/oozie/service/TestHAPartitionDependencyManagerService.java > PRE-CREATION > > core/src/test/java/org/apache/oozie/service/TestPartitionDependencyManagerEhcache.java > cfdfbd1 > > core/src/test/java/org/apache/oozie/service/TestPartitionDependencyManagerService.java > ef71fb0 > > Diff: https://reviews.apache.org/r/18754/diff/ > > > Testing > ------- > > > Thanks, > > Ryota Egashira > >
