Bug 702109 - hibernate detach utility should skip constants (static final variables)
Summary: hibernate detach utility should skip constants (static final variables)
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: RHQ Project
Classification: Other
Component: Core Server
Version: 4.0.0
Hardware: All
OS: All
urgent
medium vote
Target Milestone: ---
: ---
Assignee: John Mazzitelli
QA Contact: Mike Foley
URL:
Whiteboard:
Depends On:
Blocks: jon3 rhq401 712192
TreeView+ depends on / blocked
 
Reported: 2011-05-04 19:19 UTC by John Mazzitelli
Modified: 2013-09-02 07:16 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2013-09-02 07:16:54 UTC


Attachments (Terms of Use)

Description John Mazzitelli 2011-05-04 19:19:46 UTC
In HibernateDetachUtility.nullOutFieldsByFieldAccess, we do this:

        Class tmpClass = object.getClass();
        List<Field> fieldsToClean = new ArrayList<Field>();
        while (tmpClass != null && tmpClass != Object.class) {
            Collections.addAll(fieldsToClean, tmpClass.getDeclaredFields());
            tmpClass = tmpClass.getSuperclass();
        }

We should be more selective about what goes into fieldsToClean. We should check each field with this:

if (java.lang.reflect.Modifier.isFinal(field.getModifiers()) &&
    java.lang.reflect.Modifier.isStatic(field.getModifiers()))
{
  .. this field is a static final - no need to cleanse this...
}

Its quite possible we should not cleanse pure "static" variables, whether or not they are final.  Need to think about that some more.

Comment 1 Jay Shaughnessy 2011-05-04 19:27:42 UTC
I think that's probably true, we're not passing back static fields defined in the class.

Also, this may be a decent area for a Map of Class to Fields, why inspect the class each time, we're always getting the same set of valid Fields, I think.

Comment 2 John Mazzitelli 2011-05-04 20:13:47 UTC
we should also skip "transient" fields as well

Comment 3 John Mazzitelli 2011-05-05 14:17:38 UTC
(In reply to comment #1)
> Also, this may be a decent area for a Map of Class to Fields, why inspect the
> class each time, we're always getting the same set of valid Fields, I think.

AFAIK, the JRE already does its own caching of this data. Earlier versions of the JRE did a poor job of this, but later versions have highly efficient reflection API.

Comment 4 John Mazzitelli 2011-05-05 14:20:20 UTC
(In reply to comment #3)
> (In reply to comment #1)
> > Also, this may be a decent area for a Map of Class to Fields, why inspect the
> > class each time, we're always getting the same set of valid Fields, I think.
> 
> AFAIK, the JRE already does its own caching of this data. Earlier versions of
> the JRE did a poor job of this, but later versions have highly efficient
> reflection API.

Now as I say that, since we are stripping out fields we don't want to process, we could cache only the fields we want to process. For now, I won't do that - but that could be a place for further optimization.

Comment 5 John Mazzitelli 2011-05-05 17:37:43 UTC
commit - 3e3e382

this is a code change/optimization. Nothing for QA to test.

Note that I changed the warning message that is spit out if the util recurses too deep:

            LOG.warn("Recursed too deep [" + depth + " > " + depthAllowed
                + "], will not attempt to detach object of type ["
                + ((value != null) ? value.getClass().getName() : "N/A")
                + "]. This may cause serialization errors later. If so, "
                + "you can try to work around this by setting the system property [" + DEPTH_ALLOWED_SYSPROP
                + "] to a value higher than [" + depth + "].");

Also note that we can now configure how deep we will allow this utility to go. This is in case we hit another bug like bug #702390 - we'll at least have a workaround. To configure the depth allowed, put this in rhq-server.properties:

rhq.server.hibernate-detach-utility.depth-allowed=100

where 100 can be any depth you want.  The default is 50.

Comment 6 Charles Crouch 2011-05-09 20:11:50 UTC
Added for triage for RHQ 4.0.1

Comment 7 John Mazzitelli 2011-05-10 16:39:15 UTC
cherry picked 3e3e382 to release-4.0.0 branch, commit 610b23bf0b88470c76727b81a6c9bc032ab3f147

Comment 8 Mike Foley 2011-05-11 21:40:58 UTC
internal refactoring.  expressly untestable in itself, as documented above.

Comment 9 Heiko W. Rupp 2013-09-02 07:16:54 UTC
Bulk closing of issues that were VERIFIED, had no target release and where the status changed more than a year ago.


Note You need to log in before you can comment on or make changes to this bug.