----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3486/#review4458 -----------------------------------------------------------
Comments on code structure, variable names, lack of comments, etc. trunk/core/src/main/java/org/apache/oozie/workflow/lite/ForkJoinElement.java <https://reviews.apache.org/r/3486/#comment9985> What does a label signify? trunk/core/src/main/java/org/apache/oozie/workflow/lite/ForkJoinElement.java <https://reviews.apache.org/r/3486/#comment9986> What does a type signify? Can you enumerate it or point to the class which enumerates the types? trunk/core/src/main/java/org/apache/oozie/workflow/lite/ForkJoinElement.java <https://reviews.apache.org/r/3486/#comment9987> What is the meaning of number of paths - is it the number of outgoing edges from this node? trunk/core/src/main/java/org/apache/oozie/workflow/lite/LiteWorkflowApp.java <https://reviews.apache.org/r/3486/#comment9991> Please use a descriptive variable name. 'n' is not a strong variable name. trunk/core/src/main/java/org/apache/oozie/workflow/lite/LiteWorkflowApp.java <https://reviews.apache.org/r/3486/#comment9988> Can you remove this line? trunk/core/src/main/java/org/apache/oozie/workflow/lite/LiteWorkflowApp.java <https://reviews.apache.org/r/3486/#comment9989> Children and not childs trunk/core/src/main/java/org/apache/oozie/workflow/lite/LiteWorkflowApp.java <https://reviews.apache.org/r/3486/#comment9990> Can you add a comment here that you are checking for at least two outgoing paths from a fork node? trunk/core/src/main/java/org/apache/oozie/workflow/lite/LiteWorkflowApp.java <https://reviews.apache.org/r/3486/#comment9995> Can you pick a more descriptive variable name instead of 'se' trunk/core/src/main/java/org/apache/oozie/workflow/lite/LiteWorkflowApp.java <https://reviews.apache.org/r/3486/#comment9992> children and not childs trunk/core/src/main/java/org/apache/oozie/workflow/lite/LiteWorkflowApp.java <https://reviews.apache.org/r/3486/#comment9993> Remove the commented out section of the code trunk/core/src/main/java/org/apache/oozie/workflow/lite/LiteWorkflowApp.java <https://reviews.apache.org/r/3486/#comment9994> Can you add a corrective measure, i.e., which node has this issue and the possible remedies? trunk/core/src/main/java/org/apache/oozie/workflow/lite/LiteWorkflowApp.java <https://reviews.apache.org/r/3486/#comment9997> Move the else to line 170 and include the comment after the '{' trunk/core/src/main/java/org/apache/oozie/workflow/lite/LiteWorkflowApp.java <https://reviews.apache.org/r/3486/#comment9996> This check can be performed before this method is called. trunk/core/src/main/java/org/apache/oozie/workflow/lite/LiteWorkflowApp.java <https://reviews.apache.org/r/3486/#comment10000> Can you add a comment about the method? If I understand it correctly, this method is to be called only when the fork-join pair is already on the stack. The method then validates if all the outgoing paths in a fork add up to the incoming paths to a join and updates the counts for the join appropriately. trunk/core/src/main/java/org/apache/oozie/workflow/lite/LiteWorkflowApp.java <https://reviews.apache.org/r/3486/#comment10001> Why is the increment required here? trunk/core/src/main/java/org/apache/oozie/workflow/lite/LiteWorkflowApp.java <https://reviews.apache.org/r/3486/#comment9998> Remove the commented out code trunk/core/src/main/java/org/apache/oozie/workflow/lite/LiteWorkflowAppParser.java <https://reviews.apache.org/r/3486/#comment9999> Add a comment to describe the intention of this check trunk/core/src/test/java/org/apache/oozie/workflow/lite/TestLiteWorkflowApp.java <https://reviews.apache.org/r/3486/#comment10002> Can you add ASCII art to demonstrate the graph? It will help in understanding the intention of the test case better. trunk/core/src/test/java/org/apache/oozie/workflow/lite/TestLiteWorkflowApp.java <https://reviews.apache.org/r/3486/#comment10007> Add a try catch block for the WorkflowException and fail in the catch block. Its better than throwing the WorkflowException. trunk/core/src/test/java/org/apache/oozie/workflow/lite/TestLiteWorkflowApp.java <https://reviews.apache.org/r/3486/#comment10003> Can you add ASCII art to demonstrate the graph? It will help in understanding the intention of the test case better. trunk/core/src/test/java/org/apache/oozie/workflow/lite/TestLiteWorkflowApp.java <https://reviews.apache.org/r/3486/#comment10008> Add a try catch block for the WorkflowException and fail in the catch block. Its better than throwing the WorkflowException. trunk/core/src/test/java/org/apache/oozie/workflow/lite/TestLiteWorkflowApp.java <https://reviews.apache.org/r/3486/#comment10004> Can you add ASCII art to demonstrate the graph? It will help in understanding the intention of the test case better. trunk/core/src/test/java/org/apache/oozie/workflow/lite/TestLiteWorkflowApp.java <https://reviews.apache.org/r/3486/#comment10009> Add a try catch block for the WorkflowException and fail in the catch block. Its better than throwing the WorkflowException. trunk/core/src/test/java/org/apache/oozie/workflow/lite/TestLiteWorkflowApp.java <https://reviews.apache.org/r/3486/#comment10010> Can you add ASCII art to demonstrate the graph? It will help in understanding the intention of the test case better. trunk/core/src/test/java/org/apache/oozie/workflow/lite/TestLiteWorkflowApp.java <https://reviews.apache.org/r/3486/#comment10005> Is the throws clause required? trunk/core/src/test/java/org/apache/oozie/workflow/lite/TestLiteWorkflowApp.java <https://reviews.apache.org/r/3486/#comment10011> Can you add ASCII art to demonstrate the graph? It will help in understanding the intention of the test case better. trunk/core/src/test/java/org/apache/oozie/workflow/lite/TestLiteWorkflowApp.java <https://reviews.apache.org/r/3486/#comment10006> Is the throws clause required? - Santhosh On 2012-01-13 17:39:11, Virag Kothari wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/3486/ > ----------------------------------------------------------- > > (Updated 2012-01-13 17:39:11) > > > Review request for oozie, Mohammad Islam and Angelo K. Huang. > > > Summary > ------- > > Validate fork and join at wf submission time > https://issues.apache.org/jira/browse/OOZIE-636 > > Brief description of algo: > > A modified dfs algorithm is used. Two stacks, one for dfs traversal and other > for maintaining fork join status, are kept. When a fork is encountered > during traversal, it is added to the forkjoin stack and number of paths > associated with the fork is also stored. When a node’s child is seen as a > join, the join is added to the forkJoin stack and the no. of paths to it is > updated. When the number of paths for fork and join are equal, then the > fork/join pair is removed from the forkJoin stack and join is pushed to the > dfsStack. > > Nodes other than fork and join are only pushed to the dfs stack. > If a action node is seen, only the node's "ok-to" transition is considered > > > While(!stack.isEmpty()){ > Node n = DfsStack.pop() > n.traversed = true; > If(n.type==fork){ > ForkJoinStack.push(new Element(n, n.paths) ); > } > List<Node> childs = getUnvisitedChildnodes(n) > For(Node n: childs){ > If (n.type==join){ > Boolean b=isForkJoinCleared(ForkJoinStack) > If(!b){ > Continue; > } > stack.push(n); > n.traversed = true; > } > } > > > This addresses bug OOZIE-636. > https://issues.apache.org/jira/browse/OOZIE-636 > > > Diffs > ----- > > trunk/core/src/main/java/org/apache/oozie/ErrorCode.java 1230856 > > trunk/core/src/main/java/org/apache/oozie/workflow/lite/ForkJoinElement.java > PRE-CREATION > > trunk/core/src/main/java/org/apache/oozie/workflow/lite/LiteWorkflowApp.java > 1230856 > > trunk/core/src/main/java/org/apache/oozie/workflow/lite/LiteWorkflowAppParser.java > 1230856 > > trunk/core/src/test/java/org/apache/oozie/service/TestLiteWorkflowAppService.java > 1230856 > > trunk/core/src/test/java/org/apache/oozie/workflow/lite/TestLiteWorkflowApp.java > PRE-CREATION > trunk/core/src/test/resources/wf-schema-valid.xml 1230856 > > Diff: https://reviews.apache.org/r/3486/diff > > > Testing > ------- > > Test case to validate fork-join added > > > Thanks, > > Virag > >
