3 Replies Latest reply on May 9, 2007 11:17 AM by jamieqho

    ExpirationAlgorithm (2.0.0.CR1) doesn't work with expiry dat

    jamieqho

      Hi,

      In ExpirationAlgorithm.java, elements are being removed from a TreeMap like this:

      boolean found = set.remove(new ExpirationEntry(fqn));

      This absolutely does not work because your ExpirationEntry objects are sorted by expiration date in the Tree (as specified by your compareTo method). The code from Sun does a tree walk and cannot find an ExpirationEntry matching yours because you didn't specify an expiry date.

      The end result is two ExpirationEntry objects in the set with the same fqn, but different expiry dates.

      I was very confused by this and wondered why my objects were expirying out of the cache, even though I periodically bumped up the expiry date. Now I realize this is happening because the old expiry date was never properly removed from your set of ExpirationEntry objects.

      Is this is known bug? If no, will it be fixed soon?

      Thanks,
      Jamie

        • 1. Re: ExpirationAlgorithm (2.0.0.CR1) doesn't work with expiry
          genman

          Jamie,

          I wrote the code you're having issues with.

          This is definitely a bug.
          http://jira.jboss.com/jira/browse/JBCACHE-1051

          If you have an elegant solution (patch) it would be appreciated. This will definitely get fixed in the next release, however, elegant or not.

          • 2. Re: ExpirationAlgorithm (2.0.0.CR1) doesn't work with expiry
            genman

            The most elegant solution I could come up with is to never delete the ExpirationEntry, but double-check the node's current expiration value before eviction. The solution looks like this:

            Index: src/org/jboss/cache/eviction/ExpirationAlgorithm.java
            ===================================================================
            RCS file: /cvsroot/jboss/JBossCache/src/org/jboss/cache/eviction/ExpirationAlgorithm.java,v
            retrieving revision 1.9
            diff -u -r1.9 ExpirationAlgorithm.java
            --- src/org/jboss/cache/eviction/ExpirationAlgorithm.java 7 Feb 2007 22:06:56 -0000 1.9
            +++ src/org/jboss/cache/eviction/ExpirationAlgorithm.java 9 May 2007 08:04:08 -0000
            @@ -92,21 +92,19 @@
            
             private void setExpiration(Fqn fqn, Long l)
             {
            - boolean found = set.remove(new ExpirationEntry(fqn));
            - if (found && log.isTraceEnabled())
            - log.trace("removed old expiration for " + fqn);
             ExpirationEntry ee = new ExpirationEntry(fqn, l);
             if (log.isTraceEnabled())
             log.trace("adding eviction entry: " + ee);
             set.add(ee);
             }
            
            + @SuppressWarnings("unchecked")
             private Long getExpiration(Fqn fqn)
             {
            - NodeSPI n = policy.getCache().peek(fqn, false);
            + NodeSPI<String, Long> n = policy.getCache().peek(fqn, false);
             if (n == null)
             return null;
            - return (Long)n.getDirect(config.getExpirationKeyName());
            + return n.getDirect(config.getExpirationKeyName());
             }
            
             @Override
            @@ -126,7 +124,8 @@
             case REMOVE_ELEMENT_EVENT:
             case REMOVE_NODE_EVENT:
             case UNMARK_USE_EVENT:
            - removeEvictionEntry(node);
            + // Removals will be noticed when double-checking expiry time
            + // removeEvictionEntry(node);
             break;
             case VISIT_NODE_EVENT:
             // unused
            @@ -161,6 +160,13 @@
             for (Iterator<ExpirationEntry> i = set.iterator(); i.hasNext();)
             {
             ExpirationEntry ee = i.next();
            + Long ce = getExpiration(ee.getFqn());
            + if (ce == null || ce > ee.getExpiration())
            + {
            + // Expiration now older
            + i.remove();
            + continue;
            + }
             if (ee.getExpiration() < now || (max != 0 && set.size() > max))
             {
             i.remove();
            @@ -177,13 +183,6 @@
             "Set expiration for nodes in this region");
             }
            
            - private void removeEvictionEntry(EvictedEventNode node)
            - {
            - Fqn fqn = node.getFqn();
            - if (getExpiration(fqn) == null)
            - set.remove(new ExpirationEntry(fqn));
            - }
            -
             @Override
             public void resetEvictionQueue(Region region)
             {
            @@ -230,12 +229,10 @@
             }
            
             /**
            - * Returns true if the FQN are the same, else compares expiration, then FQN order.
            + * Compares expiration, then FQN order.
             */
             public int compareTo(ExpirationEntry ee)
             {
            - if (fqn.equals(ee.fqn))
            - return 0;
             long n = expiration - ee.expiration;
             if (n < 0)
             return -1;
            @@ -265,12 +262,12 @@
             if (!(o instanceof ExpirationEntry))
             return false;
             ExpirationEntry ee = (ExpirationEntry) o;
            - return fqn.equals(ee.fqn);
            + return expiration == ee.expiration && fqn.equals(ee.fqn);
             }
            
             public int hashCode()
             {
            - return fqn.hashCode();
            + return (int)expiration ^ fqn.hashCode();
             }
            
             public String toString()
            
            


            • 3. Re: ExpirationAlgorithm (2.0.0.CR1) doesn't work with expiry
              jamieqho

              I don't have a more elegant solution. Thanks for the fix!