You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@oozie.apache.org by Kinga Marton via Review Board <no...@reviews.apache.org> on 2019/02/11 14:23:23 UTC
Re: Review Request 69783: OOZIE-3409 - Oozie Server : Memory leak in
EL evaluation
-----------------------------------------------------------
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
>
>
Re: Review Request 69783: OOZIE-3409 - Oozie Server : Memory leak in
EL evaluation
Posted by Andras Salamon <an...@melda.info>.
> On Feb. 11, 2019, 2:23 p.m., Kinga Marton wrote:
> > core/src/main/java/org/apache/oozie/util/ELEvaluator.java
> > Lines 205-209 (original), 210-214 (patched)
> > <https://reviews.apache.org/r/69783/diff/2/?file=2122831#file2122831line211>
> >
> > 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?
We have discussed it in person but I write it down, maybe it will be useful later. The method can throw the checked javax.servlet.jsp.el.ELException but in the real life it will throw a RuntimeException which is very confusedly named javax.el.ELException. So we really need to catch that one. It will be better to catch RuntimeException instead to prepare for other RuntimeExceptions.
- 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
>
>
Re: Review Request 69783: OOZIE-3409 - Oozie Server : Memory leak in
EL evaluation
Posted by Andras Salamon <an...@melda.info>.
> On Feb. 11, 2019, 2:23 p.m., Kinga Marton wrote:
> > core/src/main/java/org/apache/oozie/util/ELEvaluator.java
> > Lines 148 (patched)
> > <https://reviews.apache.org/r/69783/diff/2/?file=2122831#file2122831line149>
> >
> > org.apache.jasper.el.ExpressionEvaluatorImpl is deprecated. Does it have a replacement? If yes, let's use the replacement instead of the deprecated class.
I found this info: Description copied from class: javax.servlet.jsp.el.ExpressionEvaluator
In that class the following info: As of JSP 2.1, replaced by javax.el.ExpressionFactory
But ExpressionFactory has no parseExpression method.
So i don't really know which non-deprecated class we should use. Jetty 9.3 also uses these classes, so I don't think it's a bug problem.
Maybe when we upgrade Jetty, JSP at the same time, we should also update apache-jsp and revisit this issue.
- 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
>
>
Re: Review Request 69783: OOZIE-3409 - Oozie Server : Memory leak in
EL evaluation
Posted by Andras Salamon <an...@melda.info>.
> 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?
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.
- 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
>
>
Re: Review Request 69783: OOZIE-3409 - Oozie Server : Memory leak in
EL evaluation
Posted by Andras Salamon <an...@melda.info>.
> 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
>
>