----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70502/#review214749 -----------------------------------------------------------
core/src/main/java/org/apache/oozie/DagEngine.java Line 304 (original), 306 (patched) <https://reviews.apache.org/r/70502/#comment300973> Could we avoid hardwiring the default value here as suggested by OOZIE-3462? core/src/main/java/org/apache/oozie/DagEngine.java Lines 305-306 (original), 310-311 (patched) <https://reviews.apache.org/r/70502/#comment300974> This code is almost duplicated in the next section, only the error code is different. I might extract it to a method. core/src/main/java/org/apache/oozie/DagEngine.java Lines 309-310 (original), 314-315 (patched) <https://reviews.apache.org/r/70502/#comment300975> Duplicated code core/src/test/java/org/apache/oozie/TestDagEngine.java Line 32 (original), 32-34 (patched) <https://reviews.apache.org/r/70502/#comment300976> Do we need these new imports? core/src/test/java/org/apache/oozie/TestDagEngine.java Line 169 (original), 173 (patched) <https://reviews.apache.org/r/70502/#comment300984> First I wanted to ask about the missing testGetJobs. After that I realised that the non-dead part of the method has no asserts in it, so I thought that it's useless. Now I don't think that it's totally useless, because the engine.submitJob might throw a DagException, so this methods tests if it correctly runs without and exception. Could you please check this? core/src/test/java/org/apache/oozie/TestDagEngine.java Line 170 (original), 174 (patched) <https://reviews.apache.org/r/70502/#comment300989> I like this setupRerunConfig method, it makes it easier to create lots of similar test methods. But I don't really like calling with "(null, false, null)" parameters. Maybe using well-named variables might help to understand it. What do you think? This applies to the other calls of course. core/src/test/java/org/apache/oozie/TestDagEngine.java Lines 178 (patched) <https://reviews.apache.org/r/70502/#comment300977> Please break line core/src/test/java/org/apache/oozie/TestDagEngine.java Lines 179 (patched) <https://reviews.apache.org/r/70502/#comment300980> Do we need the "Assert." part? It should work without that. (Same applies for all the asserts). core/src/test/java/org/apache/oozie/TestDagEngine.java Line 175 (original), 184 (patched) <https://reviews.apache.org/r/70502/#comment300987> typo: NOdes instead of Nodes core/src/test/java/org/apache/oozie/TestDagEngine.java Lines 189 (patched) <https://reviews.apache.org/r/70502/#comment300978> Please break line core/src/test/java/org/apache/oozie/TestDagEngine.java Line 180 (original), 196 (patched) <https://reviews.apache.org/r/70502/#comment300986> typo: NOdes instead of Nodes core/src/test/java/org/apache/oozie/TestDagEngine.java Lines 201 (patched) <https://reviews.apache.org/r/70502/#comment300979> Please break line core/src/test/java/org/apache/oozie/TestDagEngine.java Line 185 (original), 208 (patched) <https://reviews.apache.org/r/70502/#comment300985> typo: NOdes instead of Nodes core/src/test/java/org/apache/oozie/TestDagEngine.java Line 188 (original), 211 (patched) <https://reviews.apache.org/r/70502/#comment300988> It's a bit strange that we don't have any asserts here, but it's ok. core/src/test/java/org/apache/oozie/TestDagEngine.java Line 213 (original), 213 (patched) <https://reviews.apache.org/r/70502/#comment300983> typo: NOdes instead of Nodes core/src/test/java/org/apache/oozie/TestDagEngine.java Line 220 (original), 218 (patched) <https://reviews.apache.org/r/70502/#comment300982> typo: NOdes instead of Nodes core/src/test/java/org/apache/oozie/TestDagEngine.java Line 225 (original), 223 (patched) <https://reviews.apache.org/r/70502/#comment300981> Please break line - Andras Salamon On April 18, 2019, 11:56 a.m., Kinga Marton wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70502/ > ----------------------------------------------------------- > > (Updated April 18, 2019, 11:56 a.m.) > > > Review request for oozie and Andras Salamon. > > > Repository: oozie-git > > > Description > ------- > > Currently when you re-run a workflow with property "oozie.wf.rerun.failnodes" > set to true, > > you can no longer re-run it again with "oozie.wf.rerun.skip.nodes" property > specified, even if you set "oozie.wf.rerun.failnodes" to false. > > This kind of limitation is not reasonable. There is only one case where > "oozie.wf.rerun.failnodes" is true and "oozie.wf.rerun.skip.nodes" is not > null or empty, that should be disallowed. > > > Diffs > ----- > > core/src/main/java/org/apache/oozie/DagEngine.java 70ce96122 > core/src/test/java/org/apache/oozie/TestDagEngine.java 15f86403f > > > Diff: https://reviews.apache.org/r/70502/diff/1/ > > > Testing > ------- > > unit tests added > > > Thanks, > > Kinga Marton > >
