code review

Ian Springer ian.springer at redhat.com
Wed Jul 27 19:09:11 UTC 2011


Looks good to me.

One thing I'd change in all of the try-finally blocks is to catch 
RuntimeException when calling augeasComponent.close() and log an error 
rather than letting such an exception bubble up to the PC and causing it 
to consider the facet call (e.g. loadResourceconfiguration()) to have 
failed. e.g.:

} finally {
    if (comp != null) {
       try {
          comp.close();
       } catch (RuntimeException re) {
           LOG.error("Failed to close Augeas component.", re);
       }
    }
}

And one other nit-pick in AugeasRawConfigHelper.java 
<http://git.fedorahosted.org/git?p=rhq/rhq.git;a=blob;f=modules/plugins/augeas/src/main/java/org/rhq/plugins/augeas/helper/AugeasRawConfigHelper.java;h=d2003ca76d5aede032d7aeccea1de486061309c5;hb=4cfd2eb0b6f12bb9e4cd77862a0aa0e2292580e2>:

} catch (Exception e) {
+            if (aug != null) {
+                try {
+                    aug.close();
+                } catch (Exception e2) {
+                }
+                aug = null;
+            }
+            throw e;
+        }

No need to set augeas to null if you're going to throw an exception on 
the following line.



On 07/27/2011 11:50 AM, Filip Drabek wrote:
> Hi,
> as you know there is a problem with memory leak in augeas based 
> plugins. (https://bugzilla.redhat.com/show_bug.cgi?id=721152)
> I would like to ask you for code review of my changes in augeas based 
> plugins. The code is in branch release-3.0.0
> commit 4cfd2eb0b6f12bb9e4cd77862a0aa0e2292580e2 and 
> b787f3937f7618bfc83aad8bbc4e4ed67eb97b3e
> The problem was that after initialization of augeas we were calling 
> augeas.load() to load the configuration files, but
> we were not calling method augeas.close() to release the memory in heap.
>
> The solution is:
>  - added method finalize where we can close the augeas.
>  - once augeas is initialized and the method load() is called multiple 
> times the memory usage is still growing.
>    So we can not reuse the augeas instance and we need to call method 
> close() after each augeas use (started by method load())
>
> I have tried more solutions like new augeas components or implementing 
> inner classes for each use of augeas, but in the end
> the code was always much worse than in this implemented solution.
>
> Thanks
> Filip
>
>
>
>
>
>
> _______________________________________________
> rhq-devel mailing list
> rhq-devel at lists.fedorahosted.org
> https://fedorahosted.org/mailman/listinfo/rhq-devel


-- 
Ian Springer
Principal Software Developer
JBoss Operations Network
Red Hat
ian.springer at redhat.com

-------------- next part --------------
An HTML attachment was scrubbed...
URL: https://fedorahosted.org/pipermail/rhq-devel/attachments/20110727/96508f1e/attachment.html 


More information about the rhq-devel mailing list