OpenJPA
  1. OpenJPA
  2. OPENJPA-2414

FinderCache does not consider active Fetch Groups/FetchPlan added Fields

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.2.3, 2.3.0
    • Fix Version/s: 2.1.2, 2.2.1.1, 2.2.3, 2.3.0
    • Component/s: kernel
    • Labels:
      None

      Description

      The FinderCache retains a Map, associating a ClassMapping with a FinderQuery. However, this cache does not factor in the characteristics of the FetchPlan that was active when a mapping is created, nor does it factor them to determine if a cache hit is appropriate. This causes the find() operation to perform the same SQL as the first time it was executed, regardless of changes to the active FetchPlan afterwards.

        Activity

        Hide
        ASF subversion and git services added a comment -

        Commit 1516197 from Jody Grassel in branch 'openjpa/branches/2.1.x'
        [ https://svn.apache.org/r1516197 ]

        OPENJPA-2414: FinderCache does not consider active Fetch Groups/FetchPlan added Fields

        Show
        ASF subversion and git services added a comment - Commit 1516197 from Jody Grassel in branch 'openjpa/branches/2.1.x' [ https://svn.apache.org/r1516197 ] OPENJPA-2414 : FinderCache does not consider active Fetch Groups/FetchPlan added Fields
        Hide
        ASF subversion and git services added a comment -

        Commit 1516464 from Jody Grassel in branch 'openjpa/branches/2.1.x'
        [ https://svn.apache.org/r1516464 ]

        OPENJPA-2414: FinderCache does not consider active Fetch Groups/FetchPlan added Fields

        Show
        ASF subversion and git services added a comment - Commit 1516464 from Jody Grassel in branch 'openjpa/branches/2.1.x' [ https://svn.apache.org/r1516464 ] OPENJPA-2414 : FinderCache does not consider active Fetch Groups/FetchPlan added Fields
        Hide
        ASF subversion and git services added a comment -

        Commit 1516543 from Jody Grassel in branch 'openjpa/branches/2.2.x'
        [ https://svn.apache.org/r1516543 ]

        OPENJPA-2414: FinderCache does not consider active Fetch Groups/FetchPlan added Fields

        Show
        ASF subversion and git services added a comment - Commit 1516543 from Jody Grassel in branch 'openjpa/branches/2.2.x' [ https://svn.apache.org/r1516543 ] OPENJPA-2414 : FinderCache does not consider active Fetch Groups/FetchPlan added Fields
        Hide
        ASF subversion and git services added a comment -

        Commit 1516853 from Jody Grassel in branch 'openjpa/trunk'
        [ https://svn.apache.org/r1516853 ]

        OPENJPA-2414: FinderCache does not consider active Fetch Groups/FetchPlan added Fields

        Show
        ASF subversion and git services added a comment - Commit 1516853 from Jody Grassel in branch 'openjpa/trunk' [ https://svn.apache.org/r1516853 ] OPENJPA-2414 : FinderCache does not consider active Fetch Groups/FetchPlan added Fields
        Hide
        ASF subversion and git services added a comment -

        Commit 1516873 from Jody Grassel in branch 'openjpa/branches/2.2.x'
        [ https://svn.apache.org/r1516873 ]

        OPENJPA-2414: FinderCache does not consider active Fetch Groups/FetchPlan added Fields

        Show
        ASF subversion and git services added a comment - Commit 1516873 from Jody Grassel in branch 'openjpa/branches/2.2.x' [ https://svn.apache.org/r1516873 ] OPENJPA-2414 : FinderCache does not consider active Fetch Groups/FetchPlan added Fields
        Hide
        ASF subversion and git services added a comment -

        Commit 1516910 from Jody Grassel in branch 'openjpa/branches/2.1.x'
        [ https://svn.apache.org/r1516910 ]

        OPENJPA-2414: FinderCache does not consider active Fetch Groups/FetchPlan added Fields

        Show
        ASF subversion and git services added a comment - Commit 1516910 from Jody Grassel in branch 'openjpa/branches/2.1.x' [ https://svn.apache.org/r1516910 ] OPENJPA-2414 : FinderCache does not consider active Fetch Groups/FetchPlan added Fields
        Hide
        ASF subversion and git services added a comment -

        Commit 1516931 from Jody Grassel in branch 'openjpa/branches/2.2.1.x'
        [ https://svn.apache.org/r1516931 ]

        OPENJPA-2414: FinderCache does not consider active Fetch Groups/FetchPlan added Fields

        Show
        ASF subversion and git services added a comment - Commit 1516931 from Jody Grassel in branch 'openjpa/branches/2.2.1.x' [ https://svn.apache.org/r1516931 ] OPENJPA-2414 : FinderCache does not consider active Fetch Groups/FetchPlan added Fields
        Hide
        Pinaki Poddar added a comment -

        This change is not align with the original intent. The idea of caching a finder query is not related to whether only default Fetch Plan is being used. The idea is (or at least was) that if anything (fetch plan, lock mode) that impact a target SQL, the application must bypass the cache. This design decision is deliberate from a performance perspective. Because under such immutablity assumption, the runtime does not have to spend any extra computation cycle to use the cache. While if the application always knows when it changes something that impacts the target SQL, so it can always bypass the finder cache. This choice favors the common 80% use case over the less common use case where the application mutates something that impacts a target SQL.

        This issue is reopened. Please either start a discussion if you are suggesting an alternative design/usage or revert to "user-is-reponsible" model of usage which was the original intent.

        Show
        Pinaki Poddar added a comment - This change is not align with the original intent. The idea of caching a finder query is not related to whether only default Fetch Plan is being used. The idea is (or at least was) that if anything (fetch plan, lock mode) that impact a target SQL, the application must bypass the cache. This design decision is deliberate from a performance perspective. Because under such immutablity assumption, the runtime does not have to spend any extra computation cycle to use the cache. While if the application always knows when it changes something that impacts the target SQL, so it can always bypass the finder cache. This choice favors the common 80% use case over the less common use case where the application mutates something that impacts a target SQL. This issue is reopened. Please either start a discussion if you are suggesting an alternative design/usage or revert to "user-is-reponsible" model of usage which was the original intent.
        Hide
        Jody Grassel added a comment -

        Please show me the location in the OpenJPA documentation that supports your position as "original intent" is meaningless if it's not well documented. It's sure not intuitive and it's going to fool a lot of applications consuming OpenJPA (which has already happened, hence this going into a service branch.) If you're asserting that OpenJPA's behavior before the fix is correct and in "alignment with original intent", then you're going to have a race condition where the first em.find() sets the sql for every em.find() that comes after (look at the new tests added that look for this.) In a J2EE application with many entry points, whoever first gets to set the Fetch Plan forever associates with the characteristics of that fetch plan with every future em.find(MyEntity.class) – you will have a race condition. A call to em.find() with a default fetch plan with a FinderCache initialized with an entry with a non default fetch plan will execute the non-default fetch plan sql because that is what is cached. I think everyone can agree that is a BAD thing.

        The change this JIRA makes is quite simple: only SQL generated by the default fetch plan for the persistence context (be it just "default" or a new default as set by pu properties) is what's cached. If an em.find() is performed with a fetch plan that is not equal to the default, then it will not go to the cache and instead build the SQL as dictated by the current fetch plan. This way an altered fetch plan is honored (instead of ignored) each time it is changed, and is safe for consumption in a service stream. 80% of most OpenJPA users probably do not touch FetchPlans, so this will not impact them.

        I have considered making the FinderCache FetchPlan smart for trunk, that will be done in a future JIRA.

        I will discuss this with Kevin tomorrow morning, and if he still agrees with my change I will re-close this JIRA.

        Show
        Jody Grassel added a comment - Please show me the location in the OpenJPA documentation that supports your position as "original intent" is meaningless if it's not well documented. It's sure not intuitive and it's going to fool a lot of applications consuming OpenJPA (which has already happened, hence this going into a service branch.) If you're asserting that OpenJPA's behavior before the fix is correct and in "alignment with original intent", then you're going to have a race condition where the first em.find() sets the sql for every em.find() that comes after (look at the new tests added that look for this.) In a J2EE application with many entry points, whoever first gets to set the Fetch Plan forever associates with the characteristics of that fetch plan with every future em.find(MyEntity.class) – you will have a race condition. A call to em.find() with a default fetch plan with a FinderCache initialized with an entry with a non default fetch plan will execute the non-default fetch plan sql because that is what is cached. I think everyone can agree that is a BAD thing. The change this JIRA makes is quite simple: only SQL generated by the default fetch plan for the persistence context (be it just "default" or a new default as set by pu properties) is what's cached. If an em.find() is performed with a fetch plan that is not equal to the default, then it will not go to the cache and instead build the SQL as dictated by the current fetch plan. This way an altered fetch plan is honored (instead of ignored) each time it is changed, and is safe for consumption in a service stream. 80% of most OpenJPA users probably do not touch FetchPlans, so this will not impact them. I have considered making the FinderCache FetchPlan smart for trunk, that will be done in a future JIRA. I will discuss this with Kevin tomorrow morning, and if he still agrees with my change I will re-close this JIRA.
        Hide
        Kevin Sutter added a comment -

        Oh sure, Thanks Jody for hauling me into this discussion...

        I will agree that the current usage scenario was not intuitive or documented. And, since we have customers hitting this condition of accidentally caching the "wrong" SQL, I agree that we need to do something in the service streams to resolve the issue. And, considering the 80/20 rule, it seems that erring on the default/conservative side definitely hits the 80% side of the market.

        That all being said, Pinaki does have a valid point that we are now alienating users that use fetch plans from using the FinderCache. But, that is also consistent with Pinaki's direction to have the application control whether these generated SQL's should be cached or not. We're just making that decision for them.

        Also, does this change in behavior have any impact on the normal callpath from a performance perspective? Pinaki is hinting that it will affect performance. If we are doing additional processing on every generated SQL and access to the FinderCache, then are we negating the benefits of the cache?

        One alternative is to modify the FinderCache so that it could take the FetchPlan settings into account. Whether this extra processing would offset the benefit of the cache would have to be determined. Regardless, this is too big of an effort for the service streams. I would suggest creating a sub-JIRA feature for this effort so that we have it on the books. But, stick with this conservative approach until this sub-feature is resolved.

        That's my two cents worth. But, don't just re-close this JIRA without coming to some type of agreement. I've posted a few questions that should be discussed and resolved. Thanks.

        Show
        Kevin Sutter added a comment - Oh sure, Thanks Jody for hauling me into this discussion... I will agree that the current usage scenario was not intuitive or documented. And, since we have customers hitting this condition of accidentally caching the "wrong" SQL, I agree that we need to do something in the service streams to resolve the issue. And, considering the 80/20 rule, it seems that erring on the default/conservative side definitely hits the 80% side of the market. That all being said, Pinaki does have a valid point that we are now alienating users that use fetch plans from using the FinderCache. But, that is also consistent with Pinaki's direction to have the application control whether these generated SQL's should be cached or not. We're just making that decision for them. Also, does this change in behavior have any impact on the normal callpath from a performance perspective? Pinaki is hinting that it will affect performance. If we are doing additional processing on every generated SQL and access to the FinderCache, then are we negating the benefits of the cache? One alternative is to modify the FinderCache so that it could take the FetchPlan settings into account. Whether this extra processing would offset the benefit of the cache would have to be determined. Regardless, this is too big of an effort for the service streams. I would suggest creating a sub-JIRA feature for this effort so that we have it on the books. But, stick with this conservative approach until this sub-feature is resolved. That's my two cents worth. But, don't just re-close this JIRA without coming to some type of agreement. I've posted a few questions that should be discussed and resolved. Thanks.
        Hide
        Jody Grassel added a comment -

        An update will be coming soon, adding a Compatibilty option that can turn off the changes introduced by this JIRA.

        Show
        Jody Grassel added a comment - An update will be coming soon, adding a Compatibilty option that can turn off the changes introduced by this JIRA.
        Hide
        ASF subversion and git services added a comment -

        Commit 1529292 from Jody Grassel in branch 'openjpa/branches/2.1.x'
        [ https://svn.apache.org/r1529292 ]

        OPENJPA-2414: FinderCache does not consider active Fetch Groups/FetchPlan added Fields

        Show
        ASF subversion and git services added a comment - Commit 1529292 from Jody Grassel in branch 'openjpa/branches/2.1.x' [ https://svn.apache.org/r1529292 ] OPENJPA-2414 : FinderCache does not consider active Fetch Groups/FetchPlan added Fields
        Hide
        ASF subversion and git services added a comment -

        Commit 1530789 from Jody Grassel in branch 'openjpa/branches/2.2.1.x'
        [ https://svn.apache.org/r1530789 ]

        OPENJPA-2414: FinderCache does not consider active Fetch Groups/FetchPlan added Fields

        Show
        ASF subversion and git services added a comment - Commit 1530789 from Jody Grassel in branch 'openjpa/branches/2.2.1.x' [ https://svn.apache.org/r1530789 ] OPENJPA-2414 : FinderCache does not consider active Fetch Groups/FetchPlan added Fields
        Hide
        ASF subversion and git services added a comment -

        Commit 1532797 from Jody Grassel in branch 'openjpa/branches/2.2.x'
        [ https://svn.apache.org/r1532797 ]

        OPENJPA-2414: FinderCache does not consider active Fetch Groups/FetchPlan added Fields

        Show
        ASF subversion and git services added a comment - Commit 1532797 from Jody Grassel in branch 'openjpa/branches/2.2.x' [ https://svn.apache.org/r1532797 ] OPENJPA-2414 : FinderCache does not consider active Fetch Groups/FetchPlan added Fields
        Hide
        ASF subversion and git services added a comment -

        Commit 1532882 from Jody Grassel in branch 'openjpa/trunk'
        [ https://svn.apache.org/r1532882 ]

        OPENJPA-2414: FinderCache does not consider active Fetch Groups/FetchPlan added Fields

        Show
        ASF subversion and git services added a comment - Commit 1532882 from Jody Grassel in branch 'openjpa/trunk' [ https://svn.apache.org/r1532882 ] OPENJPA-2414 : FinderCache does not consider active Fetch Groups/FetchPlan added Fields
        Hide
        ASF subversion and git services added a comment -

        Commit 1536793 from Jody Grassel in branch 'openjpa/branches/2.3.x'
        [ https://svn.apache.org/r1536793 ]

        OPENJPA-2414: FinderCache does not consider active Fetch Groups/FetchPlan added Fields

        Show
        ASF subversion and git services added a comment - Commit 1536793 from Jody Grassel in branch 'openjpa/branches/2.3.x' [ https://svn.apache.org/r1536793 ] OPENJPA-2414 : FinderCache does not consider active Fetch Groups/FetchPlan added Fields

          People

          • Assignee:
            Jody Grassel
            Reporter:
            Jody Grassel
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development