----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/62352/#review186944 -----------------------------------------------------------
core/src/main/java/org/apache/oozie/util/graph/GraphvizRenderer.java Lines 30 (patched) <https://reviews.apache.org/r/62352/#comment263856> Import core/src/main/java/org/apache/oozie/util/graph/GraphvizRenderer.java Lines 60 (patched) <https://reviews.apache.org/r/62352/#comment263857> I would rather call this getShape(). The word "calculate" is telling me that we do some math as well. core/src/main/java/org/apache/oozie/util/graph/GraphvizRenderer.java Lines 90 (patched) <https://reviews.apache.org/r/62352/#comment263858> getColor() core/src/main/java/org/apache/oozie/util/graph/GraphvizRenderer.java Lines 125 (patched) <https://reviews.apache.org/r/62352/#comment263859> What do we "ensure" here? To me this feels like another get() method with some extra logic. core/src/main/java/org/apache/oozie/util/graph/GraphvizRenderer.java Lines 170-173 (patched) <https://reviews.apache.org/r/62352/#comment263860> To me this looks weird. We have a single threaded executor and we wait without any kind of timeout. What does this method accomplish by rendering the PNG like that? core/src/main/java/org/apache/oozie/util/graph/WorkflowActionNode.java Lines 27 (patched) <https://reviews.apache.org/r/62352/#comment263861> We store this class in a hashmap and there is no hashCode() and equals() defined for this class. Could you generate them? core/src/main/java/org/apache/oozie/util/graph/WorkflowGraphHandler.java Lines 61 (patched) <https://reviews.apache.org/r/62352/#comment263863> Could you just add a short comment like "// nop" if the method body is intentionally empty? core/src/main/java/org/apache/oozie/util/graph/WorkflowGraphHandler.java Lines 92-100 (patched) <https://reviews.apache.org/r/62352/#comment263862> Can we live without "continue"? If we can, just remove it and put this block under an "if (child != null)" branch. core/src/main/java/org/apache/oozie/util/graph/WorkflowGraphHandler.java Lines 161 (patched) <https://reviews.apache.org/r/62352/#comment263864> Why do we have to explicitly null this out? Is this class ever reused? Can't see a setter for "out". I'm also concerned about closing the output stream here. This should be the caller's responsibility. core/src/test/java/org/apache/oozie/util/graph/TestGraphGenerator.java Lines 83 (patched) <https://reviews.apache.org/r/62352/#comment263867> Please also print the stack trace to improve debuggability core/src/test/java/org/apache/oozie/util/graph/TestGraphGenerator.java Lines 96 (patched) <https://reviews.apache.org/r/62352/#comment263866> Please also print the stack trace to improve debuggability core/src/test/java/org/apache/oozie/util/graph/TestGraphGenerator.java Lines 111 (patched) <https://reviews.apache.org/r/62352/#comment263865> Please also print the stack trace to improve debuggability - Peter Bacsko On okt. 2, 2017, 9:04 du, András Piros wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/62352/ > ----------------------------------------------------------- > > (Updated okt. 2, 2017, 9:04 du) > > > Review request for oozie and Robert Kanter. > > > Repository: oozie-git > > > Description > ------- > > OOZIE-2406 Completely rewrite GraphGenerator code > > > Diffs > ----- > > client/src/main/java/org/apache/oozie/client/rest/RestConstants.java > 74843dc9a3cbb8cef38e97f81e34731f191d9aab > core/pom.xml 6f9adb66af9344ac7d2212cdc31aa203ec06c286 > core/src/main/java/org/apache/oozie/servlet/JsonRestServlet.java > 059d3cf6dc251b49940af29d82cbdd817043a176 > core/src/main/java/org/apache/oozie/servlet/V1JobServlet.java > 10812c675ebc0cc4aefca9f4a85ef2fc26d143e1 > core/src/main/java/org/apache/oozie/util/GraphGenerator.java > 6ded2c6dc15c9e8453ff800407ff0324be185f41 > core/src/main/java/org/apache/oozie/util/graph/GraphGenerator.java > PRE-CREATION > core/src/main/java/org/apache/oozie/util/graph/GraphRenderer.java > PRE-CREATION > core/src/main/java/org/apache/oozie/util/graph/GraphvizRenderer.java > PRE-CREATION > core/src/main/java/org/apache/oozie/util/graph/OutputFormat.java > PRE-CREATION > core/src/main/java/org/apache/oozie/util/graph/WorkflowActionNode.java > PRE-CREATION > core/src/main/java/org/apache/oozie/util/graph/WorkflowGraphHandler.java > PRE-CREATION > core/src/test/java/org/apache/oozie/servlet/TestV1JobServlet.java > ee9ab556c9b6930c406ca5dcd54065cbbebb86c9 > core/src/test/java/org/apache/oozie/util/TestGraphGenerator.java > 002e925b57cd830ea6d83a87cea4383165116b80 > core/src/test/java/org/apache/oozie/util/graph/TestGraphGenerator.java > PRE-CREATION > core/src/test/resources/graphWF.xml > 6a7b0427a9951835a7533a04b66258ded369d5bf > core/src/test/resources/graphWF_26_actions.xml > a091be0f3559ede195ccc3339adee4478a8da8c0 > core/src/test/resources/graphWF_50_actions.xml PRE-CREATION > core/src/test/resources/graphWF_decision_fork_join.xml PRE-CREATION > docs/src/site/twiki/WebServicesAPI.twiki > ef3e60242512decd48beb3d8c9ac747b7d128eda > examples/src/main/apps/java-main/workflow.xml > 98e01ca92187e1c567686f6c2b4f689bb2a5ef6a > pom.xml 0b94484da1c97618e9168cea0ebbfff7f70f723c > webapp/src/main/webapp/oozie-console.js > 72c8a198a4ffe60f74a9f700831f65efcb3066c4 > > > Diff: https://reviews.apache.org/r/62352/diff/5/ > > > Testing > ------- > > `TestGraphGenerator`, `TestV1JobServlet` > > > Thanks, > > András Piros > >
