You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@oozie.apache.org by Mona Chitnis <mo...@yahoo.in> on 2013/01/24 02:59:50 UTC
Review Request: OOZIE-1186 Image load for Job DAG visualization should
handle resources better
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9079/
-----------------------------------------------------------
Review request for oozie.
Description
-------
https://issues.apache.org/jira/browse/OOZIE-1186
This addresses bug OOZIE-1186.
https://issues.apache.org/jira/browse/OOZIE-1186
Diffs
-----
trunk/core/src/main/java/org/apache/oozie/servlet/V1JobServlet.java 1437616
trunk/core/src/main/java/org/apache/oozie/util/GraphGenerator.java 1437616
trunk/core/src/test/java/org/apache/oozie/util/TestGraphGenerator.java 1437616
Diff: https://reviews.apache.org/r/9079/diff/
Testing
-------
unit test done + end-to-end using Yourkit memory profiler
Thanks,
Mona Chitnis
Re: Review Request: OOZIE-1186 Image load for Job DAG visualization should
handle resources better
Posted by Virag Kothari <vi...@yahoo-inc.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9079/#review15679
-----------------------------------------------------------
Ship it!
+1..Good to know about dispose(). It seems we misread the javadoc. It was probably saying that if we override the paint(Graphics g) of Component, g.dispose() shouldn't be called within the method as the graphics object was not created by us
trunk/core/src/test/java/org/apache/oozie/util/TestGraphGenerator.java
<https://reviews.apache.org/r/9079/#comment33823>
remove println statement
- Virag Kothari
On Jan. 25, 2013, 3:46 a.m., Mona Chitnis wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9079/
> -----------------------------------------------------------
>
> (Updated Jan. 25, 2013, 3:46 a.m.)
>
>
> Review request for oozie.
>
>
> Description
> -------
>
> https://issues.apache.org/jira/browse/OOZIE-1186
>
>
> This addresses bug OOZIE-1186.
> https://issues.apache.org/jira/browse/OOZIE-1186
>
>
> Diffs
> -----
>
> trunk/core/src/main/java/org/apache/oozie/util/GraphGenerator.java 1437616
> trunk/core/src/test/java/org/apache/oozie/util/TestGraphGenerator.java 1437616
>
> Diff: https://reviews.apache.org/r/9079/diff/
>
>
> Testing
> -------
>
> unit test done + end-to-end using Yourkit memory profiler
>
>
> Thanks,
>
> Mona Chitnis
>
>
Re: Review Request: OOZIE-1186 Image load for Job DAG visualization should
handle resources better
Posted by Mona Chitnis <mo...@yahoo.in>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9079/
-----------------------------------------------------------
(Updated Jan. 25, 2013, 3:46 a.m.)
Review request for oozie.
Changes
-------
To understand the utility of using Graphics.dispose(), I studied some of this code in detail. Here's some data to back the changes in the patch.
Although, the javadocs indicate that we do not need to invoke g.dispose() explicitly, looking carefully into the Component class's paint() implementation, note how it only disposes the new graphics object it creates (co) using the Graphics context and not the original one (g).
http://javasourcecode.org/html/open-source/jdk/jdk-6u23/javax/swing/JComponent.java.html
(lines 967-1053)
bq
public void paint(Graphics g) {
boolean shouldClearPaintFlags = false;
if ((getWidth() <= 0) || (getHeight() <= 0)) {
return;
}
Graphics componentGraphics = getComponentGraphics(g);
Graphics co = componentGraphics.create();
....
finally {
co.dispose();
bq.
Therefore, in our GraphGenerator code, as we are creating a new Graphics object, we alone are responsible for disposing it. The superimplementation of paint() in the JUNG library's classes e.g.VisualizationImageServer are not seen to override paint() in a way to do dispose() either.
The heap memory profiling clearly shows the changes with and w/o the dispose line. Without it, heap occupied keeps monotonically rising with repeated job requests, dangerously in the ballpark of GBs, till GC hits. With patch, only spikes with each request but returns to low immediately and stays in MBs.
Excluded the unit test as it was flaky between different run environments. But applying formatter introduced few changes to that file. Other review comments addressed.
Description
-------
https://issues.apache.org/jira/browse/OOZIE-1186
This addresses bug OOZIE-1186.
https://issues.apache.org/jira/browse/OOZIE-1186
Diffs (updated)
-----
trunk/core/src/main/java/org/apache/oozie/util/GraphGenerator.java 1437616
trunk/core/src/test/java/org/apache/oozie/util/TestGraphGenerator.java 1437616
Diff: https://reviews.apache.org/r/9079/diff/
Testing
-------
unit test done + end-to-end using Yourkit memory profiler
Thanks,
Mona Chitnis
Re: Review Request: OOZIE-1186 Image load for Job DAG visualization should
handle resources better
Posted by Mona Chitnis <mo...@yahoo.in>.
> On Jan. 24, 2013, 8:22 p.m., Virag Kothari wrote:
> > trunk/core/src/test/java/org/apache/oozie/util/TestGraphGenerator.java, line 113
> > <https://reviews.apache.org/r/9079/diff/1/?file=251438#file251438line113>
> >
> > The test case fails/passes inconsistently.
> > I also tried with thread.sleep() to allow some time for the memory to be freed but doesn't work.
> > System.gc() is just an indicator for garbage collection and wont help
> > Its ok to not have a testcase as its difficult to write a unit test for a patch which is just flushing system resources and not solving a memory leak issue.
> >
commenting the testcase. Letting code be there for reference
- Mona
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9079/#review15651
-----------------------------------------------------------
On Jan. 24, 2013, 1:59 a.m., Mona Chitnis wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9079/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2013, 1:59 a.m.)
>
>
> Review request for oozie.
>
>
> Description
> -------
>
> https://issues.apache.org/jira/browse/OOZIE-1186
>
>
> This addresses bug OOZIE-1186.
> https://issues.apache.org/jira/browse/OOZIE-1186
>
>
> Diffs
> -----
>
> trunk/core/src/main/java/org/apache/oozie/servlet/V1JobServlet.java 1437616
> trunk/core/src/main/java/org/apache/oozie/util/GraphGenerator.java 1437616
> trunk/core/src/test/java/org/apache/oozie/util/TestGraphGenerator.java 1437616
>
> Diff: https://reviews.apache.org/r/9079/diff/
>
>
> Testing
> -------
>
> unit test done + end-to-end using Yourkit memory profiler
>
>
> Thanks,
>
> Mona Chitnis
>
>
Re: Review Request: OOZIE-1186 Image load for Job DAG visualization should
handle resources better
Posted by Virag Kothari <vi...@yahoo-inc.com>.
> On Jan. 24, 2013, 8:22 p.m., Virag Kothari wrote:
> > trunk/core/src/test/java/org/apache/oozie/util/TestGraphGenerator.java, line 113
> > <https://reviews.apache.org/r/9079/diff/1/?file=251438#file251438line113>
> >
> > The test case fails/passes inconsistently.
> > I also tried with thread.sleep() to allow some time for the memory to be freed but doesn't work.
> > System.gc() is just an indicator for garbage collection and wont help
> > Its ok to not have a testcase as its difficult to write a unit test for a patch which is just flushing system resources and not solving a memory leak issue.
> >
>
> Mona Chitnis wrote:
> commenting the testcase. Letting code be there for reference
Mohammad had once told me not to have commented code. You can put a TODO comment or a note for reference
- Virag
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9079/#review15651
-----------------------------------------------------------
On Jan. 24, 2013, 1:59 a.m., Mona Chitnis wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9079/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2013, 1:59 a.m.)
>
>
> Review request for oozie.
>
>
> Description
> -------
>
> https://issues.apache.org/jira/browse/OOZIE-1186
>
>
> This addresses bug OOZIE-1186.
> https://issues.apache.org/jira/browse/OOZIE-1186
>
>
> Diffs
> -----
>
> trunk/core/src/main/java/org/apache/oozie/servlet/V1JobServlet.java 1437616
> trunk/core/src/main/java/org/apache/oozie/util/GraphGenerator.java 1437616
> trunk/core/src/test/java/org/apache/oozie/util/TestGraphGenerator.java 1437616
>
> Diff: https://reviews.apache.org/r/9079/diff/
>
>
> Testing
> -------
>
> unit test done + end-to-end using Yourkit memory profiler
>
>
> Thanks,
>
> Mona Chitnis
>
>
Re: Review Request: OOZIE-1186 Image load for Job DAG visualization should
handle resources better
Posted by Virag Kothari <vi...@yahoo-inc.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9079/#review15651
-----------------------------------------------------------
trunk/core/src/main/java/org/apache/oozie/servlet/V1JobServlet.java
<https://reviews.apache.org/r/9079/#comment33767>
Test case is not required. We can remove the extra boolean parameter if it was just for testcase.
trunk/core/src/main/java/org/apache/oozie/util/GraphGenerator.java
<https://reviews.apache.org/r/9079/#comment33755>
not required
trunk/core/src/test/java/org/apache/oozie/util/TestGraphGenerator.java
<https://reviews.apache.org/r/9079/#comment33763>
The test case fails/passes inconsistently.
I also tried with thread.sleep() to allow some time for the memory to be freed but doesn't work.
System.gc() is just an indicator for garbage collection and wont help
Its ok to not have a testcase as its difficult to write a unit test for a patch which is just flushing system resources and not solving a memory leak issue.
- Virag Kothari
On Jan. 24, 2013, 1:59 a.m., Mona Chitnis wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9079/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2013, 1:59 a.m.)
>
>
> Review request for oozie.
>
>
> Description
> -------
>
> https://issues.apache.org/jira/browse/OOZIE-1186
>
>
> This addresses bug OOZIE-1186.
> https://issues.apache.org/jira/browse/OOZIE-1186
>
>
> Diffs
> -----
>
> trunk/core/src/main/java/org/apache/oozie/servlet/V1JobServlet.java 1437616
> trunk/core/src/main/java/org/apache/oozie/util/GraphGenerator.java 1437616
> trunk/core/src/test/java/org/apache/oozie/util/TestGraphGenerator.java 1437616
>
> Diff: https://reviews.apache.org/r/9079/diff/
>
>
> Testing
> -------
>
> unit test done + end-to-end using Yourkit memory profiler
>
>
> Thanks,
>
> Mona Chitnis
>
>
Re: Review Request: OOZIE-1186 Image load for Job DAG visualization should
handle resources better
Posted by Rohini Palaniswamy <ro...@gmail.com>.
> On Jan. 24, 2013, 5:33 p.m., Rohini Palaniswamy wrote:
> > trunk/core/src/main/java/org/apache/oozie/util/GraphGenerator.java, line 263
> > <https://reviews.apache.org/r/9079/diff/1/?file=251437#file251437line263>
> >
> > Can we also add a out = null after close.
> >
> > http://wiki.apache.org/tomcat/FAQ/KnownIssues#ImageIOIssues
Actually you need to do the out.close() and out = null in finally also.
- Rohini
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9079/#review15647
-----------------------------------------------------------
On Jan. 24, 2013, 1:59 a.m., Mona Chitnis wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9079/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2013, 1:59 a.m.)
>
>
> Review request for oozie.
>
>
> Description
> -------
>
> https://issues.apache.org/jira/browse/OOZIE-1186
>
>
> This addresses bug OOZIE-1186.
> https://issues.apache.org/jira/browse/OOZIE-1186
>
>
> Diffs
> -----
>
> trunk/core/src/main/java/org/apache/oozie/servlet/V1JobServlet.java 1437616
> trunk/core/src/main/java/org/apache/oozie/util/GraphGenerator.java 1437616
> trunk/core/src/test/java/org/apache/oozie/util/TestGraphGenerator.java 1437616
>
> Diff: https://reviews.apache.org/r/9079/diff/
>
>
> Testing
> -------
>
> unit test done + end-to-end using Yourkit memory profiler
>
>
> Thanks,
>
> Mona Chitnis
>
>
Re: Review Request: OOZIE-1186 Image load for Job DAG visualization should
handle resources better
Posted by Rohini Palaniswamy <ro...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9079/#review15647
-----------------------------------------------------------
Looks good. Just need one minor change to deal with java bug.
trunk/core/src/main/java/org/apache/oozie/util/GraphGenerator.java
<https://reviews.apache.org/r/9079/#comment33747>
Can we also add a out = null after close.
http://wiki.apache.org/tomcat/FAQ/KnownIssues#ImageIOIssues
- Rohini Palaniswamy
On Jan. 24, 2013, 1:59 a.m., Mona Chitnis wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9079/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2013, 1:59 a.m.)
>
>
> Review request for oozie.
>
>
> Description
> -------
>
> https://issues.apache.org/jira/browse/OOZIE-1186
>
>
> This addresses bug OOZIE-1186.
> https://issues.apache.org/jira/browse/OOZIE-1186
>
>
> Diffs
> -----
>
> trunk/core/src/main/java/org/apache/oozie/servlet/V1JobServlet.java 1437616
> trunk/core/src/main/java/org/apache/oozie/util/GraphGenerator.java 1437616
> trunk/core/src/test/java/org/apache/oozie/util/TestGraphGenerator.java 1437616
>
> Diff: https://reviews.apache.org/r/9079/diff/
>
>
> Testing
> -------
>
> unit test done + end-to-end using Yourkit memory profiler
>
>
> Thanks,
>
> Mona Chitnis
>
>