----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50202/#review143032 -----------------------------------------------------------
core/src/main/java/org/apache/oozie/workflow/lite/LiteWorkflowValidator.java (line 55) <https://reviews.apache.org/r/50202/#comment208721> I'm not a big fan of the generic Pair classes. You loose the context of what you're storing here (because no member names). Plus, you can't easily add additional members later if you need to. So I'd recommend replacing Pair with an private inner class (something like ForkJoinCounter or more generic like BasicValidationContext), and putting in the two member ints we need. Another alternative is to use some class variables for the fork and join counts and then you don't have to pass them around at all. core/src/main/java/org/apache/oozie/workflow/lite/LiteWorkflowValidator.java (line 77) <https://reviews.apache.org/r/50202/#comment208723> I would explicetly mention that this is recursive core/src/main/java/org/apache/oozie/workflow/lite/LiteWorkflowValidator.java (line 146) <https://reviews.apache.org/r/50202/#comment208734> I would also explicetly state that this is recurisve. core/src/main/java/org/apache/oozie/workflow/lite/LiteWorkflowValidator.java (line 164) <https://reviews.apache.org/r/50202/#comment208736> Can you add some more comments on how this method and its code works? It's pretty hard to follow what's happening as-is. My original code was pretty hard to follow, even with comments :) core/src/main/java/org/apache/oozie/workflow/lite/LiteWorkflowValidator.java (line 252) <https://reviews.apache.org/r/50202/#comment208731> This (uses reflection) to create a new ActionExecutor of the action type, which we then don't need here. It would be more efficient to add a new method to ActionService that checks if a given type is valid. Something like this: ```` boolean hasActionType(String actionType) { ParamChecker.notEmpty(actionType, "actionType"); return executors.containsKey(actionType); } ```` And then simply: ```` boolean supportedAction = Services.get().get(ActionService.class).hasActionType(action.getName()); ```` core/src/test/java/org/apache/oozie/workflow/lite/TestLiteWorkflowAppParser.java (line 18) <https://reviews.apache.org/r/50202/#comment208733> It would be good to inclide the Test changes I made in the wip patch I posted. It's mostly just some comment clarifications, but there's also a fancy test for verifying that forkjoin validation doesn't take too long (the testFoo method, which I forgot to rename). - Robert Kanter On July 19, 2016, 7:56 p.m., Peter Bacsko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50202/ > ----------------------------------------------------------- > > (Updated July 19, 2016, 7:56 p.m.) > > > Review request for oozie. > > > Bugs: OOZIE-1978 > https://issues.apache.org/jira/browse/OOZIE-1978 > > > Repository: oozie-git > > > Description > ------- > > See OOZIE-1978 for details. > > > Diffs > ----- > > core/src/main/java/org/apache/oozie/ErrorCode.java 2907ca2 > > core/src/main/java/org/apache/oozie/workflow/lite/LiteWorkflowAppParser.java > a1b9cdb > > core/src/main/java/org/apache/oozie/workflow/lite/LiteWorkflowValidator.java > PRE-CREATION > core/src/test/java/org/apache/oozie/command/wf/TestSubmitXCommand.java > 73464c8 > > core/src/test/java/org/apache/oozie/workflow/lite/TestLiteWorkflowAppParser.java > 9002b6c > > Diff: https://reviews.apache.org/r/50202/diff/ > > > Testing > ------- > > Existing tests pass. > > We might need to add some more, at least testing acyclic graph detection. > > > Thanks, > > Peter Bacsko > >
