----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38474/#review112102 -----------------------------------------------------------
These are comments for Page 1 and 2. Still I have not fully gone through and understood few of the classes. Will comment on those and the testcases/Page 3 in next review. At high level, 1) Noticed naming and EL function changed from design doc you had put up earlier. Liked those better. - Possible to rename <input-check> to <input-logic> as per the design doc - Possible to reuse the existing EL also as per design doc? i.e coord:dataIn(CorD) instead of coord:inputCheckDataIn(CorD) Keeps it simple for user. 2) User documentation missing as mentioned by Robert. 3) Couple of comments are my usual nitpicks on typos and variable naming. client/src/main/resources/oozie-coordinator-0.5.xsd (line 115) <https://reviews.apache.org/r/38474/#comment172502> type="coordinator:IDENTIFIER" . Same for other names. Shouldn't it be use="required" ? client/src/main/resources/oozie-coordinator-0.5.xsd (lines 116 - 117) <https://reviews.apache.org/r/38474/#comment172504> min and wait seems to be supported only at the data-in level. client/src/main/resources/oozie-coordinator-0.5.xsd (line 139) <https://reviews.apache.org/r/38474/#comment172503> Why does this need a name? core/src/main/java/org/apache/oozie/CoordinatorActionBean.java (lines 822 - 826) <https://reviews.apache.org/r/38474/#comment172561> Declare variables at the top along with other variables core/src/main/java/org/apache/oozie/command/coord/CoordActionInputCheckXCommand.java (line 169) <https://reviews.apache.org/r/38474/#comment172506> Nitpick. isPushDependenciesMeet -> isPushDependenciesMet . There are couple of other places as well. Please grep for meet and change to met. core/src/main/java/org/apache/oozie/command/coord/CoordActionInputCheckXCommand.java <https://reviews.apache.org/r/38474/#comment172552> Can we keep this log statement? Might not be exactly correct anymore if input-logic is defined, but still can be helpful to some extent in determining what to look for core/src/main/java/org/apache/oozie/command/coord/CoordActionInputCheckXCommand.java (lines 172 - 174) <https://reviews.apache.org/r/38474/#comment172553> Seems redundant. core/src/main/java/org/apache/oozie/command/coord/CoordActionInputCheckXCommand.java (line 275) <https://reviews.apache.org/r/38474/#comment172562> This is redundant. UPDATE_COORD_ACTION_FOR_INPUTCHECK only serializes pull missing dependencies core/src/main/java/org/apache/oozie/command/coord/CoordActionInputCheckXCommand.java (line 436) <https://reviews.apache.org/r/38474/#comment172565> Why public? Also method seems to be unused. To be removed? core/src/main/java/org/apache/oozie/command/coord/CoordActionInputCheckXCommand.java (line 494) <https://reviews.apache.org/r/38474/#comment172566> Why public? Also method seems to be unused as it is copied to CoordOldInputDependency. To be removed? core/src/main/java/org/apache/oozie/command/coord/CoordActionUpdatePushMissingDependency.java (line 43) <https://reviews.apache.org/r/38474/#comment172567> Seems wrong. Getter is that of push dependencies instead of pull. core/src/main/java/org/apache/oozie/command/coord/CoordMaterializeTransitionXCommand.java (line 533) <https://reviews.apache.org/r/38474/#comment172507> isInputLogicSpecified core/src/main/java/org/apache/oozie/command/coord/CoordMaterializeTransitionXCommand.java (line 534) <https://reviews.apache.org/r/38474/#comment172508> validateInputLogic core/src/main/java/org/apache/oozie/command/coord/CoordPushDependencyCheckXCommand.java (line 123) <https://reviews.apache.org/r/38474/#comment172576> Keep the info log statement. May not be fully correct when OR is involved, but still would be useful. List<String> availableList - This is not required. It will try to unregister already available stuff. Can just do unregisterAvailableDependencies(actionDep.getAvailableDependencies()); as before core/src/main/java/org/apache/oozie/command/coord/CoordPushDependencyCheckXCommand.java (line 171) <https://reviews.apache.org/r/38474/#comment172581> coordPushInputDependency.isDependencyMet() is redundant. Pushdependency cannot be met without changeindependency core/src/main/java/org/apache/oozie/coord/CoordUtils.java (line 434) <https://reviews.apache.org/r/38474/#comment172511> getInputLogic core/src/main/java/org/apache/oozie/coord/CoordUtils.java (line 439) <https://reviews.apache.org/r/38474/#comment172512> Nitpick. getDataInputFromString core/src/main/java/org/apache/oozie/coord/input/dependency/AbstractCoordInputDependency.java (line 64) <https://reviews.apache.org/r/38474/#comment172530> Nitpick. Does nothing and can be removed core/src/main/java/org/apache/oozie/coord/input/dependency/AbstractCoordInputDependency.java (lines 299 - 300) <https://reviews.apache.org/r/38474/#comment172548> Can write the boolean first and then map. Can stop deserializing if dependency is met for display purposes. core/src/main/java/org/apache/oozie/coord/input/dependency/CoordInputDependency.java (line 33) <https://reviews.apache.org/r/38474/#comment172519> V=1 core/src/main/java/org/apache/oozie/coord/input/dependency/CoordInputDependency.java (lines 96 - 98) <https://reviews.apache.org/r/38474/#comment172547> join -> missingDependencies core/src/main/java/org/apache/oozie/coord/input/dependency/CoordInputDependencyFactory.java (line 43) <https://reviews.apache.org/r/38474/#comment172554> isInputCheck -> hasInputLogic. Other places too core/src/main/java/org/apache/oozie/coord/input/dependency/CoordInputDependencyFactory.java (line 58) <https://reviews.apache.org/r/38474/#comment172555> Nitpick. Pushinput -> PushInput core/src/main/java/org/apache/oozie/util/WritableUtils.java (line 130) <https://reviews.apache.org/r/38474/#comment172517> readStringList core/src/main/java/org/apache/oozie/util/WritableUtils.java (line 154) <https://reviews.apache.org/r/38474/#comment172518> writeStringList - 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 > >
