OpenJPA
  1. OpenJPA
  2. OPENJPA-370

LoadFetchGroup annotation was not recognized during the fetch1

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Invalid
    • Affects Version/s: 1.0.1, 1.0.2, 1.1.0
    • Fix Version/s: 1.0.2, 1.1.0
    • Component/s: kernel
    • Labels:
      None

      Description

      Employee class has a LoadFetchGroup annotation defined on the Rating field, when getRating was called, the address should be returned also. However, openjpa did not handle the LoadFetchGroup correctly, therefore, address was not eargly fetched.
      public class FGEmployee{
      @Id
      private int id;

      @OneToOne(fetch=FetchType.LAZY)
      private FGAddress address;

      @Basic(fetch=FetchType.LAZY)
      @LoadFetchGroup("AddressFetchGroup")
      private String rating;

      @ManyToOne(fetch=FetchType.LAZY)
      private FGManager manager;

      ..
      }

      1. TestFetchGroup.patch
        29 kB
        Teresa Kan
      2. ASF.LICENSE.NOT.GRANTED--smime.p7s
        2 kB
        Craig L Russell
      3. TestJIRA370.zip
        3 kB
        Pinaki Poddar
      4. OPENJPA_370_2.patch
        10 kB
        Teresa Kan
      5. TestFetchGroup.zip
        8 kB
        Teresa Kan

        Issue Links

          Activity

          Hide
          Teresa Kan added a comment -

          Attach the patch. See the following changes:
          Index: openjpa-kernel/src/main/java/org/apache/openjpa/kernel/DelegatingFetchConfiguration.java
          ===================================================================
          — openjpa-kernel/src/main/java/org/apache/openjpa/kernel/DelegatingFetchConfiguration.java (revision 574344)
          +++ openjpa-kernel/src/main/java/org/apache/openjpa/kernel/DelegatingFetchConfiguration.java (working copy)
          @@ -287,6 +287,9 @@
          }
          }

          + public void removeLoadFetchGroup()

          { + + }

          public Set getFields() {
          try {
          return _fetch.getFields();
          Index: openjpa-kernel/src/main/java/org/apache/openjpa/kernel/FetchConfiguration.java
          ===================================================================
          — openjpa-kernel/src/main/java/org/apache/openjpa/kernel/FetchConfiguration.java (revision 574344)
          +++ openjpa-kernel/src/main/java/org/apache/openjpa/kernel/FetchConfiguration.java (working copy)
          @@ -192,7 +192,10 @@
          public FetchConfiguration clearFetchGroups();

          /**

          • * Resets the set of fetch groups to the list in the global configuration.
            + * Resets the set of fetch groups to the list in the global configuration.
            + * The global configuration will be the default plus any fetch groups
            + * that are added by the openjpa.FetchGroups property in the persistence.xml
            + * file.
            */
            public FetchConfiguration resetFetchGroups();

          @@ -197,6 +200,11 @@
          public FetchConfiguration resetFetchGroups();

          /**
          + * Remove the loadFetchGroup if there is any loadFetchGroup.
          + */
          + public void removeLoadFetchGroup();
          +
          + /**

          • Returns the set of fully-qualified field names that this component
          • will use when loading objects. Defaults to the empty set. This set is
          • not thread safe.
            Index: openjpa-kernel/src/main/java/org/apache/openjpa/kernel/FetchConfigurationImpl.java
            ===================================================================
              • openjpa-kernel/src/main/java/org/apache/openjpa/kernel/FetchConfigurationImpl.java (revision 575491)
                +++ openjpa-kernel/src/main/java/org/apache/openjpa/kernel/FetchConfigurationImpl.java (working copy)
                @@ -92,6 +92,7 @@
                private boolean _load = true;
                private int _availableRecursion;
                private int _availableDepth;
                + private String _lfg = null;

          public FetchConfigurationImpl()

          { this(null); @@ -287,7 +288,14 @@ getFetchGroupsList())); return this; }

          +
          + public void removeLoadFetchGroup()

          { + if (_lfg != null) + removeFetchGroup(_lfg); + _lfg = null; + }

          +
          public Set getFields()

          { return (_state.fields == null) ? Collections.EMPTY_SET : _state.fields; }

          @@ -568,8 +576,16 @@
          return true;
          String[] fgs = fmd.getCustomFetchGroups();
          for (int i = 0; i < fgs.length; i++)

          • if (hasFetchGroup(fgs[i]))
            + if (hasFetchGroup(fgs[i])) {
            + String fg = fmd.getLoadFetchGroup();
            + if (fg != null)
            Unknown macro: {+ if (!hasFetchGroup(fg)) { + addFetchGroup(fg); + _lfg = fg; + }+ }

            return true;
            + }
            return false;
            }

          @@ -576,7 +592,7 @@
          /**

          • Return the available recursion depth via the given field for the
          • given type.
          • *
            + *
          • @param traverse whether we're traversing the field
            */
            private int getAvailableRecursionDepth(FieldMetaData fm, Class type,
            Index: openjpa-kernel/src/main/java/org/apache/openjpa/kernel/StateManagerImpl.java
            ===================================================================
              • openjpa-kernel/src/main/java/org/apache/openjpa/kernel/StateManagerImpl.java (revision 575494)
                +++ openjpa-kernel/src/main/java/org/apache/openjpa/kernel/StateManagerImpl.java (working copy)
                @@ -358,6 +358,10 @@
                // is loaded, etc
                int lockLevel = calculateLockLevel(active, forWrite, fetch);
                boolean ret = loadFields(fields, fetch, lockLevel, sdata, forWrite);
                + // call back to FetchConfiguration to clean up any loadFetchGroup.
                + // The loadFetchGroup was set by the FetchConfigurationImpl.includes()
                + // during the process of the getUnloadedInternal method.
                + fetch.removeLoadFetchGroup();
                obtainLocks(active, forWrite, lockLevel, fetch, sdata);
                return ret;
                }
                Index: openjpa-kernel/src/main/java/org/apache/openjpa/meta/FieldMetaData.java
                ===================================================================
              • openjpa-kernel/src/main/java/org/apache/openjpa/meta/FieldMetaData.java (revision 574693)
                +++ openjpa-kernel/src/main/java/org/apache/openjpa/meta/FieldMetaData.java (working copy)
                @@ -710,6 +710,8 @@
                /**
          • The fetch group that is to be loaded when this receiver is loaded, or
          • null if none set.
            + * The LoadFetchGroup does not relate to the FetchGroup annotation. Therefore
            + * resets a fetch group will not remove this LoadFetchGroup.
            */
            public void setLoadFetchGroup (String lfg) {
            if ("".equals(lfg))
          Show
          Teresa Kan added a comment - Attach the patch. See the following changes: Index: openjpa-kernel/src/main/java/org/apache/openjpa/kernel/DelegatingFetchConfiguration.java =================================================================== — openjpa-kernel/src/main/java/org/apache/openjpa/kernel/DelegatingFetchConfiguration.java (revision 574344) +++ openjpa-kernel/src/main/java/org/apache/openjpa/kernel/DelegatingFetchConfiguration.java (working copy) @@ -287,6 +287,9 @@ } } + public void removeLoadFetchGroup() { + + } public Set getFields() { try { return _fetch.getFields(); Index: openjpa-kernel/src/main/java/org/apache/openjpa/kernel/FetchConfiguration.java =================================================================== — openjpa-kernel/src/main/java/org/apache/openjpa/kernel/FetchConfiguration.java (revision 574344) +++ openjpa-kernel/src/main/java/org/apache/openjpa/kernel/FetchConfiguration.java (working copy) @@ -192,7 +192,10 @@ public FetchConfiguration clearFetchGroups(); /** * Resets the set of fetch groups to the list in the global configuration. + * Resets the set of fetch groups to the list in the global configuration. + * The global configuration will be the default plus any fetch groups + * that are added by the openjpa.FetchGroups property in the persistence.xml + * file. */ public FetchConfiguration resetFetchGroups(); @@ -197,6 +200,11 @@ public FetchConfiguration resetFetchGroups(); /** + * Remove the loadFetchGroup if there is any loadFetchGroup. + */ + public void removeLoadFetchGroup(); + + /** Returns the set of fully-qualified field names that this component will use when loading objects. Defaults to the empty set. This set is not thread safe. Index: openjpa-kernel/src/main/java/org/apache/openjpa/kernel/FetchConfigurationImpl.java =================================================================== openjpa-kernel/src/main/java/org/apache/openjpa/kernel/FetchConfigurationImpl.java (revision 575491) +++ openjpa-kernel/src/main/java/org/apache/openjpa/kernel/FetchConfigurationImpl.java (working copy) @@ -92,6 +92,7 @@ private boolean _load = true; private int _availableRecursion; private int _availableDepth; + private String _lfg = null; public FetchConfigurationImpl() { this(null); @@ -287,7 +288,14 @@ getFetchGroupsList())); return this; } + + public void removeLoadFetchGroup() { + if (_lfg != null) + removeFetchGroup(_lfg); + _lfg = null; + } + public Set getFields() { return (_state.fields == null) ? Collections.EMPTY_SET : _state.fields; } @@ -568,8 +576,16 @@ return true; String[] fgs = fmd.getCustomFetchGroups(); for (int i = 0; i < fgs.length; i++) if (hasFetchGroup(fgs [i] )) + if (hasFetchGroup(fgs [i] )) { + String fg = fmd.getLoadFetchGroup(); + if (fg != null) Unknown macro: {+ if (!hasFetchGroup(fg)) { + addFetchGroup(fg); + _lfg = fg; + }+ } return true; + } return false; } @@ -576,7 +592,7 @@ /** Return the available recursion depth via the given field for the given type. * + * @param traverse whether we're traversing the field */ private int getAvailableRecursionDepth(FieldMetaData fm, Class type, Index: openjpa-kernel/src/main/java/org/apache/openjpa/kernel/StateManagerImpl.java =================================================================== openjpa-kernel/src/main/java/org/apache/openjpa/kernel/StateManagerImpl.java (revision 575494) +++ openjpa-kernel/src/main/java/org/apache/openjpa/kernel/StateManagerImpl.java (working copy) @@ -358,6 +358,10 @@ // is loaded, etc int lockLevel = calculateLockLevel(active, forWrite, fetch); boolean ret = loadFields(fields, fetch, lockLevel, sdata, forWrite); + // call back to FetchConfiguration to clean up any loadFetchGroup. + // The loadFetchGroup was set by the FetchConfigurationImpl.includes() + // during the process of the getUnloadedInternal method. + fetch.removeLoadFetchGroup(); obtainLocks(active, forWrite, lockLevel, fetch, sdata); return ret; } Index: openjpa-kernel/src/main/java/org/apache/openjpa/meta/FieldMetaData.java =================================================================== openjpa-kernel/src/main/java/org/apache/openjpa/meta/FieldMetaData.java (revision 574693) +++ openjpa-kernel/src/main/java/org/apache/openjpa/meta/FieldMetaData.java (working copy) @@ -710,6 +710,8 @@ /** The fetch group that is to be loaded when this receiver is loaded, or null if none set. + * The LoadFetchGroup does not relate to the FetchGroup annotation. Therefore + * resets a fetch group will not remove this LoadFetchGroup. */ public void setLoadFetchGroup (String lfg) { if ("".equals(lfg))
          Hide
          Teresa Kan added a comment -

          I redo the fix for the LoadFetchGroup. The basic fix is based on the following principles:

          1) when a field X which is part of the loadFetchGroup L but it is not an eager fetch field, field X can't determine whether it can be fetched or not because it is not sure that the owner of this loadFetchGroup will be loaded . Therefore, we have to wait until the owner of the loadFetchGroup is fetched.
          2) when a field Y has a FetchGroup associated with , then field Y is required to fetch.
          ==> Then we need further to check whether this field Y has any loadFetchGroup. If field Y has a loadFetchGroup L; then we need to get the fields that associate with L.

          I have been looking into how to capture the info at parser phase. For example, if this is a LoadFetchGroup, then it should know all the fields that associated with this group. However, I found out that the fetch group is a String of name in the FieldMetaData. Unless we change the design to use the real FetchGroup object instead of the name, then we can capture the data at parsing time.

          Anyway, I used the existing design and implementation to change the code. I have two patches:
          1) Patch 1 :
          It adds the LoadFetchGroup to the current active fetchgroup once it is found that the owner has defined this LoadFetchGroup, then StateManagerImpl will remove it later once all the fields are loaded.
          2) Patch 2:
          StateManagerImpl let the FetchConfiguration figure out what fields are needed to fetch for any fetch group. Instead of going to the FetchConfiguration.requiresFetch one field at a time, I introduced a new method FetchConfiguration.requiresFetch(set fetchgroup, FieldMetaData[] fmds) to return all the fields for the fetch groups.

          FetchConfiguration.requiresFetch() gets all the fields from the new includes(fetchgroup, fmds) method. This method loops thru each field and checks whether it associates with an active fetchgroup. If it is, then checks for a loadFetchGroup. If this field has the LoadFetchGroup, then get all the fields that associate with this LoadFetchGroup from getLoadFetchGroupFields().

          The getLoadFetchGroupFields() basically walk thru each field to check whether it is part of this LoadFetchGroup. Once all the fields are found, saved it in a cached Map for future use.

          Now we need to make decision about which patch should we use:
          Patch 1 is simple and less expensive ; but it does not fit well into the architecture. Patch 2 is kind of fit well with the architecute but it is more expensive. Although I cached the loadFetchGroupFields info, we still need to go thru once to build this cache.
          The best solution is to make use of the FetchGroup object. During the parse phase, capture the fields info in the FetchGroup. In addition to capture the fetch group info in the Field, each FetchGroup also captures the fields info. In this case, once we find a fetch group, we can get all the loadFetchGroup and its assocated fields info.

          Since I'm not totally familiar with the code, so it will take me a while to redo the FetchGroup objects in the parse phase. My suggestion is to commit either the patch1 or patch 2 in the version 1.0.x first. Then continue to work on the real implementation in 1.1.0.

          Show
          Teresa Kan added a comment - I redo the fix for the LoadFetchGroup. The basic fix is based on the following principles: 1) when a field X which is part of the loadFetchGroup L but it is not an eager fetch field, field X can't determine whether it can be fetched or not because it is not sure that the owner of this loadFetchGroup will be loaded . Therefore, we have to wait until the owner of the loadFetchGroup is fetched. 2) when a field Y has a FetchGroup associated with , then field Y is required to fetch. ==> Then we need further to check whether this field Y has any loadFetchGroup. If field Y has a loadFetchGroup L; then we need to get the fields that associate with L. I have been looking into how to capture the info at parser phase. For example, if this is a LoadFetchGroup, then it should know all the fields that associated with this group. However, I found out that the fetch group is a String of name in the FieldMetaData. Unless we change the design to use the real FetchGroup object instead of the name, then we can capture the data at parsing time. Anyway, I used the existing design and implementation to change the code. I have two patches: 1) Patch 1 : It adds the LoadFetchGroup to the current active fetchgroup once it is found that the owner has defined this LoadFetchGroup, then StateManagerImpl will remove it later once all the fields are loaded. 2) Patch 2: StateManagerImpl let the FetchConfiguration figure out what fields are needed to fetch for any fetch group. Instead of going to the FetchConfiguration.requiresFetch one field at a time, I introduced a new method FetchConfiguration.requiresFetch(set fetchgroup, FieldMetaData[] fmds) to return all the fields for the fetch groups. FetchConfiguration.requiresFetch() gets all the fields from the new includes(fetchgroup, fmds) method. This method loops thru each field and checks whether it associates with an active fetchgroup. If it is, then checks for a loadFetchGroup. If this field has the LoadFetchGroup, then get all the fields that associate with this LoadFetchGroup from getLoadFetchGroupFields(). The getLoadFetchGroupFields() basically walk thru each field to check whether it is part of this LoadFetchGroup. Once all the fields are found, saved it in a cached Map for future use. Now we need to make decision about which patch should we use: Patch 1 is simple and less expensive ; but it does not fit well into the architecture. Patch 2 is kind of fit well with the architecute but it is more expensive. Although I cached the loadFetchGroupFields info, we still need to go thru once to build this cache. The best solution is to make use of the FetchGroup object. During the parse phase, capture the fields info in the FetchGroup. In addition to capture the fetch group info in the Field, each FetchGroup also captures the fields info. In this case, once we find a fetch group, we can get all the loadFetchGroup and its assocated fields info. Since I'm not totally familiar with the code, so it will take me a while to redo the FetchGroup objects in the parse phase. My suggestion is to commit either the patch1 or patch 2 in the version 1.0.x first. Then continue to work on the real implementation in 1.1.0.
          Hide
          Kevin Sutter added a comment -

          Teresa,
          I'm looking at your patch and the comments in this Issue. The Patch 2 approach looks good to me. I suggest we go ahead with this patch for both 1.0.x and 1.1.0. If there are tweaks (desired or required) to the implementation, then we'll deal with them. At least with this patch, we now have a working LoadFetchGroup annotation.

          Question concerning the tests. They all seem to run just fine, but is the current package the proper location (org.apache.openjpa.persistence.query)? I'm thinking a separate fetchgroup package would be better. And, we 're bound to add more tests along the way. If that's okay with you, I'll do the move, you don't have to supply another patch. I will also add the required license header on the testcases.

          Thanks,
          Kevin

          Show
          Kevin Sutter added a comment - Teresa, I'm looking at your patch and the comments in this Issue. The Patch 2 approach looks good to me. I suggest we go ahead with this patch for both 1.0.x and 1.1.0. If there are tweaks (desired or required) to the implementation, then we'll deal with them. At least with this patch, we now have a working LoadFetchGroup annotation. Question concerning the tests. They all seem to run just fine, but is the current package the proper location (org.apache.openjpa.persistence.query)? I'm thinking a separate fetchgroup package would be better. And, we 're bound to add more tests along the way. If that's okay with you, I'll do the move, you don't have to supply another patch. I will also add the required license header on the testcases. Thanks, Kevin
          Hide
          Kevin Sutter added a comment -

          Resolved in 1.0.x Branch and 1.1.0 Trunk.

          Show
          Kevin Sutter added a comment - Resolved in 1.0.x Branch and 1.1.0 Trunk.
          Hide
          Patrick Linskey added a comment -

          It looks like this patch might have a performance impact due to the additional time required to compute field inclusion. We should consider other implementation routes or potentially rolling this back on the 1.0.x branch prior to 1.0.1, depending on what we decide our release criteria should be.

          Show
          Patrick Linskey added a comment - It looks like this patch might have a performance impact due to the additional time required to compute field inclusion. We should consider other implementation routes or potentially rolling this back on the 1.0.x branch prior to 1.0.1, depending on what we decide our release criteria should be.
          Hide
          Craig L Russell added a comment -

          I'm also concerned about the performance impact of the fix.

          The load fetch-group should not be a compute-intensive operation. It's meant to be a one-level operation, not a multi-level or recursive operation.

          The reason to use load fetch-group is to identify a fetch group to use when a field declared as a lazy loaded field (not declared in a currently active fetch-group at the time the instance it's a member of is fetched) is referenced. So it only applies when there is a requirement to load an unloaded field.

          > 1) when a field X which is part of the loadFetchGroup L but it is not an eager fetch field, field X can't determine whether it can be fetched or not because it is not sure that the owner of this loadFetchGroup will be loaded . Therefore, we have to wait until the owner of the loadFetchGroup is fetched.

          There is a difference between fetching a field and loading a field. Fetching a field is getting it from the datastore. Loading a field is fetching a field that is not currently loaded. For most operations (find, query, navigate) only the current fetch plan is needed to decide whether a field is fetched or not (a simple union of all fetch-groups in the fetch plan, disregarding the load fetch-group. The only time the load fetch-group is used is for field access operations where the instance is in memory but the field wasn't fetched before. This is due to one of two reasons: the instance is hollow and a field is accessed; the instance is not hollow and a field is accessed that wasnt' part of the fetch plan when the instance was fetched.

          > 2) when a field Y has a FetchGroup associated with , then field Y is required to fetch.
          > ==> Then we need further to check whether this field Y has any loadFetchGroup. If field Y has a loadFetchGroup L; then we need to get the fields that associate with L.

          I'm not really following this argument. The relevant part of the specification (JDO) is:

          <spec>
          When the application dereferences an unloaded field, the JDO implementation uses the current fetch plan and the load-fetch-group of the field to create the fetch strategy for the field. The specific behavior depends on whether the unloaded field is a relation to another persistence-capable class.
          for non-relation fields, the current fetch plan is applied to the field's owning instance, and the fields in the field's load-fetch-group, plus the field itself are added to the list of fields.
          for relation fields, the fields in the owning instance are fetched as immediately above, and additionally the instances referred by the field are loaded using the current fetch plan plus the field's load-fetch-group.
          </spec>

          From the discussion, it sounds like patch 1 is more appropriate to implement the intent.

          Show
          Craig L Russell added a comment - I'm also concerned about the performance impact of the fix. The load fetch-group should not be a compute-intensive operation. It's meant to be a one-level operation, not a multi-level or recursive operation. The reason to use load fetch-group is to identify a fetch group to use when a field declared as a lazy loaded field (not declared in a currently active fetch-group at the time the instance it's a member of is fetched) is referenced. So it only applies when there is a requirement to load an unloaded field. > 1) when a field X which is part of the loadFetchGroup L but it is not an eager fetch field, field X can't determine whether it can be fetched or not because it is not sure that the owner of this loadFetchGroup will be loaded . Therefore, we have to wait until the owner of the loadFetchGroup is fetched. There is a difference between fetching a field and loading a field. Fetching a field is getting it from the datastore. Loading a field is fetching a field that is not currently loaded. For most operations (find, query, navigate) only the current fetch plan is needed to decide whether a field is fetched or not (a simple union of all fetch-groups in the fetch plan, disregarding the load fetch-group. The only time the load fetch-group is used is for field access operations where the instance is in memory but the field wasn't fetched before. This is due to one of two reasons: the instance is hollow and a field is accessed; the instance is not hollow and a field is accessed that wasnt' part of the fetch plan when the instance was fetched. > 2) when a field Y has a FetchGroup associated with , then field Y is required to fetch. > ==> Then we need further to check whether this field Y has any loadFetchGroup. If field Y has a loadFetchGroup L; then we need to get the fields that associate with L. I'm not really following this argument. The relevant part of the specification (JDO) is: <spec> When the application dereferences an unloaded field, the JDO implementation uses the current fetch plan and the load-fetch-group of the field to create the fetch strategy for the field. The specific behavior depends on whether the unloaded field is a relation to another persistence-capable class. for non-relation fields, the current fetch plan is applied to the field's owning instance, and the fields in the field's load-fetch-group, plus the field itself are added to the list of fields. for relation fields, the fields in the owning instance are fetched as immediately above, and additionally the instances referred by the field are loaded using the current fetch plan plus the field's load-fetch-group. </spec> From the discussion, it sounds like patch 1 is more appropriate to implement the intent.
          Hide
          Kevin Sutter added a comment -

          Hi,
          Teresa is out for a week or so, so I will attempt to represent her interests on this issue...

          First thing is that there were several problems with the FetchGroup and FetchAttribute processing when Teresa started the investigation. She worked with Pinaki on several of the problems: http://www.nabble.com/FetchGroup-and-FetchAttribute-question-tf4357034.html#a12416564

          One of the problems that was not resolved directly by Pinaki was related to the LoadFetchGroup processing. In a nutshell, LoadFetchGroup just didn't work per the documentation (or maybe our interpretation of the documentation).

          In any case, she opened this Issue to resolve the problem with LoadFetchGroup. Although it may not be the best performing, at least it is now functional. So, from the standpoint of whether this fix should be part of 1.0.x or not, I would say that it's better to have a functional LoadFetchGroup or not at all.

          When I look back on this Issue, it discusses two patches, but as far as I can tell, there was only one patch ever attached to this issue. There was the separate patch file for the testcase, but there was only a single runtime code patch. From my discussions with Teresa, I believe the first patch idea was beyond her current understanding of the OpenJPA code base. She could develop the patch #2 idea based on the current architecture – although the performance may not be quite as good.

          Bottom line, I agree that we could probably improve the implementation of LoadFetchGroup to be better performing. But, my take is that we should focus our efforts on this improvement in the 1.1.0 trunk instead of pulling the functional implementation from the 1.0.x branch. Unless someone is volunteering to redo the fix for 1.0.x to be both functional and better performing, and a timely manner...

          Kevin

          Show
          Kevin Sutter added a comment - Hi, Teresa is out for a week or so, so I will attempt to represent her interests on this issue... First thing is that there were several problems with the FetchGroup and FetchAttribute processing when Teresa started the investigation. She worked with Pinaki on several of the problems: http://www.nabble.com/FetchGroup-and-FetchAttribute-question-tf4357034.html#a12416564 One of the problems that was not resolved directly by Pinaki was related to the LoadFetchGroup processing. In a nutshell, LoadFetchGroup just didn't work per the documentation (or maybe our interpretation of the documentation). In any case, she opened this Issue to resolve the problem with LoadFetchGroup. Although it may not be the best performing, at least it is now functional. So, from the standpoint of whether this fix should be part of 1.0.x or not, I would say that it's better to have a functional LoadFetchGroup or not at all. When I look back on this Issue, it discusses two patches, but as far as I can tell, there was only one patch ever attached to this issue. There was the separate patch file for the testcase, but there was only a single runtime code patch. From my discussions with Teresa, I believe the first patch idea was beyond her current understanding of the OpenJPA code base. She could develop the patch #2 idea based on the current architecture – although the performance may not be quite as good. Bottom line, I agree that we could probably improve the implementation of LoadFetchGroup to be better performing. But, my take is that we should focus our efforts on this improvement in the 1.1.0 trunk instead of pulling the functional implementation from the 1.0.x branch. Unless someone is volunteering to redo the fix for 1.0.x to be both functional and better performing, and a timely manner... Kevin
          Hide
          Patrick Linskey added a comment -

          > But, my take is that we should focus our efforts on this improvement in the
          > 1.1.0 trunk instead of pulling the functional implementation from the 1.0.x branch.

          I'm not as concerned with the 1.0.x behavior as with the 1.1.0 behavior, since I'm not in the process of releasing a product based on the 1.0.x branch.

          I do believe that the performance degradation is significant and important.

          > Unless someone is volunteering to redo the fix for 1.0.x to be both functional
          > and better performing, and a timely manner...

          I'm a bit concerned about the sentiment behind this statement. I think that performance regressions are big deals, and based on what I've seen from the performance impact of this patch, I basically think that the patch does not work. Sure, it resolves the issue, but it causes regressions in other areas of the product. As I mentioned, I'm ok letting things slide for 1.0.1, given that I understand that the goal of 1.0.1 is to get a product out. But I do not like it, and personally would prefer to see the behavior either fixed in a performant manner or backed out, especially in what we are claiming is a maintenance branch. And I definitely think that resolving this has to be a top priority.

          Show
          Patrick Linskey added a comment - > But, my take is that we should focus our efforts on this improvement in the > 1.1.0 trunk instead of pulling the functional implementation from the 1.0.x branch. I'm not as concerned with the 1.0.x behavior as with the 1.1.0 behavior, since I'm not in the process of releasing a product based on the 1.0.x branch. I do believe that the performance degradation is significant and important. > Unless someone is volunteering to redo the fix for 1.0.x to be both functional > and better performing, and a timely manner... I'm a bit concerned about the sentiment behind this statement. I think that performance regressions are big deals, and based on what I've seen from the performance impact of this patch, I basically think that the patch does not work. Sure, it resolves the issue, but it causes regressions in other areas of the product. As I mentioned, I'm ok letting things slide for 1.0.1, given that I understand that the goal of 1.0.1 is to get a product out. But I do not like it, and personally would prefer to see the behavior either fixed in a performant manner or backed out, especially in what we are claiming is a maintenance branch. And I definitely think that resolving this has to be a top priority.
          Hide
          Kevin Sutter added a comment -

          > I'm a bit concerned about the sentiment behind this statement. I think that performance regressions are big deals, and based on what I've seen from the performance impact of this patch, I basically think that the patch does not work. Sure, it resolves the issue, but it causes regressions in other areas of the product. As I mentioned, I'm ok letting things slide for 1.0.1, given that I understand that the goal of 1.0.1 is to get a product out. But I do not like it, and personally would prefer to see the behavior either fixed in a performant manner or backed out, especially in what we are claiming is a maintenance branch. And I definitely think that resolving this has to be a top priority.

          Sorry, but I was reacting to your statements about potentially pulling this fix from the 1.0.1 release. As you have pointed out, we're a little behind the eight ball since we're trying to close down the 1.0.1 release for an upcoming product release. So, pulling the existing fix out of the 1.0.x branch would affect some of our product test efforts and cause some unnecessary churn (IMHO).

          From a JPA perspective, the use of LoadFetchGroup is an extension. From our test efforts, it was shown not to work per our understanding of the documentation (and Pinaki's assistance). So, the problems discovered were resolved and put into the maintenance release. These were functional fixes. We knew there might be some performance concerns, but not being experts in this area, we were not aware of how there might be "regressions in other areas of the product". I'm assuming you are referring to the JDO extensions...

          So, where do we go from here? As you can tell, I would prefer to leave the fixes in the 1.0.x branch. We have re-opened the Issue. And, when Teresa comes back, she can take another look at a fix that performs better. If we can't wait, then somebody else will have to volunteer to take ownership on this Issue.

          Thanks,
          Kevin

          Show
          Kevin Sutter added a comment - > I'm a bit concerned about the sentiment behind this statement. I think that performance regressions are big deals, and based on what I've seen from the performance impact of this patch, I basically think that the patch does not work. Sure, it resolves the issue, but it causes regressions in other areas of the product. As I mentioned, I'm ok letting things slide for 1.0.1, given that I understand that the goal of 1.0.1 is to get a product out. But I do not like it, and personally would prefer to see the behavior either fixed in a performant manner or backed out, especially in what we are claiming is a maintenance branch. And I definitely think that resolving this has to be a top priority. Sorry, but I was reacting to your statements about potentially pulling this fix from the 1.0.1 release. As you have pointed out, we're a little behind the eight ball since we're trying to close down the 1.0.1 release for an upcoming product release. So, pulling the existing fix out of the 1.0.x branch would affect some of our product test efforts and cause some unnecessary churn (IMHO). From a JPA perspective, the use of LoadFetchGroup is an extension. From our test efforts, it was shown not to work per our understanding of the documentation (and Pinaki's assistance). So, the problems discovered were resolved and put into the maintenance release. These were functional fixes. We knew there might be some performance concerns, but not being experts in this area, we were not aware of how there might be "regressions in other areas of the product". I'm assuming you are referring to the JDO extensions... So, where do we go from here? As you can tell, I would prefer to leave the fixes in the 1.0.x branch. We have re-opened the Issue. And, when Teresa comes back, she can take another look at a fix that performs better. If we can't wait, then somebody else will have to volunteer to take ownership on this Issue. Thanks, Kevin
          Hide
          Patrick Linskey added a comment -

          Maybe I should be more clear about the impact. Our tests seem to indicate that this causes performance regressions across the board, not just when the feature is used. That is, the current implementation appears to slow down all relationship traversals and pk loads that do not come from cache.

          I haven't confirmed that 100%, but that's what the initial runs seem to show.

          Show
          Patrick Linskey added a comment - Maybe I should be more clear about the impact. Our tests seem to indicate that this causes performance regressions across the board, not just when the feature is used. That is, the current implementation appears to slow down all relationship traversals and pk loads that do not come from cache. I haven't confirmed that 100%, but that's what the initial runs seem to show.
          Hide
          Patrick Linskey added a comment -

          In our tests, we see a 20% performance regression due to this change. Our tests do not use the LoadFetchGroup feature. This is from a benchmark that queries an entity by a non-pk field and has eager relationship loading enabled for two other entities. The query-cache and the data-cache are large enough so that no DB calls occur during steady-state (and this has been verified by the profiles).

          Show
          Patrick Linskey added a comment - In our tests, we see a 20% performance regression due to this change. Our tests do not use the LoadFetchGroup feature. This is from a benchmark that queries an entity by a non-pk field and has eager relationship loading enabled for two other entities. The query-cache and the data-cache are large enough so that no DB calls occur during steady-state (and this has been verified by the profiles).
          Hide
          Kevin Sutter added a comment -

          Thanks, Patrick, for the additional detail. Now I understand your concern. And, I take that your profiling has shown that the performance degradation is due to the changes introduced by this Issue? Can you share any more details? Thanks.

          So, we're in kind of a quandary... Do we cut the 1.0.1 release with this change which makes LoadFetchGroups functional, or do we remove this change to get the performance back up?

          I'm surprised that we haven't detected a performance problem since we introduced this change. I will check on that as well.

          Thanks,
          Kevin

          Show
          Kevin Sutter added a comment - Thanks, Patrick, for the additional detail. Now I understand your concern. And, I take that your profiling has shown that the performance degradation is due to the changes introduced by this Issue? Can you share any more details? Thanks. So, we're in kind of a quandary... Do we cut the 1.0.1 release with this change which makes LoadFetchGroups functional, or do we remove this change to get the performance back up? I'm surprised that we haven't detected a performance problem since we introduced this change. I will check on that as well. Thanks, Kevin
          Hide
          Pinaki Poddar added a comment -

          > In a nutshell, LoadFetchGroup just didn't work per the documentation (or maybe our interpretation of the documentation).

          This statement – the root of this change – may require more validation. I have recently tried to verify LoadFetchGroup behavior with few tests and they are coming out positive. At this point, I will not contest the observation as my tests may not cover many variations. Yet, even if the above statement is true, we should first investigate why the existing (i.e. before Teresa's patch) mechanics is not working.

          The 'existing mechanics' is more in the spirit of how Craig has explained and elaborated LoadFetchGroup. Essentially, LoadFetchGroup is not taken into account when data is loaded from datastore. It is where the tight inner-loop evaluates for each field if it is to be 'fetched' from the datastore. Given high frequency of this evaluation – Patrick's concern on performance degradation is certainly valid.

          Before the instance is returned to the caller, the fields are checked again if they cause any other field be loaded because LoadFecthGroup. At that point, if field f has a LoadFetchGroup L and L is not part of the active fetch configuration then temporarily add L to the active FetchConfiguration and go for another 'fetch' (i.e. from the datastore).

          See StateManagerImpl.java for details:

          /**

          • Load the given field's fetch group; the field itself may already be
          • loaded if it is being set by the user.
            */
            protected void loadField(int field, int lockLevel, boolean forWrite, boolean fgs)
          Show
          Pinaki Poddar added a comment - > In a nutshell, LoadFetchGroup just didn't work per the documentation (or maybe our interpretation of the documentation). This statement – the root of this change – may require more validation. I have recently tried to verify LoadFetchGroup behavior with few tests and they are coming out positive. At this point, I will not contest the observation as my tests may not cover many variations. Yet, even if the above statement is true, we should first investigate why the existing (i.e. before Teresa's patch) mechanics is not working. The 'existing mechanics' is more in the spirit of how Craig has explained and elaborated LoadFetchGroup. Essentially, LoadFetchGroup is not taken into account when data is loaded from datastore. It is where the tight inner-loop evaluates for each field if it is to be 'fetched' from the datastore. Given high frequency of this evaluation – Patrick's concern on performance degradation is certainly valid. Before the instance is returned to the caller, the fields are checked again if they cause any other field be loaded because LoadFecthGroup. At that point, if field f has a LoadFetchGroup L and L is not part of the active fetch configuration then temporarily add L to the active FetchConfiguration and go for another 'fetch' (i.e. from the datastore). See StateManagerImpl.java for details: /** Load the given field's fetch group; the field itself may already be loaded if it is being set by the user. */ protected void loadField(int field, int lockLevel, boolean forWrite, boolean fgs)
          Hide
          Craig L Russell added a comment -

          > Before the instance is returned to the caller, the fields are checked again if they cause any other field be loaded because LoadFecthGroup. At that point, if field f has a LoadFetchGroup L and L is not part of the active fetch configuration then temporarily add L to the active FetchConfiguration and go for another 'fetch' (i.e. from the datastore).

          This sounds wrong. The effect of the load fetch group should be part of the fetch strategy, and no post-fetch analysis should be done. The only time the load fetch group is used is if a field f is accessed and it's not already fetched.

          The intent of the load fetch group is to augment the fetch plan under which the persistent instance was fetched. It's designed to provide an intelligent fetch strategy for the lower-usage cases where some use needs field f1 (not in any fetch group in the current fetch plan) and when using field f1 you want to also fetch fields f2, f3, and f4, that are also not part of the current fetch plan.

          Show
          Craig L Russell added a comment - > Before the instance is returned to the caller, the fields are checked again if they cause any other field be loaded because LoadFecthGroup. At that point, if field f has a LoadFetchGroup L and L is not part of the active fetch configuration then temporarily add L to the active FetchConfiguration and go for another 'fetch' (i.e. from the datastore). This sounds wrong. The effect of the load fetch group should be part of the fetch strategy, and no post-fetch analysis should be done. The only time the load fetch group is used is if a field f is accessed and it's not already fetched. The intent of the load fetch group is to augment the fetch plan under which the persistent instance was fetched. It's designed to provide an intelligent fetch strategy for the lower-usage cases where some use needs field f1 (not in any fetch group in the current fetch plan) and when using field f1 you want to also fetch fields f2, f3, and f4, that are also not part of the current fetch plan.
          Hide
          Kevin Sutter added a comment -

          Due to the performance concern comments, I have backed out these changes in my own sandbox and am running our performance suites trying to validate the problem. If the results indicate such a significant performance degradation, then I will gladly back out the changes until we figure out the proper solution.

          One thing that I immediately noticed is that the testcase that we committed with these changes no longer works.

          http://svn.apache.org/viewcvs.cgi//openjpa/branches/1.0.x/openjpa-persistence-jdbc/src/test/java/org/apache/openjpa/persistence/fetchgroups/TestFetchGroup.java/?rev=580995&view=markup

          So, maybe the first step is to validate the testcase. Could one of the interested parties that knows more about the intent of LoadFetchGroups (better than me) review this testcase?

          Thanks,
          Kevin

          Show
          Kevin Sutter added a comment - Due to the performance concern comments, I have backed out these changes in my own sandbox and am running our performance suites trying to validate the problem. If the results indicate such a significant performance degradation, then I will gladly back out the changes until we figure out the proper solution. One thing that I immediately noticed is that the testcase that we committed with these changes no longer works. http://svn.apache.org/viewcvs.cgi//openjpa/branches/1.0.x/openjpa-persistence-jdbc/src/test/java/org/apache/openjpa/persistence/fetchgroups/TestFetchGroup.java/?rev=580995&view=markup So, maybe the first step is to validate the testcase. Could one of the interested parties that knows more about the intent of LoadFetchGroups (better than me) review this testcase? Thanks, Kevin
          Hide
          Patrick Linskey added a comment -

          > I take that your profiling has shown that the performance degradation
          > is due to the changes introduced by this Issue? Can you share any more details?

          We've seen a ~ 30% regression since an OpenJPA snapshot from a few months back. Rolling back this particular change cuts that to a 10% regression.

          Show
          Patrick Linskey added a comment - > I take that your profiling has shown that the performance degradation > is due to the changes introduced by this Issue? Can you share any more details? We've seen a ~ 30% regression since an OpenJPA snapshot from a few months back. Rolling back this particular change cuts that to a 10% regression.
          Hide
          Craig L Russell added a comment -

          I'm looking at the test case TestFetchGroup.

          The test cases should check firstName and lastName in addition to the other fields.

          The difference between test001 and test002 isn't checked. In test001, lastName and firstName should be not null because they are in the default fetch group. In test002, they should be null because the reset fetch group has no fields.

          In test003, the only fields fetched should be the id and rating. In particular, address, firstName, and lastName should not be fetched. This test case seems to be central to the discussion here, and it appears to be checking the wrong result.

          The comments in the assertions don't match what's being checked. And the typos are very distracting.

          In test008 there are two EntityManagers used, and it doesn't appear that the usage of oem and oem1 are correct. What is the intent of having two em's?

          Show
          Craig L Russell added a comment - I'm looking at the test case TestFetchGroup. The test cases should check firstName and lastName in addition to the other fields. The difference between test001 and test002 isn't checked. In test001, lastName and firstName should be not null because they are in the default fetch group. In test002, they should be null because the reset fetch group has no fields. In test003, the only fields fetched should be the id and rating. In particular, address, firstName, and lastName should not be fetched. This test case seems to be central to the discussion here, and it appears to be checking the wrong result. The comments in the assertions don't match what's being checked. And the typos are very distracting. In test008 there are two EntityManagers used, and it doesn't appear that the usage of oem and oem1 are correct. What is the intent of having two em's?
          Hide
          Kevin Sutter added a comment -

          Okay, my performance runs are not quite as drastic as the ones cited by Patrick, but they still are not great. And, given the concerns about our interpretation of the LoadFetchGroups processing (via the testcase analysis), I will back out these changes from both the 1.0.x and 1.1.0 branches. I will do this this afternoon.

          Thanks for the insights and the patience while we figure this out. Teresa should be back in the saddle next week and we can figure out the proper solution and proper interpretation of LoadFetchGroups at that time (unless the collective "we" can do it before she gets back).

          Thanks,
          Kevin

          Show
          Kevin Sutter added a comment - Okay, my performance runs are not quite as drastic as the ones cited by Patrick, but they still are not great. And, given the concerns about our interpretation of the LoadFetchGroups processing (via the testcase analysis), I will back out these changes from both the 1.0.x and 1.1.0 branches. I will do this this afternoon. Thanks for the insights and the patience while we figure this out. Teresa should be back in the saddle next week and we can figure out the proper solution and proper interpretation of LoadFetchGroups at that time (unless the collective "we" can do it before she gets back). Thanks, Kevin
          Hide
          Pinaki Poddar added a comment -

          Test case for LoadFetchGroup.
          These test cases employ only basic fields (no relation). They all pass in my local environment.

          Craig, will you please review the cases to see that they are testing correct semantics of load-fetch-group?
          If you think that the test cases are alright then I will add relation fields in the mix.

          Show
          Pinaki Poddar added a comment - Test case for LoadFetchGroup. These test cases employ only basic fields (no relation). They all pass in my local environment. Craig, will you please review the cases to see that they are testing correct semantics of load-fetch-group? If you think that the test cases are alright then I will add relation fields in the mix.
          Hide
          Kevin Sutter added a comment -

          Moving the fix version for this Issue from 1.0.1 to 1.0.2.

          Show
          Kevin Sutter added a comment - Moving the fix version for this Issue from 1.0.1 to 1.0.2.
          Hide
          Kevin Sutter added a comment -

          The fix for OPENJPA-403 was due to the code originally dropped for OPENJPA-370. If portions of the original code patch are ever used again for OPENJPA-370, then you should be aware of the quick patch required for OPENJPA-403.

          Show
          Kevin Sutter added a comment - The fix for OPENJPA-403 was due to the code originally dropped for OPENJPA-370 . If portions of the original code patch are ever used again for OPENJPA-370 , then you should be aware of the quick patch required for OPENJPA-403 .
          Hide
          Craig L Russell added a comment -

          The test case doesn't have an example of where load fetch group is used. The only tests are for regular fetch groups. Some specific comments:

          1. The test case should use spaces not tabs for delimiters.

          2. The tests should use commit not rollback to detach the instances.

          3. The tests testFieldsInFetchGroupAndLoadFetchGroupAreLoaded and testFieldsInFetchGroupAndLoadFetchGroupAreLoadedUsingFind

          don't test load fetch groups. I'd suggest renaming them and changing their expected results as follows:

          /**

          • Verifies that when a field with a LoadFetchGroup L is fetched, the fields
          • included in L are not fetched either.
            *
            */
            public void testFieldsInFetchGroupAndNotLoadFetchGroupAreLoaded() { OpenJPAEntityManager em = emf.createEntityManager(); List<PObject> pcs = findByQuery(em, "SELECT p FROM PObject p", "f3"); for (PObject pc:pcs) assertLoaded(em, pc, "f3"); }

          public void testFieldsInFetchGroupAndNotLoadFetchGroupAreLoadedUsingFind()

          { OpenJPAEntityManager em = emf.createEntityManager(); PObject pc = findById(em, OID, "f3"); assertLoaded(em, pc, "f3"); }

          To test load fetch group you need to add internal methods that load field f3 after querying or finding it and then commit the transaction. Something like findByIdAndAccessField (OpenJPAEntityManager em, String field, Object oid, String... fetchGroups). After finding the object, it would access the named field and then commit. Similar functionality is needed for findByQueryAndAccessField.

          To do this, I would refactor isLoaded into isLoaded and loadField. The loadField can be used by the findByIdAndAccessField and findByQueryAndAccessField methods to access a field.

          Show
          Craig L Russell added a comment - The test case doesn't have an example of where load fetch group is used. The only tests are for regular fetch groups. Some specific comments: 1. The test case should use spaces not tabs for delimiters. 2. The tests should use commit not rollback to detach the instances. 3. The tests testFieldsInFetchGroupAndLoadFetchGroupAreLoaded and testFieldsInFetchGroupAndLoadFetchGroupAreLoadedUsingFind don't test load fetch groups. I'd suggest renaming them and changing their expected results as follows: /** Verifies that when a field with a LoadFetchGroup L is fetched, the fields included in L are not fetched either. * */ public void testFieldsInFetchGroupAndNotLoadFetchGroupAreLoaded() { OpenJPAEntityManager em = emf.createEntityManager(); List<PObject> pcs = findByQuery(em, "SELECT p FROM PObject p", "f3"); for (PObject pc:pcs) assertLoaded(em, pc, "f3"); } public void testFieldsInFetchGroupAndNotLoadFetchGroupAreLoadedUsingFind() { OpenJPAEntityManager em = emf.createEntityManager(); PObject pc = findById(em, OID, "f3"); assertLoaded(em, pc, "f3"); } To test load fetch group you need to add internal methods that load field f3 after querying or finding it and then commit the transaction. Something like findByIdAndAccessField (OpenJPAEntityManager em, String field, Object oid, String... fetchGroups). After finding the object, it would access the named field and then commit. Similar functionality is needed for findByQueryAndAccessField. To do this, I would refactor isLoaded into isLoaded and loadField. The loadField can be used by the findByIdAndAccessField and findByQueryAndAccessField methods to access a field.
          Hide
          Craig L Russell added a comment -

          Hi Pinaki,

          Just saw this message. I'll take a look later today.

          Thanks,

          Craig

          Craig Russell
          Architect, Sun Java Enterprise System http://java.sun.com/products/jdo
          408 276-5638 Craig.Russell@sun.com
          P.S. A good JDO? O, Gasp!

          Show
          Craig L Russell added a comment - Hi Pinaki, Just saw this message. I'll take a look later today. Thanks, Craig Craig Russell Architect, Sun Java Enterprise System http://java.sun.com/products/jdo 408 276-5638 Craig.Russell@sun.com P.S. A good JDO? O, Gasp!
          Hide
          Craig L Russell added a comment -

          Joe Grassel commented:
          > I'm wondering what the intent of the LoadFetchGroup function was when it was designed. The manual states:

          > "A field can also declare a load fetch group. When you access a lazy loaded field for the first time, OpenJPA makes a datastore trip to fetch that field's data. Sometimes, however, you know that whenever you access a lazy field A, you're likely to access lazy fields B and C as well. Therefore, it would be more efficient to fetch the data for A, B, and C in the same datastore trip. By setting A's load fetch group to the name of a fetch group containing B and C, you can tell OpenJPA to load all of these fields together when A is first accessed."

          > I guess I have a question about the function I'd like clarified:

          > What does it mean when B and C are co-fetched in the same datastore trip? Is the data just loaded into the entitymanager's datacache and held there until a hit is made on it (when the application finally reads the entity persistable property for the first time, this would save an additional hit to the database) or is it genuinely considered eagerly fetched (entity persistable property field is populated when the entity object is constructed by the find/query operation?)

          It's useful to highlight when the load fetch group behavior is activated: when you access a lazy loaded field for the first time. This is not done during query or find, but only when the lazy loaded field is accessed (from the application) and it's not already loaded.

          To your other point, if fields B and C are in field A's load fetch group, then accessing field A makes is available in the detached instance, and fields B and C are also available in the detached instance as if they were accessed at the same time as field A was accessed.

          > This makes a big difference in what an application programmer should expect. If the former, then LoadFetchGroup is just a datastore optimization that doesn't really make B and C eagerly loaded. It just saves a datastore trip should they ever be loaded. That means that if the entity becomes detached, B and C are not available because they were never accessed when the entity was managed by the persistence context.

          Yes, this does affect the fields that are detached. If fields B and C are loaded, then they are also loaded in the detached instance. But I'd be careful calling this behavior eager loading. Eager loading is done for fields based on the fetch plan in effect for a find or query that first loads the instance into memory. The load fetch group isn't considered here. The load fetch group is only activated when you access a field that wasn't eagerly loaded.

          > The latter, and the function behavior I expected, if data is acquired from the datastore hit, then I'd expect it to be available for reading from the entity object, even if the field was not access prior to becoming detached, since active fetch groups (or those referenced by a load fetch group) effectively nullify the LAZY loading setting on an affected persistable attribute. Knowing what behavior to expect is especially important, especially in the situation where entities are acquired with a transaction-scoped persistence context when then find/query occurs outside of a transaction. I'd expect A to be loaded because it was referenced in an active fetch group, and B and C to be loaded (and referenceable in the entity) due to the load fetch group setting.

          No, here's the difference between active fetch groups and load fetch groups. If you want fields B and C to be loaded when the instance is first accessed via find or query, then you need to include B and C in one of the active fetch groups when you execute find or query. If you want fields B and C to be loaded only when some lazy loaded field is accessed, then put B and C into a fetch group and define that fetch group as the load fetch group of the lazy loaded fields that you want to trigger the fetch of fields B and C.

          A slightly different slant on this is that if field A is in some fetch group FG1, and use of field A requires fields B and C, then any fetch group that includes A (e.g. FG1) should also include B and C. There's no need for a load fetch group here.

          > Also, I noticed that some of the examples closed the entitymanager in order to test loadfetchgroup behavior – what about when an entity is just detached from the persistence context, em.close() is one way to approach it, but that only works in JSE and JEE: Application Managed Persistence Contexts. That's not going to work in Container Managed Persistence Contexts, and detachment is probably going to be frequently seen by Transaction Scoped persistence contexts, and situations where entities are serialized across the wire to distinct application components (say, to an application client, a web service, or via RMIIIOP to a remote application server's ejb/web container.) I would expect that data to be available due to the fetchgroup/loadfetchgroup configuration.

          The test cases use em.clear() or em.close() to detach the instances, but any operation that detaches instances, including serialization, should exhibit the behavior.

          > This includes both non-relational and relational lazy-loaded fields.

          I don't understand this comment. Are you referring to relationship fields?

          Show
          Craig L Russell added a comment - Joe Grassel commented: > I'm wondering what the intent of the LoadFetchGroup function was when it was designed. The manual states: > "A field can also declare a load fetch group. When you access a lazy loaded field for the first time, OpenJPA makes a datastore trip to fetch that field's data. Sometimes, however, you know that whenever you access a lazy field A, you're likely to access lazy fields B and C as well. Therefore, it would be more efficient to fetch the data for A, B, and C in the same datastore trip. By setting A's load fetch group to the name of a fetch group containing B and C, you can tell OpenJPA to load all of these fields together when A is first accessed." > I guess I have a question about the function I'd like clarified: > What does it mean when B and C are co-fetched in the same datastore trip? Is the data just loaded into the entitymanager's datacache and held there until a hit is made on it (when the application finally reads the entity persistable property for the first time, this would save an additional hit to the database) or is it genuinely considered eagerly fetched (entity persistable property field is populated when the entity object is constructed by the find/query operation?) It's useful to highlight when the load fetch group behavior is activated: when you access a lazy loaded field for the first time. This is not done during query or find, but only when the lazy loaded field is accessed (from the application) and it's not already loaded. To your other point, if fields B and C are in field A's load fetch group, then accessing field A makes is available in the detached instance, and fields B and C are also available in the detached instance as if they were accessed at the same time as field A was accessed. > This makes a big difference in what an application programmer should expect. If the former, then LoadFetchGroup is just a datastore optimization that doesn't really make B and C eagerly loaded. It just saves a datastore trip should they ever be loaded. That means that if the entity becomes detached, B and C are not available because they were never accessed when the entity was managed by the persistence context. Yes, this does affect the fields that are detached. If fields B and C are loaded, then they are also loaded in the detached instance. But I'd be careful calling this behavior eager loading. Eager loading is done for fields based on the fetch plan in effect for a find or query that first loads the instance into memory. The load fetch group isn't considered here. The load fetch group is only activated when you access a field that wasn't eagerly loaded. > The latter, and the function behavior I expected, if data is acquired from the datastore hit, then I'd expect it to be available for reading from the entity object, even if the field was not access prior to becoming detached, since active fetch groups (or those referenced by a load fetch group) effectively nullify the LAZY loading setting on an affected persistable attribute. Knowing what behavior to expect is especially important, especially in the situation where entities are acquired with a transaction-scoped persistence context when then find/query occurs outside of a transaction. I'd expect A to be loaded because it was referenced in an active fetch group, and B and C to be loaded (and referenceable in the entity) due to the load fetch group setting. No, here's the difference between active fetch groups and load fetch groups. If you want fields B and C to be loaded when the instance is first accessed via find or query, then you need to include B and C in one of the active fetch groups when you execute find or query. If you want fields B and C to be loaded only when some lazy loaded field is accessed, then put B and C into a fetch group and define that fetch group as the load fetch group of the lazy loaded fields that you want to trigger the fetch of fields B and C. A slightly different slant on this is that if field A is in some fetch group FG1, and use of field A requires fields B and C, then any fetch group that includes A (e.g. FG1) should also include B and C. There's no need for a load fetch group here. > Also, I noticed that some of the examples closed the entitymanager in order to test loadfetchgroup behavior – what about when an entity is just detached from the persistence context, em.close() is one way to approach it, but that only works in JSE and JEE: Application Managed Persistence Contexts. That's not going to work in Container Managed Persistence Contexts, and detachment is probably going to be frequently seen by Transaction Scoped persistence contexts, and situations where entities are serialized across the wire to distinct application components (say, to an application client, a web service, or via RMIIIOP to a remote application server's ejb/web container.) I would expect that data to be available due to the fetchgroup/loadfetchgroup configuration. The test cases use em.clear() or em.close() to detach the instances, but any operation that detaches instances, including serialization, should exhibit the behavior. > This includes both non-relational and relational lazy-loaded fields. I don't understand this comment. Are you referring to relationship fields?
          Hide
          Craig L Russell added a comment -

          Joe Grassel commented:
          > So, for B and C to be loaded, the lazy-loaded field that has the load fetch group annotation has to be accessed while the entity is still managed. When A is faulted in, it faults B and C in automatically, even if they are never accessed while the entity is managed.

          True.

          > So given the entity FGEmployee (using this since the code is already available as an attachment, and keeps my message shorter), which (all, some, or none) of the following LoadFetchGroup scenarios are valid:

          > Scenario A:
          > Starting environment: No fetch groups other then "default" are active, persistence context is clear of any managed entities

          > 1) Start a transaction
          > 2) Find/Query FGEmployee
          > 3) Read rating (this causes addressGroup to be faulted in, due to the @LoadFetchGroup on rating)
          > 4) Commit the transaction
          > 5) Clear the persistence context (not necessary for TS-CM PCs)
          > 6) Read rating, since it was read while the entity was managed in step 3, its data should be available
          > 7) Read addressGroup, although it was not read while the entity was managed, it was faulted in when addressGroup was accessed in step 3, because of the @LoadFetchGroup, so it should be available. Other lazy loaded fields, like description, dept, manager, are not available at this point since they were never accessed while the entity was managed, and since default was the only active fetch group, they would have observed lazy loading behavior.

          True.

          > Scenario B:
          > Starting environment: Active Fetch Groups: "default", "RatingFetchGroup". Persistence context is clear of any managed entities

          > 1) Start a transaction
          > 2) Find/Query FGEmployee
          > 3) Commit the Transaction
          > 4) Clear the persistence context

          > (Note, the first four steps can be condensed to just 1 for a TS-CM PS if performed outside of a transaction)

          > 5) Read rating, since it was targeted by the RatingFetchGroup, it should be available even though it was never accessed while the entity was managed. This has been tested and proven that this is the observed behavior for persistable attributes identified by fetch groups.

          True.

          > 6) Read addressGroup. This is the rub. Is it available? ratings was loaded in because it was part of an active fetch plan. I would expect that since rating was loaded, and since rating has declared that addressGroup should also be loaded via its @LoadFetchGroup annotation, then addressGroup should be available as well.

          No. Rating was not accessed when not loaded, so its load fetch group is not used.

          The behavior that you said you would expect does not need any additional concept. It can be accomplished just by defining your fetch groups. The reason we added load fetch groups was to handle the case where the application took a small probability path and accessed a field that is normally not needed. Because the field was now accessed, we need to go to the datastore. And without load fetch group, we would not be able to describe the fields needed by the small probability path, and would end up faulting several fields individually. With load fetch group, we can optimally fault in all the related fields.

          > Other lazy loaded fields, like description, dept, manager, are not available at this point since they were never accessed while the entity was managed, and were never a member of any of the active fetch groups, they would have observed lazy loading behavior.

          True.

          > The documentation as it is written makes it sound like both scenarios are valid – any situation where rating's data is available, addressGroup's data is also supposed to be available, thanks to the @LoadFetchGroup. Which scenarios are valid interpretations of the function? Scenario A, Scenario B, both, or neither?

          A, not B.

          >>Yes, this does affect the fields that are detached. If fields B and C are loaded, then they are also loaded in the detached instance. But I'd be careful calling this behavior eager loading. Eager loading is done for fields based on the fetch plan in effect for a find or query that first loads the instance into memory. The load fetch group isn't considered here. The load fetch group is only activated when you access a field that wasn't eagerly loaded.

          >So, this means that Scenario A is the only scenario where @LoadFetchGroup will work? Since rating, in Scenario B, would not be considered a lazy loaded field because it is a member of an active fetch plan?

          True. Rating, in scenario B, is not lazy loaded. It's eagerly loaded.

          >> A slightly different slant on this is that if field A is in some fetch group FG1, and use of field A requires fields B and C, then any fetch group that includes A (e.g. FG1) should also include B and C. There's no need for a load fetch group here.

          > So, in the situation where we want both B and C to be loaded if A is loaded, we would either:

          > 1) Do Scenario A
          > 2) Have fetch groups that include A, B, and C active when the find/query is performed.

          True.

          > If that is the case, then I think the documentation really needs to be updated to make that crystal clear.

          I agree some examples would be useful here. Volunteers?

          To update the description, we could change "When you access a lazy loaded field for the first time" to "When you access a lazy loaded field by the application for the first time after being queried or found".

          > Otherwise, it will be easy to assume (as I did) that @LoadFetchGroups will always be triggered if the persistable attribute annotated with it is loaded, be it approached in Scenario A or B. By saying "you can tell OpenJPA to load all of these fields together when A is first accessed." that sounds like B and C will always be loaded along with A, no matter what the loading circumstance (A loaded by a getter method while the entity is managed, or A loaded in by fetch group).

          So you understood "When you access a lazy loaded field for the first time" that "access" is a find or query? We meant to intend "access" to mean "access by the application".

          >>>This includes both non-relational and relational lazy-loaded fields.

          >>I don't understand this comment. Are you referring to relationship fields?

          >Yes, making sure that both attributes that make sense to be lazy loaded (Strings, blobs, etc.) and relationships (any, although the collection based relationships are lazy by default) are included under consideration here.

          Yes, the same rules apply to relationship fields. If they are lazy loaded, not part of any active fetch group, and then accessed by the application; when they are fetched from the datastore, the current fetch plan is augmented with the load fetch group of the relationship field and the fields that are in the augmented fetch plan are loaded from the datastore. This might load additional fields in the instance containing the lazy loaded relationship field, as well as loading the related object(s) according to the augmented fetch plan.

          Show
          Craig L Russell added a comment - Joe Grassel commented: > So, for B and C to be loaded, the lazy-loaded field that has the load fetch group annotation has to be accessed while the entity is still managed. When A is faulted in, it faults B and C in automatically, even if they are never accessed while the entity is managed. True. > So given the entity FGEmployee (using this since the code is already available as an attachment, and keeps my message shorter), which (all, some, or none) of the following LoadFetchGroup scenarios are valid: > Scenario A: > Starting environment: No fetch groups other then "default" are active, persistence context is clear of any managed entities > 1) Start a transaction > 2) Find/Query FGEmployee > 3) Read rating (this causes addressGroup to be faulted in, due to the @LoadFetchGroup on rating) > 4) Commit the transaction > 5) Clear the persistence context (not necessary for TS-CM PCs) > 6) Read rating, since it was read while the entity was managed in step 3, its data should be available > 7) Read addressGroup, although it was not read while the entity was managed, it was faulted in when addressGroup was accessed in step 3, because of the @LoadFetchGroup, so it should be available. Other lazy loaded fields, like description, dept, manager, are not available at this point since they were never accessed while the entity was managed, and since default was the only active fetch group, they would have observed lazy loading behavior. True. > Scenario B: > Starting environment: Active Fetch Groups: "default", "RatingFetchGroup". Persistence context is clear of any managed entities > 1) Start a transaction > 2) Find/Query FGEmployee > 3) Commit the Transaction > 4) Clear the persistence context > (Note, the first four steps can be condensed to just 1 for a TS-CM PS if performed outside of a transaction) > 5) Read rating, since it was targeted by the RatingFetchGroup, it should be available even though it was never accessed while the entity was managed. This has been tested and proven that this is the observed behavior for persistable attributes identified by fetch groups. True. > 6) Read addressGroup. This is the rub. Is it available? ratings was loaded in because it was part of an active fetch plan. I would expect that since rating was loaded, and since rating has declared that addressGroup should also be loaded via its @LoadFetchGroup annotation, then addressGroup should be available as well. No. Rating was not accessed when not loaded, so its load fetch group is not used. The behavior that you said you would expect does not need any additional concept. It can be accomplished just by defining your fetch groups. The reason we added load fetch groups was to handle the case where the application took a small probability path and accessed a field that is normally not needed. Because the field was now accessed, we need to go to the datastore. And without load fetch group, we would not be able to describe the fields needed by the small probability path, and would end up faulting several fields individually. With load fetch group, we can optimally fault in all the related fields. > Other lazy loaded fields, like description, dept, manager, are not available at this point since they were never accessed while the entity was managed, and were never a member of any of the active fetch groups, they would have observed lazy loading behavior. True. > The documentation as it is written makes it sound like both scenarios are valid – any situation where rating's data is available, addressGroup's data is also supposed to be available, thanks to the @LoadFetchGroup. Which scenarios are valid interpretations of the function? Scenario A, Scenario B, both, or neither? A, not B. >>Yes, this does affect the fields that are detached. If fields B and C are loaded, then they are also loaded in the detached instance. But I'd be careful calling this behavior eager loading. Eager loading is done for fields based on the fetch plan in effect for a find or query that first loads the instance into memory. The load fetch group isn't considered here. The load fetch group is only activated when you access a field that wasn't eagerly loaded. >So, this means that Scenario A is the only scenario where @LoadFetchGroup will work? Since rating, in Scenario B, would not be considered a lazy loaded field because it is a member of an active fetch plan? True. Rating, in scenario B, is not lazy loaded. It's eagerly loaded. >> A slightly different slant on this is that if field A is in some fetch group FG1, and use of field A requires fields B and C, then any fetch group that includes A (e.g. FG1) should also include B and C. There's no need for a load fetch group here. > So, in the situation where we want both B and C to be loaded if A is loaded, we would either: > 1) Do Scenario A > 2) Have fetch groups that include A, B, and C active when the find/query is performed. True. > If that is the case, then I think the documentation really needs to be updated to make that crystal clear. I agree some examples would be useful here. Volunteers? To update the description, we could change "When you access a lazy loaded field for the first time" to "When you access a lazy loaded field by the application for the first time after being queried or found". > Otherwise, it will be easy to assume (as I did) that @LoadFetchGroups will always be triggered if the persistable attribute annotated with it is loaded, be it approached in Scenario A or B. By saying "you can tell OpenJPA to load all of these fields together when A is first accessed." that sounds like B and C will always be loaded along with A, no matter what the loading circumstance (A loaded by a getter method while the entity is managed, or A loaded in by fetch group). So you understood "When you access a lazy loaded field for the first time" that "access" is a find or query? We meant to intend "access" to mean "access by the application". >>>This includes both non-relational and relational lazy-loaded fields. >>I don't understand this comment. Are you referring to relationship fields? >Yes, making sure that both attributes that make sense to be lazy loaded (Strings, blobs, etc.) and relationships (any, although the collection based relationships are lazy by default) are included under consideration here. Yes, the same rules apply to relationship fields. If they are lazy loaded, not part of any active fetch group, and then accessed by the application; when they are fetched from the datastore, the current fetch plan is augmented with the load fetch group of the relationship field and the fields that are in the augmented fetch plan are loaded from the datastore. This might load additional fields in the instance containing the lazy loaded relationship field, as well as loading the related object(s) according to the augmented fetch plan.
          Hide
          Teresa Kan added a comment -

          I modified the TestFetchGroup testcase based on the current discussion and understanding of the fetch group and loadFetchGroup concept.. The testcases work with the current implementation now. However, I have one question about Craig's comment on the testcase 002,
          Craig said,
          "The difference between test001 and test002 isn't checked. In test001, lastName and firstName should be not null because they are in the default fetch group. In test002, they should be null because the reset fetch group has no fields."
          According to the documentation, resetFetchGroup will reset back to the global configuration which I interpreted it as the default fetch group plus the property defined in the persistence.xml file . That's how the current implementaiton work at this moment. Therefore, both firstName and lastName are still available (not null) even though resetFetchGroup has been called.
          Please confirm the interpretation! If this is not true, then we have a bug in the openjpa for the resetFetchGroup.

          Thanks
          Teresa

          Show
          Teresa Kan added a comment - I modified the TestFetchGroup testcase based on the current discussion and understanding of the fetch group and loadFetchGroup concept.. The testcases work with the current implementation now. However, I have one question about Craig's comment on the testcase 002, Craig said, "The difference between test001 and test002 isn't checked. In test001, lastName and firstName should be not null because they are in the default fetch group. In test002, they should be null because the reset fetch group has no fields." According to the documentation, resetFetchGroup will reset back to the global configuration which I interpreted it as the default fetch group plus the property defined in the persistence.xml file . That's how the current implementaiton work at this moment. Therefore, both firstName and lastName are still available (not null) even though resetFetchGroup has been called. Please confirm the interpretation! If this is not true, then we have a bug in the openjpa for the resetFetchGroup. Thanks Teresa
          Hide
          Patrick Linskey added a comment -

          I believe that resetFetchGroup should reset the current fetch groups, not the current state of the instances.

          Show
          Patrick Linskey added a comment - I believe that resetFetchGroup should reset the current fetch groups, not the current state of the instances.
          Hide
          Craig L Russell added a comment -

          > According to the documentation, resetFetchGroup will reset back to the global configuration which I interpreted it as the default fetch group plus the property defined in the persistence.xml file . That's how the current implementaiton work at this moment. Therefore, both firstName and lastName are still available (not null) even though resetFetchGroup has been called.
          > Please confirm the interpretation! If this is not true, then we have a bug in the openjpa for the resetFetchGroup.

          You're right here. There is nothing in the JDO spec corresponding to resetFetchGroups, so my comments will need to be updated. I erroneously thought that resetFetchGroups referred to clearFetchGroups. Which we should add as a test case to make sure that clear does in fact remove all fetch groups. So in the test after resetFetchGroups, firstName and lastName should be available.

          I also noticed we have a conflict between the javadoc and the documentation. The javadoc refers to FetchConfiguration while the manual refers to FetchPlan. FetchPlan is an interface in JDO, whereas FetchConfiguration is an OpenJPA interface. So we're documenting FetchPlan in the manual but implementing FetchConfiguration.

          I'll raise a separate JIRA for this.

          Show
          Craig L Russell added a comment - > According to the documentation, resetFetchGroup will reset back to the global configuration which I interpreted it as the default fetch group plus the property defined in the persistence.xml file . That's how the current implementaiton work at this moment. Therefore, both firstName and lastName are still available (not null) even though resetFetchGroup has been called. > Please confirm the interpretation! If this is not true, then we have a bug in the openjpa for the resetFetchGroup. You're right here. There is nothing in the JDO spec corresponding to resetFetchGroups, so my comments will need to be updated. I erroneously thought that resetFetchGroups referred to clearFetchGroups. Which we should add as a test case to make sure that clear does in fact remove all fetch groups. So in the test after resetFetchGroups, firstName and lastName should be available. I also noticed we have a conflict between the javadoc and the documentation. The javadoc refers to FetchConfiguration while the manual refers to FetchPlan. FetchPlan is an interface in JDO, whereas FetchConfiguration is an OpenJPA interface. So we're documenting FetchPlan in the manual but implementing FetchConfiguration. I'll raise a separate JIRA for this.
          Hide
          Teresa Kan added a comment -

          Craig,
          Yes, the clearFetchGroup() performs as expected. All fields are cleared even the default fetch group. I added the test009 to include the clearFetchGroup() to the TestFetchGroup.patch.

          Show
          Teresa Kan added a comment - Craig, Yes, the clearFetchGroup() performs as expected. All fields are cleared even the default fetch group. I added the test009 to include the clearFetchGroup() to the TestFetchGroup.patch.
          Hide
          Patrick Linskey added a comment -

          I believe that the upshot of this is that it works as designed. If there are remaining issues that I did not notice, please expand on them.

          Show
          Patrick Linskey added a comment - I believe that the upshot of this is that it works as designed. If there are remaining issues that I did not notice, please expand on them.

            People

            • Assignee:
              Teresa Kan
              Reporter:
              Teresa Kan
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development