[
https://issues.apache.org/jira/browse/OOZIE-636?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13188865#comment-13188865
]
[email protected] commented on OOZIE-636:
-----------------------------------------------------
-----------------------------------------------------------
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:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/3486/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2012-01-13 17:39:11)
bq.
bq.
bq. Review request for oozie, Mohammad Islam and Angelo K. Huang.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. Validate fork and join at wf submission time
bq. https://issues.apache.org/jira/browse/OOZIE-636
bq.
bq. Brief description of algo:
bq.
bq. 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.
bq.
bq. Nodes other than fork and join are only pushed to the dfs stack.
bq. If a action node is seen, only the node's "ok-to" transition is considered
bq.
bq.
bq. While(!stack.isEmpty()){
bq. Node n = DfsStack.pop()
bq. n.traversed = true;
bq. If(n.type==fork){
bq. ForkJoinStack.push(new Element(n, n.paths) );
bq. }
bq. List<Node> childs = getUnvisitedChildnodes(n)
bq. For(Node n: childs){
bq. If (n.type==join){
bq. Boolean b=isForkJoinCleared(ForkJoinStack)
bq. If(!b){
bq. Continue;
bq. }
bq. stack.push(n);
bq. n.traversed = true;
bq. }
bq. }
bq.
bq.
bq. This addresses bug OOZIE-636.
bq. https://issues.apache.org/jira/browse/OOZIE-636
bq.
bq.
bq. Diffs
bq. -----
bq.
bq. trunk/core/src/main/java/org/apache/oozie/ErrorCode.java 1230856
bq.
trunk/core/src/main/java/org/apache/oozie/workflow/lite/ForkJoinElement.java
PRE-CREATION
bq.
trunk/core/src/main/java/org/apache/oozie/workflow/lite/LiteWorkflowApp.java
1230856
bq.
trunk/core/src/main/java/org/apache/oozie/workflow/lite/LiteWorkflowAppParser.java
1230856
bq.
trunk/core/src/test/java/org/apache/oozie/service/TestLiteWorkflowAppService.java
1230856
bq.
trunk/core/src/test/java/org/apache/oozie/workflow/lite/TestLiteWorkflowApp.java
PRE-CREATION
bq. trunk/core/src/test/resources/wf-schema-valid.xml 1230856
bq.
bq. Diff: https://reviews.apache.org/r/3486/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq. Test case to validate fork-join added
bq.
bq.
bq. Thanks,
bq.
bq. Virag
bq.
bq.
> Check fork and join in the workflow in the submission time
> -----------------------------------------------------------
>
> Key: OOZIE-636
> URL: https://issues.apache.org/jira/browse/OOZIE-636
> Project: Oozie
> Issue Type: Bug
> Reporter: Virag Kothari
>
> Enhancement: Oozie should check that the fork node and join node are correct
> in pair when user submits the job. This should be a static check, not when
> the workflow is running.
> Current logic bug:
> A workflow with different number of forks and joins was run. The wf job
> should have been killed but it succeeded. Also, strangely, the action was
> killed.
> Following are the different types of tests run and their results with varying
> delays.
> test1: wf job SUCCEEDED, action java12 KILLED.
> delay11=11
> delay12=12
> delay121=1
> delay122=2
> delay21=1
> delay22=1
> test2: wf job SUCCEEDED, action java12 KILLED.
> delay11=1
> delay12=12
> delay121=1
> delay122=2
> delay21=1
> delay22=1
> test3: wf job SUCCEEED, all actions OK. question: why wf job always pass in
> this scenario, even when fork-join not in
> pair?
> delay11=10
> delay12=10
> delay121=15
> delay122=15
> delay21=20
> delay22=20
> workflow.xml
> ============
> <workflow-app xmlns='uri:oozie:workflow:0.1' name='fork-join-4735180-wf'>
> <start to='fork1' />
> <fork name="fork1">
> <path start="java11" />
> <path start="fork12" />
> </fork>
> <action name='java11'>
> <java>
> <job-tracker>${jobTracker}</job-tracker>
> <name-node>${nameNode}</name-node>
> <configuration>
> <property>
> <name>mapred.job.queue.name</name>
> <value>${queueName}</value>
> </property>
> </configuration>
> <main-class>qa.test.tests.testsleep</main-class>
> <arg>${delay11}</arg>
> </java>
> <ok to="java12" />
> <error to="fail" />
> </action>
> <action name='java12'>
> <java>
> <job-tracker>${jobTracker}</job-tracker>
> <name-node>${nameNode}</name-node>
> <configuration>
> <property>
> <name>mapred.job.queue.name</name>
> <value>${queueName}</value>
> </property>
> </configuration>
> <main-class>qa.test.tests.testsleep</main-class>
> <arg>${delay12}</arg>
> </java>
> <ok to="join1" />
> <error to="fail" />
> </action>
> <fork name="fork12">
> <path start="java121" />
> <path start="java122" />
> </fork>
> <action name='java121'>
> <java>
> <job-tracker>${jobTracker}</job-tracker>
> <name-node>${nameNode}</name-node>
> <configuration>
> <property>
> <name>mapred.job.queue.name</name>
> <value>${queueName}</value>
> </property>
> </configuration>
> <main-class>qa.test.tests.testsleep</main-class>
> <arg>${delay121}</arg>
> </java>
> <ok to="join12" />
> <error to="fail" />
> </action>
> <action name='java122'>
> <java>
> <job-tracker>${jobTracker}</job-tracker>
> <name-node>${nameNode}</name-node>
> <configuration>
> <property>
> <name>mapred.job.queue.name</name>
> <value>${queueName}</value>
> </property>
> </configuration>
> <main-class>qa.test.tests.testsleep</main-class>
> <arg>${delay122}</arg>
> </java>
> <ok to="join12" />
> <error to="fail" />
> </action>
> <join name="join12" to="fork2" />
> <fork name="fork2">
> <path start="java21" />
> <path start="java22" />
> </fork>
> <action name='java21'>
> <java>
> <job-tracker>${jobTracker}</job-tracker>
> <name-node>${nameNode}</name-node>
> <configuration>
> <property>
> <name>mapred.job.queue.name</name>
> <value>${queueName}</value>
> </property>
> </configuration>
> <main-class>qa.test.tests.testsleep</main-class>
> <arg>${delay21}</arg>
> </java>
> <ok to="join1" />
> <error to="fail" />
> </action>
> <action name='java22'>
> <java>
> <job-tracker>${jobTracker}</job-tracker>
> <name-node>${nameNode}</name-node>
> <configuration>
> <property>
> <name>mapred.job.queue.name</name>
> <value>${queueName}</value>
> </property>
> </configuration>
> <main-class>qa.test.tests.testsleep</main-class>
> <arg>${delay22}</arg>
> </java>
> <ok to="join1" />
> <error to="fail" />
> </action>
> <join name="join1" to="end" />
> <kill name="fail">
> <message>Streaming Map/Reduce failed, error
> message[${wf:errorMessage(wf:lastErrorNode())}]</message>
> </kill>
> <end name='end' />
> </workflow-app>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators:
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
