> On Feb. 11, 2019, 2:23 p.m., Kinga Marton wrote: > > core/src/main/java/org/apache/oozie/command/coord/CoordSubmitXCommand.java > > Lines 450-461 (original) > > <https://reviews.apache.org/r/69783/diff/2/?file=2122830#file2122830line451> > > > > 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? > > Andras Salamon wrote: > In the old code evaluator.parseExpressionString was throwing ELException. > checkForExistence was throwing the cause of it, which was some kind of > Exception. handleELParseException was creating a CoordinatorJobException in > this case. > > In the new code StringUtils.checkStaticExistence does not throw any > exception, so isInvalid will be true in this case, and the next check ( > handleExpresionWith* ) will throw a very similar CoordinatorJobException. I > think the error code will be the same, the error message might be different.
I've added back exception throwing if the expression is invalid, so I put back this code. > On Feb. 11, 2019, 2:23 p.m., Kinga Marton wrote: > > core/src/main/java/org/apache/oozie/util/ELEvaluator.java > > Lines 231-234 (original) > > <https://reviews.apache.org/r/69783/diff/2/?file=2122831#file2122831line237> > > > > org.apache.jasper.el.ExpressionEvaluatorImpl has as well a > > parseExpression method. That method is not usable here instead of string > > processing? That would also require a function mapper. In the old code it was possible to tokenize it without that. > On Feb. 11, 2019, 2:23 p.m., Kinga Marton wrote: > > core/src/main/java/org/apache/oozie/util/StringUtils.java > > Lines 66 (patched) > > <https://reviews.apache.org/r/69783/diff/2/?file=2122832#file2122832line66> > > > > before entering the while, we can return false if the expr doesn't > > contain the sequence at all. I've added this code (a good speedup). But later I introduced the exception throwing if the expression is invalid so I deleted this code. > On Feb. 11, 2019, 2:23 p.m., Kinga Marton wrote: > > core/src/main/java/org/apache/oozie/util/StringUtils.java > > Lines 91 (patched) > > <https://reviews.apache.org/r/69783/diff/2/?file=2122832#file2122832line91> > > > > Shouldn't we handle somehow the not valid expressions as well? Great idea, I'll throw an Exception if the expression is not valid, and I've added test cases. > On Feb. 11, 2019, 2:23 p.m., Kinga Marton wrote: > > core/src/test/java/org/apache/oozie/util/TestStringUtils.java > > Lines 73 (patched) > > <https://reviews.apache.org/r/69783/diff/2/?file=2122835#file2122835line73> > > > > Before the fix this case would have thrown an Exception. I've introduced exception throwing, so I fixed this example. - Andras ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69783/#review212604 ----------------------------------------------------------- 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 > >
