----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38474/#review112235 -----------------------------------------------------------
core/src/main/java/org/apache/oozie/command/coord/CoordCommandUtils.java (line 78) <https://reviews.apache.org/r/38474/#comment172613> RESOLVED_UNRESOLVED_SEPARATOR. Same as before as it is used to separate resolved and unresolved(latest/future) instances. Looking at places in code where it is used to separate unresolved instances there could be some issue or backward incompatibility with it. Earlier unresolved_separator were separated by CoordELFunctions.INSTANCE_SEPARATOR similar to resolved instances. core/src/main/java/org/apache/oozie/command/coord/CoordCommandUtils.java (line 575) <https://reviews.apache.org/r/38474/#comment172604> actionBean.getPushInputDependencies().getMissingDependencies() can be a local variable instead of calling twice. It does string operations. core/src/main/java/org/apache/oozie/command/coord/CoordCommandUtils.java (line 576) <https://reviews.apache.org/r/38474/#comment172603> Redundant. Can assign directly core/src/main/java/org/apache/oozie/command/coord/CoordCommandUtils.java (lines 703 - 705) <https://reviews.apache.org/r/38474/#comment172610> for loop can be outside of if-else block core/src/main/java/org/apache/oozie/coord/CoordELFunctions.java (line 375) <https://reviews.apache.org/r/38474/#comment172779> This should be either !isResolved or eval.getVariable(CoordELConstants.RESOLVED_PATH) == null if you want to store partial paths when it is not resolved fully for min processing core/src/main/java/org/apache/oozie/coord/CoordELFunctions.java (line 513) <https://reviews.apache.org/r/38474/#comment172594> Remove line core/src/main/java/org/apache/oozie/coord/input/dependency/CoordInputDependenciesBuilder.java (line 45) <https://reviews.apache.org/r/38474/#comment172758> input -> dataset for both method and param core/src/main/java/org/apache/oozie/coord/input/dependency/CoordInputDependenciesBuilder.java (line 56) <https://reviews.apache.org/r/38474/#comment172760> combine -> combineDatasets for both method and param core/src/main/java/org/apache/oozie/coord/input/dependency/CoordInputDependenciesBuilder.java (lines 74 - 75) <https://reviews.apache.org/r/38474/#comment172761> input -> dataset combine -> combineDatasets core/src/main/java/org/apache/oozie/coord/input/dependency/CoordInputDependenciesBuilder.java (line 159) <https://reviews.apache.org/r/38474/#comment172755> Get rid of code for m and h. Only support numeric value to be consistent with other parameters in coordinator xml like timeout, sla start/end/duration. In future can add support for EL and multiplication. I understand that we added support for m and h in APIs for querying, but configuration in general (hadoop and other hadoop projects) has always been just numeric and lets keep it standard. core/src/main/java/org/apache/oozie/coord/input/dependency/CoordInputDependenciesUtil.java (line 141) <https://reviews.apache.org/r/38474/#comment172762> Use [{0}], [{1}] instead of MessageFormat to be consistent with rest of Oozie code. In other places in the code as well. XLog does do MessageFormat internally. core/src/main/java/org/apache/oozie/coord/input/dependency/CoordInputDependencyCheck.java (line 31) <https://reviews.apache.org/r/38474/#comment172630> Please explain in javadoc what the return string is for both input and combine functions. core/src/main/java/org/apache/oozie/coord/input/dependency/CoordInputDependencyCheck.java (line 33) <https://reviews.apache.org/r/38474/#comment172647> evalInput core/src/main/java/org/apache/oozie/coord/input/dependency/CoordInputDependencyCheck.java (line 43) <https://reviews.apache.org/r/38474/#comment172648> evalCombineInput core/src/main/java/org/apache/oozie/coord/input/dependency/CoordInputDependencyCheckPhaseOne.java (line 47) <https://reviews.apache.org/r/38474/#comment172639> private core/src/main/java/org/apache/oozie/coord/input/dependency/CoordInputDependencyCheckPhaseOne.java (line 98) <https://reviews.apache.org/r/38474/#comment172765> Looks like you are not stopping when one path is not found. This means you will hit the namenode lot more and is bad. Need to stop if one path does not exist as the old code did. core/src/main/java/org/apache/oozie/coord/input/dependency/CoordInputDependencyCheckPhaseOne.java (line 152) <https://reviews.apache.org/r/38474/#comment172768> In combine, you need to stop if a path does not exist in all datasets to avoid hitting namenode unnecessarily. core/src/main/java/org/apache/oozie/coord/input/dependency/CoordInputDependencyCheckPhaseOne.java (lines 155 - 156) <https://reviews.apache.org/r/38474/#comment172767> log message should be moved into if (!pathExists(coordInputInstance.getInputDataInstance(), jobConf)) block core/src/main/java/org/apache/oozie/coord/input/dependency/CoordInputDependencyCheckPhaseOne.java (lines 247 - 251) <https://reviews.apache.org/r/38474/#comment172606> Method seems redundant as it just does call another method. Can be just inlined. core/src/main/java/org/apache/oozie/coord/input/dependency/CoordInputDependencyCheckPhaseOne.java (line 286) <https://reviews.apache.org/r/38474/#comment172770> Using true and false as special case values in the string makes the code hacky and was also very hard to follow the code and understand the logic. Can you use a object instead of String and store the path in one variable and this state in another. core/src/main/java/org/apache/oozie/coord/input/dependency/CoordInputDependencyCheckPhaseTwo.java (line 73) <https://reviews.apache.org/r/38474/#comment172785> Why do we need RESOLVED_PATH_WITH_DONE_FLAG instead of RESOLVED_PATH? core/src/main/java/org/apache/oozie/coord/input/dependency/CoordInputDependencyCheckPhaseTwo.java (line 116) <https://reviews.apache.org/r/38474/#comment172774> Why do we need to do this? Where do old instances come from? Can you elaborate that on the comment. core/src/main/java/org/apache/oozie/coord/input/dependency/CoordOldInputDependency.java (line 138) <https://reviews.apache.org/r/38474/#comment172614> dependency -> unresolvedDependencies core/src/main/java/org/apache/oozie/coord/input/dependency/CoordOldInputDependency.java (line 140) <https://reviews.apache.org/r/38474/#comment172621> Should be CoordELFunctions.INSTANCE_SEPARATOR core/src/main/java/org/apache/oozie/coord/input/dependency/CoordOldInputDependency.java (line 282) <https://reviews.apache.org/r/38474/#comment172782> Use the new constant added for is_resolved core/src/main/java/org/apache/oozie/coord/input/dependency/CoordOldInputDependency.java (line 290) <https://reviews.apache.org/r/38474/#comment172783> Use the new constant added for resolved_path core/src/main/java/org/apache/oozie/coord/input/dependency/CoordUnResolvedInputDependency.java (line 35) <https://reviews.apache.org/r/38474/#comment172625> private core/src/main/java/org/apache/oozie/coord/input/dependency/CoordUnResolvedInputDependency.java (lines 53 - 54) <https://reviews.apache.org/r/38474/#comment172627> Plural naming for variable and getter. Either dependencies or dependencyList core/src/main/java/org/apache/oozie/coord/input/dependency/CoordUnResolvedInputDependency.java (line 71) <https://reviews.apache.org/r/38474/#comment172628> Avoid adding separator here. core/src/main/java/org/apache/oozie/coord/input/dependency/OozieJexlInterpreter.java (line 132) <https://reviews.apache.org/r/38474/#comment172638> PHASE_TWO_EVALUATION and TIMED_WAITING could be part of a single enum CoordInputDependencyCheck.STATE Can use static import of the constant to avoid repeating the class name everywhere. - Rohini Palaniswamy On Dec. 15, 2015, 7:33 p.m., Purshotam Shah wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38474/ > ----------------------------------------------------------- > > (Updated Dec. 15, 2015, 7:33 p.m.) > > > Review request for oozie. > > > Bugs: OOZIE-1976 > https://issues.apache.org/jira/browse/OOZIE-1976 > > > Repository: oozie-git > > > Description > ------- > > There are three components in this patch > > 1. User interface > A new tag is added to coordinator.xml > ex. > <input-check> > <or name="test"> > <and> > <data-in dataset="A"/>" > <data-in dataset="B"/> > </and> > <and> > <data-in dataset="C"/> > <data-in dataset="D"/> > </and> > > </or>; > <input-check> > > > input-check will have nested and/or/combine operation. It can have min and > wait at operator or at date-in. > If input-check tag is missing then it consider to be old approach where all > data dependency are needed. > > 2. Processing > input-check is converted into logical expression > (a&&B)||(c&&d) > We use jexl to parse the logical expression. > > There are three phase in parsing. > phase 1 : only resolved dataset are parsed ( only current). > phase 2 : once all current are resolved, then future/latest are parsed. > phase 3 : Doesn't do any filecheck, just return what is being parsed by > phase1 and phase2. Is used for EL functions > > > 3. Storage. > if inputcheck is enable, push_missing_dependencies and missing_dependencies > are serialized and stored in DB. > If then not then it's old approach, where they are stored in plan text. This > is backward compatible. > > > Diffs > ----- > > client/src/main/resources/oozie-coordinator-0.5.xsd > e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 > core/pom.xml b063dab79415447a86c1a33f5c3f5304e0dffca0 > core/src/main/java/org/apache/oozie/CoordinatorActionBean.java > 91bff4dca2ef2dece68ca7260724ba3e43b1a08e > core/src/main/java/org/apache/oozie/ErrorCode.java > 6c1e3997c9a1cb0bb0de39d687a083af0a7b5f04 > > core/src/main/java/org/apache/oozie/command/coord/CoordActionInputCheckXCommand.java > 742d00dd47aab55392abc8fbc207f8100728b832 > > core/src/main/java/org/apache/oozie/command/coord/CoordActionUpdatePushMissingDependency.java > 4e1c5b3392358cb6b1e98e16e469310338f27fed > core/src/main/java/org/apache/oozie/command/coord/CoordCommandUtils.java > 58ef483272264e0d391d4a1e6533dc1cab9940da > > core/src/main/java/org/apache/oozie/command/coord/CoordMaterializeTransitionXCommand.java > 39e6ac15ce3a3ea7f2ed9178688537f7b1d7842d > > core/src/main/java/org/apache/oozie/command/coord/CoordPushDependencyCheckXCommand.java > b05344d89e8df0e11fe69c1aa725d19a18eb0a2b > core/src/main/java/org/apache/oozie/coord/CoordELConstants.java > f010a817fc900821c0e429fc16e1d3902a98d8bb > core/src/main/java/org/apache/oozie/coord/CoordELEvaluator.java > 8b2f4560ae66bbcd707a446382e647663ea67be1 > core/src/main/java/org/apache/oozie/coord/CoordELFunctions.java > 5d238663aa94f0dd55a9190b60bfe621439c7b53 > core/src/main/java/org/apache/oozie/coord/CoordUtils.java > 94c69740618110ea180b188ab0c5a02db76f8b4d > core/src/main/java/org/apache/oozie/coord/SyncCoordAction.java > 44258eb5be40bf6769e32c8780117e8533d80d7e > > core/src/main/java/org/apache/oozie/coord/input/dependency/AbstractCoordInputDependency.java > e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 > > core/src/main/java/org/apache/oozie/coord/input/dependency/CoordInputDependenciesBuilder.java > e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 > > core/src/main/java/org/apache/oozie/coord/input/dependency/CoordInputDependenciesChecker.java > e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 > > core/src/main/java/org/apache/oozie/coord/input/dependency/CoordInputDependenciesUtil.java > e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 > > core/src/main/java/org/apache/oozie/coord/input/dependency/CoordInputDependency.java > e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 > > core/src/main/java/org/apache/oozie/coord/input/dependency/CoordInputDependencyCheck.java > e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 > > core/src/main/java/org/apache/oozie/coord/input/dependency/CoordInputDependencyCheckPhaseOne.java > e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 > > core/src/main/java/org/apache/oozie/coord/input/dependency/CoordInputDependencyCheckPhaseThree.java > e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 > > core/src/main/java/org/apache/oozie/coord/input/dependency/CoordInputDependencyCheckPhaseTwo.java > e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 > > core/src/main/java/org/apache/oozie/coord/input/dependency/CoordInputDependencyCheckPhaseValidate.java > e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 > > core/src/main/java/org/apache/oozie/coord/input/dependency/CoordInputDependencyFactory.java > e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 > > core/src/main/java/org/apache/oozie/coord/input/dependency/CoordInputInstance.java > e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 > > core/src/main/java/org/apache/oozie/coord/input/dependency/CoordOldInputDependency.java > e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 > > core/src/main/java/org/apache/oozie/coord/input/dependency/CoordPullInputDependency.java > e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 > > core/src/main/java/org/apache/oozie/coord/input/dependency/CoordPushInputDependency.java > e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 > > core/src/main/java/org/apache/oozie/coord/input/dependency/CoordUnResolvedInputDependency.java > e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 > > core/src/main/java/org/apache/oozie/coord/input/dependency/InputLogicParser.java > e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 > > core/src/main/java/org/apache/oozie/coord/input/dependency/OozieJexlEngine.java > e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 > > core/src/main/java/org/apache/oozie/coord/input/dependency/OozieJexlInterpreter.java > e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 > core/src/main/java/org/apache/oozie/dependency/ActionDependency.java > c280d1dc2387c9b3df96d4e83ad5563e10b1e1ef > core/src/main/java/org/apache/oozie/dependency/DependencyChecker.java > a5507575a3403103133f85a78c873c9976419857 > core/src/main/java/org/apache/oozie/util/WritableUtils.java > 76a68953541125cab05bd05c94aa73b50e5363ff > core/src/main/resources/oozie-default.xml > faf37405757719554a5b6e3671d0c87723eb9959 > > core/src/test/java/org/apache/oozie/command/coord/TestCoordActionInputCheckXCommand.java > 1fe1b3adb5ceaa83985ef31278e49a3224cde0fb > > core/src/test/java/org/apache/oozie/command/coord/TestCoordCommandUtils.java > 7062e697e5162ac25688d9cb62352bd139a7013a > > core/src/test/java/org/apache/oozie/command/coord/TestCoordMaterializeTransitionXCommand.java > 29e7ca145612fadee661fe975aa0ea2cab1cbfa3 > > core/src/test/java/org/apache/oozie/command/coord/TestCoordSubmitXCommand.java > 52eb9dd1fb903ac86294a55d6e2a6918390bf4e9 > > core/src/test/java/org/apache/oozie/coord/input/dependency/TestCoordInputCheckPush.java > e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 > > core/src/test/java/org/apache/oozie/coord/input/dependency/TestCoordinatorInputCheck.java > e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 > > core/src/test/java/org/apache/oozie/coord/input/dependency/TestInputLogicParser.java > e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 > core/src/test/resources/coord-action-sla.xml > f3f1bc09c51272a35d65edd7271599e7e8ba1ba4 > core/src/test/resources/coord-dataset-offset.xml > 5dfbb1b40ee86dfc3336031eea3c5ff3144b9f5a > core/src/test/resources/coord-inputcheck-combine.xml > e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 > core/src/test/resources/coord-inputcheck-hcat.xml > e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 > core/src/test/resources/coord-inputcheck-latest.xml > e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 > core/src/test/resources/coord-inputcheck-range-latest.xml > e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 > core/src/test/resources/coord-inputcheck-range.xml > e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 > core/src/test/resources/coord-inputcheck.xml > e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 > pom.xml a74ffab081defd15a1c5dbbf15355aa4e0455029 > > Diff: https://reviews.apache.org/r/38474/diff/ > > > Testing > ------- > > > Thanks, > > Purshotam Shah > >
