----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69783/#review212604 -----------------------------------------------------------
core/src/main/java/org/apache/oozie/command/coord/CoordSubmitXCommand.java Lines 450-461 (original) <https://reviews.apache.org/r/69783/#comment298573> Before the patch in case of an Exception it was converted to a CoordinatorJobException, but now, this conversion is missing. This can lead to some kind of functional changes, what we should avoid. No? core/src/main/java/org/apache/oozie/util/ELEvaluator.java Lines 148 (patched) <https://reviews.apache.org/r/69783/#comment298568> org.apache.jasper.el.ExpressionEvaluatorImpl is deprecated. Does it have a replacement? If yes, let's use the replacement instead of the deprecated class. core/src/main/java/org/apache/oozie/util/ELEvaluator.java Lines 205-209 (original), 210-214 (patched) <https://reviews.apache.org/r/69783/#comment298569> As I can see the evaluator.evaluate can throw javax.servlet.jsp.el.ELException, but in the catch part you are checking if the thrown exception is javax.el.ELException. Is this a bug or I am missing something? core/src/main/java/org/apache/oozie/util/ELEvaluator.java Lines 231-234 (original) <https://reviews.apache.org/r/69783/#comment298570> org.apache.jasper.el.ExpressionEvaluatorImpl has as well a parseExpression method. That method is not usable here instead of string processing? core/src/main/java/org/apache/oozie/util/StringUtils.java Lines 62 (patched) <https://reviews.apache.org/r/69783/#comment298572> Is not possible to use org.apache.jasper.el.ExpressionEvaluatorImpl's parseExpression method instead of this String processing? core/src/main/java/org/apache/oozie/util/StringUtils.java Lines 66 (patched) <https://reviews.apache.org/r/69783/#comment298571> before entering the while, we can return false if the expr doesn't contain the sequence at all. core/src/main/java/org/apache/oozie/util/StringUtils.java Lines 91 (patched) <https://reviews.apache.org/r/69783/#comment298574> Shouldn't we handle somehow the not valid expressions as well? core/src/test/java/org/apache/oozie/util/TestStringUtils.java Lines 73 (patched) <https://reviews.apache.org/r/69783/#comment298575> Before the fix this case would have thrown an Exception. - Kinga Marton On Jan. 28, 2019, 7:32 a.m., Andras Salamon wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69783/ > ----------------------------------------------------------- > > (Updated Jan. 28, 2019, 7:32 a.m.) > > > Review request for oozie, András Piros and Kinga Marton. > > > Repository: oozie-git > > > Description > ------- > > OOZIE-3409 - Oozie Server : Memory leak in EL evaluation > > > Diffs > ----- > > core/pom.xml b6c07d345 > core/src/main/java/org/apache/oozie/command/coord/CoordSubmitXCommand.java > c1c644459 > core/src/main/java/org/apache/oozie/util/ELEvaluator.java 90d79779f > core/src/main/java/org/apache/oozie/util/StringUtils.java 26079be93 > core/src/test/java/org/apache/oozie/command/wf/TestActionErrors.java > 519b38406 > core/src/test/java/org/apache/oozie/util/TestELEvaluator.java 04a869c0b > core/src/test/java/org/apache/oozie/util/TestStringUtils.java 423d6be27 > examples/pom.xml 8900535b2 > pom.xml 93fffc791 > > > Diff: https://reviews.apache.org/r/69783/diff/2/ > > > Testing > ------- > > Using grind to execute unit tests. > Using CDH cluster to create lots of dynamic workflows. > > > Thanks, > > Andras Salamon > >
