OFBiz
  1. OFBiz
  2. OFBIZ-4341

GenericDelegator.findOne cache not working for not-found values (cached not-found treated like cache miss)

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: Trunk
    • Fix Version/s: Trunk
    • Component/s: framework
    • Labels:
      None

      Description

      GenericDelegator.findOne doesn't handle the cache consistently.

      When it doesn't find anything, it puts GenericValue.NULL_VALUE in the cache. When trying to read from the cache it uses the getFromPrimaryKeyCache method, which returns null when GenericValue.NULL_VALUE was stored in the cache, just like for cache misses. So a not-found value in the cache is treated like a cache miss and the cache isn't actually used.

      1. OFBIZ-4341-findOne-cache-fix.patch
        1 kB
        Martin Kreidenweis
      2. OFBIZ-4341.patch
        1.0 kB
        Dimitri Unruh

        Activity

        Hide
        Martin Kreidenweis added a comment -

        patch fixing the findOne cache issue for not-found values

        Show
        Martin Kreidenweis added a comment - patch fixing the findOne cache issue for not-found values
        Hide
        Martin Kreidenweis added a comment -

        Just checking up on the ticket status...

        Do you need more info or did you just not get around to applying the patch yet?

        One thing we might want to be careful about: Some code might actually depend on the buggy behavior and expect that the cache is not used for not-found primary key values. Still, the intention of the original code seems to me like it wanted to use that cache and it just didn't work. So I think we should fix this.

        We found the issue while profiling a service in our application.

        Show
        Martin Kreidenweis added a comment - Just checking up on the ticket status... Do you need more info or did you just not get around to applying the patch yet? One thing we might want to be careful about: Some code might actually depend on the buggy behavior and expect that the cache is not used for not-found primary key values. Still, the intention of the original code seems to me like it wanted to use that cache and it just didn't work. So I think we should fix this. We found the issue while profiling a service in our application.
        Hide
        Dimitri Unruh added a comment -

        Hi Martin,

        we also had the same problem with this issue. Our resolution looks very similar to your patch. The main difference is the getFromPrimaryKeyCache method.

        Maybe you will have a look at this and tell me what you think.

        Dimitri

        Show
        Dimitri Unruh added a comment - Hi Martin, we also had the same problem with this issue. Our resolution looks very similar to your patch. The main difference is the getFromPrimaryKeyCache method. Maybe you will have a look at this and tell me what you think. Dimitri
        Hide
        Martin Kreidenweis added a comment -

        Hi Dimitri,

        your patch causes the public Delegator.getFromPrimaryKeyCache() method to return GenericValue.NULL_VALUE. I don't know if that's a good idea. The NULL_VALUE looks like an internal thing to me that the cache uses because it doesn't want to store null directly. (It would have the same problem that cache misses and null-hits are harder to distinguish.) So maybe we don't want to return that value from the Delegator.

        But as I can't currently find any other uses of the getFromPrimaryKeyCache method in OFBiz code, I guess your patch is fine as well.

        Does somebody still know how the GenericValue.NULL_VALUE was originally intended to be used?

        Martin

        Show
        Martin Kreidenweis added a comment - Hi Dimitri, your patch causes the public Delegator.getFromPrimaryKeyCache() method to return GenericValue.NULL_VALUE . I don't know if that's a good idea. The NULL_VALUE looks like an internal thing to me that the cache uses because it doesn't want to store null directly. (It would have the same problem that cache misses and null-hits are harder to distinguish.) So maybe we don't want to return that value from the Delegator. But as I can't currently find any other uses of the getFromPrimaryKeyCache method in OFBiz code, I guess your patch is fine as well. Does somebody still know how the GenericValue.NULL_VALUE was originally intended to be used? Martin
        Hide
        Dimitri Unruh added a comment -

        Martin,

        I know what you mean

        So, it could be dangarous to change the Delegator.getFromPrimaryKeyCache() (for custom modifications), but anyway in my opinion the current implementation is wrong.
        If GenericValue.NULL_VALUE is in the cacht, i will get it...

        Show
        Dimitri Unruh added a comment - Martin, I know what you mean So, it could be dangarous to change the Delegator.getFromPrimaryKeyCache() (for custom modifications), but anyway in my opinion the current implementation is wrong. If GenericValue.NULL_VALUE is in the cacht, i will get it...
        Hide
        Dimitri Unruh added a comment -

        I had a look in the SVN history.
        The Delegator.getFromPrimaryKeyCache() method has been changed in rev. 589514.

        In my opinion, we should change the method.
        Any other ideas?

        Anyway, at least we got a problem here. So, if we don't change the Delegator.getFromPrimaryKeyCache() method, we should use Martins patch

        Show
        Dimitri Unruh added a comment - I had a look in the SVN history. The Delegator.getFromPrimaryKeyCache() method has been changed in rev. 589514. In my opinion, we should change the method. Any other ideas? Anyway, at least we got a problem here. So, if we don't change the Delegator.getFromPrimaryKeyCache() method, we should use Martins patch
        Hide
        Martin Kreidenweis added a comment -

        Thanks for checking it out, Dimitri.
        So considering the history of the getFromPrimaryKeyCache method I'm also OK with changing it back to returning the NULL_VALUE.

        Jacques shall decide which patch he wants to use.

        Show
        Martin Kreidenweis added a comment - Thanks for checking it out, Dimitri. So considering the history of the getFromPrimaryKeyCache method I'm also OK with changing it back to returning the NULL_VALUE. Jacques shall decide which patch he wants to use.
        Hide
        Dimitri Unruh added a comment -

        Jacques, may the Force be with you

        Show
        Dimitri Unruh added a comment - Jacques, may the Force be with you
        Hide
        Jacques Le Roux added a comment -

        Thanks Guys,

        I will have a look as soon as I will get a chance. I also hope to change methods signature, as discussed with Martin: http://markmail.org/message/rhn26erd3vtq73ag

        Show
        Jacques Le Roux added a comment - Thanks Guys, I will have a look as soon as I will get a chance. I also hope to change methods signature, as discussed with Martin: http://markmail.org/message/rhn26erd3vtq73ag
        Hide
        Adrian Crum added a comment -

        Fixed in rev 1527626.

        Show
        Adrian Crum added a comment - Fixed in rev 1527626.

          People

          • Assignee:
            Jacques Le Roux
            Reporter:
            Martin Kreidenweis
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development