You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@oozie.apache.org by Peter Bacsko <pb...@cloudera.com> on 2016/07/19 19:56:34 UTC
Review Request 50202: [OOZIE-1978] Forkjoin validation code is
ridiculously slow in some cases
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50202/
-----------------------------------------------------------
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
Re: Review Request 50202: [OOZIE-1978] Forkjoin validation code is
ridiculously slow in some cases
Posted by Robert Kanter <rk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50202/#review146921
-----------------------------------------------------------
I'll need to spend some more time going through the forkjoin validation logic, but here's some feedback so far.
core/src/main/java/org/apache/oozie/workflow/lite/LiteWorkflowValidator.java (line 49)
<https://reviews.apache.org/r/50202/#comment213742>
If this somehow does happen, this will bring down the entire Oozie Server!
Instead, we should throw a WorkflowException so that it fails the Workflow.
core/src/main/java/org/apache/oozie/workflow/lite/LiteWorkflowValidator.java (line 78)
<https://reviews.apache.org/r/50202/#comment213743>
it only has supported action nodes
core/src/main/java/org/apache/oozie/workflow/lite/LiteWorkflowValidator.java (line 88)
<https://reviews.apache.org/r/50202/#comment213748>
constraints
core/src/main/java/org/apache/oozie/workflow/lite/LiteWorkflowValidator.java (line 95)
<https://reviews.apache.org/r/50202/#comment213745>
I'd put this logic back in here. It's doing a few different things depending on the node type and it would be easier to follow if they were right here instead of far away in another method.
core/src/main/java/org/apache/oozie/workflow/lite/LiteWorkflowValidator.java (line 134)
<https://reviews.apache.org/r/50202/#comment213747>
contraints
core/src/main/java/org/apache/oozie/workflow/lite/LiteWorkflowValidator.java (lines 149 - 154)
<https://reviews.apache.org/r/50202/#comment213749>
I like the diagram and giant comments :)
core/src/main/java/org/apache/oozie/workflow/lite/LiteWorkflowValidator.java (line 296)
<https://reviews.apache.org/r/50202/#comment213744>
Again, let's throw a WorkflowException.
core/src/test/java/org/apache/oozie/TestWorkflowBean.java (lines 35 - 37)
<https://reviews.apache.org/r/50202/#comment213750>
Unrelated changes
core/src/test/java/org/apache/oozie/workflow/lite/TestLiteWorkflowAppParser.java (line 1446)
<https://reviews.apache.org/r/50202/#comment213752>
This should scale using the same parameter as waitFor in XTestCase.
- Robert Kanter
On July 23, 2016, 5:54 p.m., Peter Bacsko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50202/
> -----------------------------------------------------------
>
> (Updated July 23, 2016, 5:54 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/service/ActionService.java becc69b
> 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/TestWorkflowBean.java b68f0f7
> core/src/test/java/org/apache/oozie/command/wf/TestSubmitXCommand.java 73464c8
> core/src/test/java/org/apache/oozie/workflow/lite/TestLiteWorkflowAppParser.java 9002b6c
> core/src/test/resources/wf-long.xml PRE-CREATION
>
> 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
>
>
Re: Review Request 50202: [OOZIE-1978] Forkjoin validation code is
ridiculously slow in some cases
Posted by Robert Kanter <rk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50202/#review149163
-----------------------------------------------------------
Ship it!
Ship It!
- Robert Kanter
On Aug. 31, 2016, 1:02 p.m., Peter Bacsko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50202/
> -----------------------------------------------------------
>
> (Updated Aug. 31, 2016, 1:02 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/service/ActionService.java becc69b
> core/src/main/java/org/apache/oozie/workflow/lite/LiteWorkflowAppParser.java 0541634
> 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
> core/src/test/resources/wf-long.xml PRE-CREATION
>
> 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
>
>
Re: Review Request 50202: [OOZIE-1978] Forkjoin validation code is
ridiculously slow in some cases
Posted by Peter Bacsko <pb...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50202/
-----------------------------------------------------------
(Updated aug. 31, 2016, 1:02 du)
Review request for oozie.
Changes
-------
Addressing issues mentioned by Robert
Bugs: OOZIE-1978
https://issues.apache.org/jira/browse/OOZIE-1978
Repository: oozie-git
Description
-------
See OOZIE-1978 for details.
Diffs (updated)
-----
core/src/main/java/org/apache/oozie/ErrorCode.java 2907ca2
core/src/main/java/org/apache/oozie/service/ActionService.java becc69b
core/src/main/java/org/apache/oozie/workflow/lite/LiteWorkflowAppParser.java 0541634
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
core/src/test/resources/wf-long.xml PRE-CREATION
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
Re: Review Request 50202: [OOZIE-1978] Forkjoin validation code is
ridiculously slow in some cases
Posted by Peter Bacsko <pb...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50202/
-----------------------------------------------------------
(Updated j�l. 23, 2016, 5:54 du)
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 (updated)
-----
core/src/main/java/org/apache/oozie/ErrorCode.java 2907ca2
core/src/main/java/org/apache/oozie/service/ActionService.java becc69b
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/TestWorkflowBean.java b68f0f7
core/src/test/java/org/apache/oozie/command/wf/TestSubmitXCommand.java 73464c8
core/src/test/java/org/apache/oozie/workflow/lite/TestLiteWorkflowAppParser.java 9002b6c
core/src/test/resources/wf-long.xml PRE-CREATION
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
Re: Review Request 50202: [OOZIE-1978] Forkjoin validation code is
ridiculously slow in some cases
Posted by Attila Sasvari <as...@cloudera.com>.
> On j�l. 21, 2016, 11:32 de, Attila Sasvari wrote:
> > core/src/main/java/org/apache/oozie/workflow/lite/LiteWorkflowValidator.java, line 142
> > <https://reviews.apache.org/r/50202/diff/1/?file=1447527#file1447527line142>
> >
> > This method does too many things / goes againts the Single responsibility principle.
> >
> > Can you extract methods?
>
> Peter Bacsko wrote:
> You mean I should walk the graph multiple times and verify the conditions separately or shall I just extract methods from performValidation() to make it readable?
just to make it readable (for example validation of "okTo" paths to a given node could be a separate method)
- Attila
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50202/#review143068
-----------------------------------------------------------
On j�l. 19, 2016, 7:56 du, Peter Bacsko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50202/
> -----------------------------------------------------------
>
> (Updated j�l. 19, 2016, 7:56 du)
>
>
> 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
>
>
Re: Review Request 50202: [OOZIE-1978] Forkjoin validation code is
ridiculously slow in some cases
Posted by Peter Bacsko <pb...@cloudera.com>.
> On j�l. 21, 2016, 11:32 de, Attila Sasvari wrote:
> > core/src/main/java/org/apache/oozie/workflow/lite/LiteWorkflowValidator.java, line 142
> > <https://reviews.apache.org/r/50202/diff/1/?file=1447527#file1447527line142>
> >
> > This method does too many things / goes againts the Single responsibility principle.
> >
> > Can you extract methods?
You mean I should walk the graph multiple times and verify the conditions separately or shall I just extract methods from performValidation() to make it readable?
- Peter
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50202/#review143068
-----------------------------------------------------------
On j�l. 19, 2016, 7:56 du, Peter Bacsko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50202/
> -----------------------------------------------------------
>
> (Updated j�l. 19, 2016, 7:56 du)
>
>
> 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
>
>
Re: Review Request 50202: [OOZIE-1978] Forkjoin validation code is
ridiculously slow in some cases
Posted by Attila Sasvari <as...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50202/#review143068
-----------------------------------------------------------
core/src/main/java/org/apache/oozie/workflow/lite/LiteWorkflowValidator.java (line 111)
<https://reviews.apache.org/r/50202/#comment208780>
can you move this declaration closer where it is used?
core/src/main/java/org/apache/oozie/workflow/lite/LiteWorkflowValidator.java (line 142)
<https://reviews.apache.org/r/50202/#comment208771>
This method does too many things / goes againts the Single responsibility principle.
Can you extract methods?
- Attila Sasvari
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
>
>
Re: Review Request 50202: [OOZIE-1978] Forkjoin validation code is
ridiculously slow in some cases
Posted by Peter Bacsko <pb...@cloudera.com>.
> On j�l. 21, 2016, 12:51 de, Robert Kanter wrote:
> > core/src/main/java/org/apache/oozie/workflow/lite/LiteWorkflowValidator.java, line 55
> > <https://reviews.apache.org/r/50202/diff/1/?file=1447527#file1447527line55>
> >
> > 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.
I will change this to a mini inner-class with two int fields.
> On j�l. 21, 2016, 12:51 de, Robert Kanter wrote:
> > core/src/main/java/org/apache/oozie/workflow/lite/LiteWorkflowValidator.java, line 164
> > <https://reviews.apache.org/r/50202/diff/1/?file=1447527#file1447527line164>
> >
> > 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 :)
Yes, it's something that was on my mind, I just completely forgot about it. I'll add enough comments to make things clearer.
> On j�l. 21, 2016, 12:51 de, Robert Kanter wrote:
> > core/src/main/java/org/apache/oozie/workflow/lite/LiteWorkflowValidator.java, line 252
> > <https://reviews.apache.org/r/50202/diff/1/?file=1447527#file1447527line252>
> >
> > 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());
> > ````
Good idea - will do it.
> On j�l. 21, 2016, 12:51 de, Robert Kanter wrote:
> > core/src/test/java/org/apache/oozie/workflow/lite/TestLiteWorkflowAppParser.java, line 18
> > <https://reviews.apache.org/r/50202/diff/1/?file=1447529#file1447529line18>
> >
> > 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).
Yes, will include that.
- Peter
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50202/#review143032
-----------------------------------------------------------
On j�l. 19, 2016, 7:56 du, Peter Bacsko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50202/
> -----------------------------------------------------------
>
> (Updated j�l. 19, 2016, 7:56 du)
>
>
> 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
>
>
Re: Review Request 50202: [OOZIE-1978] Forkjoin validation code is
ridiculously slow in some cases
Posted by Robert Kanter <rk...@cloudera.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 50202: [OOZIE-1978] Forkjoin validation code is
ridiculously slow in some cases
Posted by Robert Kanter <rk...@cloudera.com>.
> On July 20, 2016, 1:21 p.m., Peter Bacsko wrote:
> > core/src/main/java/org/apache/oozie/workflow/lite/LiteWorkflowValidator.java, line 158
> > <https://reviews.apache.org/r/50202/diff/1/?file=1447527#file1447527line158>
> >
> > Note: I know that this method has too many arguments.
> >
> > It doesn't look nice. A possible approach is to move some of the stuff (maps, app) to class variables. However I don't want to store state in this class - we only need these stuff during validation. Of course we could go ahead and re-create the maps & app variables during each validateWorkflow() call but I don't like that either.
> >
> > If someone has a good idea, don't hold it back.
I know that too many arguments is considered bad style according to some, but I have no problem with lots of arguments. The method needs them, so I see nothing wrong with it.
- Robert
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50202/#review142927
-----------------------------------------------------------
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
>
>
Re: Review Request 50202: [OOZIE-1978] Forkjoin validation code is
ridiculously slow in some cases
Posted by Peter Bacsko <pb...@cloudera.com>.
> On j�l. 20, 2016, 1:21 du, Peter Bacsko wrote:
> > core/src/main/java/org/apache/oozie/workflow/lite/LiteWorkflowValidator.java, line 158
> > <https://reviews.apache.org/r/50202/diff/1/?file=1447527#file1447527line158>
> >
> > Note: I know that this method has too many arguments.
> >
> > It doesn't look nice. A possible approach is to move some of the stuff (maps, app) to class variables. However I don't want to store state in this class - we only need these stuff during validation. Of course we could go ahead and re-create the maps & app variables during each validateWorkflow() call but I don't like that either.
> >
> > If someone has a good idea, don't hold it back.
>
> Robert Kanter wrote:
> I know that too many arguments is considered bad style according to some, but I have no problem with lots of arguments. The method needs them, so I see nothing wrong with it.
Ok, cool :)
- Peter
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50202/#review142927
-----------------------------------------------------------
On j�l. 23, 2016, 5:54 du, Peter Bacsko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50202/
> -----------------------------------------------------------
>
> (Updated j�l. 23, 2016, 5:54 du)
>
>
> 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/service/ActionService.java becc69b
> 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/TestWorkflowBean.java b68f0f7
> core/src/test/java/org/apache/oozie/command/wf/TestSubmitXCommand.java 73464c8
> core/src/test/java/org/apache/oozie/workflow/lite/TestLiteWorkflowAppParser.java 9002b6c
> core/src/test/resources/wf-long.xml PRE-CREATION
>
> 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
>
>
Re: Review Request 50202: [OOZIE-1978] Forkjoin validation code is
ridiculously slow in some cases
Posted by Peter Bacsko <pb...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50202/#review142927
-----------------------------------------------------------
core/src/main/java/org/apache/oozie/workflow/lite/LiteWorkflowValidator.java (line 158)
<https://reviews.apache.org/r/50202/#comment208599>
Note: I know that this method has too many arguments.
It doesn't look nice. A possible approach is to move some of the stuff (maps, app) to class variables. However I don't want to store state in this class - we only need these stuff during validation. Of course we could go ahead and re-create the maps & app variables during each validateWorkflow() call but I don't like that either.
If someone has a good idea, don't hold it back.
core/src/main/java/org/apache/oozie/workflow/lite/LiteWorkflowValidator.java (line 294)
<https://reviews.apache.org/r/50202/#comment208600>
I think I can get rid of this. Once the loop is detected, just add the node to the end of the deque and call the Joiner in-place.
core/src/test/java/org/apache/oozie/workflow/lite/TestLiteWorkflowAppParser.java (line 1380)
<https://reviews.apache.org/r/50202/#comment208601>
I'm no longer using E0732. This assumes that it's possible to decide which is the proper Join node for a given fork. I don't think it's possible to correctly decide this in the current implementation. So I dropped this and introduced two new codes.
- Peter Bacsko
On j�l. 19, 2016, 7:56 du, Peter Bacsko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50202/
> -----------------------------------------------------------
>
> (Updated j�l. 19, 2016, 7:56 du)
>
>
> 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
>
>
Re: Review Request 50202: [OOZIE-1978] Forkjoin validation code is
ridiculously slow in some cases
Posted by Peter Bacsko <pb...@cloudera.com>.
> On j�l. 20, 2016, 9:16 de, Abhishek Bafna wrote:
> > core/src/main/java/org/apache/oozie/workflow/lite/LiteWorkflowValidator.java, line 61
> > <https://reviews.apache.org/r/50202/diff/1/?file=1447527#file1447527line61>
> >
> > I think it would be a good idea to use equals() method instead of reference comparison (!= or ==).
I will change it to intValue() instead of getValue().
> On j�l. 20, 2016, 9:16 de, Abhishek Bafna wrote:
> > core/src/main/java/org/apache/oozie/workflow/lite/LiteWorkflowValidator.java, line 123
> > <https://reviews.apache.org/r/50202/diff/1/?file=1447527#file1447527line123>
> >
> > [minor] This null check seems to be redundant. While parsing the workflow.
I checked the existing code, seems like only my code throws E708 in this case.
Previously this existed in LiteWorkflowAppParser, but I moved that logic to here.
> On j�l. 20, 2016, 9:16 de, Abhishek Bafna wrote:
> > core/src/main/java/org/apache/oozie/workflow/lite/LiteWorkflowValidator.java, line 127
> > <https://reviews.apache.org/r/50202/diff/1/?file=1447527#file1447527line127>
> >
> > [minor]This seems redundant to me. We are doing this check while pasrsing the workflow. [LiteWorkflowApp#addNode]
> >
> > In case, we would like to keep the check, it would be good to repleace the (==) with String#equals() method.
Yepp, you're absosutely right. This check is not necessary.
- Peter
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50202/#review142917
-----------------------------------------------------------
On j�l. 19, 2016, 7:56 du, Peter Bacsko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50202/
> -----------------------------------------------------------
>
> (Updated j�l. 19, 2016, 7:56 du)
>
>
> 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
>
>
Re: Review Request 50202: [OOZIE-1978] Forkjoin validation code is
ridiculously slow in some cases
Posted by Abhishek Bafna <ba...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50202/#review142917
-----------------------------------------------------------
core/src/main/java/org/apache/oozie/workflow/lite/LiteWorkflowValidator.java (line 61)
<https://reviews.apache.org/r/50202/#comment208592>
I think it would be a good idea to use equals() method instead of reference comparison (!= or ==).
core/src/main/java/org/apache/oozie/workflow/lite/LiteWorkflowValidator.java (line 123)
<https://reviews.apache.org/r/50202/#comment208591>
[minor] This null check seems to be redundant. While parsing the workflow.
core/src/main/java/org/apache/oozie/workflow/lite/LiteWorkflowValidator.java (line 127)
<https://reviews.apache.org/r/50202/#comment208589>
[minor]This seems redundant to me. We are doing this check while pasrsing the workflow. [LiteWorkflowApp#addNode]
In case, we would like to keep the check, it would be good to repleace the (==) with String#equals() method.
- Abhishek Bafna
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
>
>