Element.updateUpdateStatistics() resets Element.version

Type: Bug
Status: Closed
Priority: 1 Critical
Resolution: Fixed
Component/s: ehcache
Labels:

Assignee: cdennis
Reporter: mlewczuk
Created: March 29, 2010
Votes: 0
Watchers: 2
Updated:
July 27, 2012
Resolved:
December 10, 2010

Description

public final void updateUpdateStatistics() {
    lastUpdateTime = System.currentTimeMillis();
    version = lastUpdateTime;
} 

That is obviously wrong - “version” is set by application developer in order to describe the data contained within an element, so Ehcache should not touch it at all, or if “version” is needed by Ehcache’s internal procedures then you must extend the docs, couse there is no word about it.

Comments

gluck 2010-03-29

An update i.e. a put that overwrites an existing key updates the version. That way you can compare versions if you pull out of the cache and holding one locally. Why is that wrong?

Marek Lewczuk 2010-03-30

Greg, it is very good to change the version if existing key was updated, but the question is what version to use ? In current code it is always set to System.currentTimeMillis(), which means, that there is no control over versioning. See following example:

package test;

import net.sf.ehcache.Cache; import net.sf.ehcache.CacheManager; import net.sf.ehcache.Element; import net.sf.ehcache.store.MemoryStoreEvictionPolicy;

public class CacheTest {

public static void main (String[] args) throws Exception \{
	CacheManager cc = CacheManager.getInstance();
	cc.addCache(new Cache("mltest", 50, MemoryStoreEvictionPolicy.LRU, true, null, true, 0, 0, true, 120, null, null, 0, 2, false));
	Cache ca = cc.getCache("mltest");

	Element a = new Element("a key", "a value", 1L);
	ca.put(a);
	System.out.println(ca.get("a key"));
	Element b = new Element("a key", "a value", 2L);
	ca.put(b);
	System.out.println(ca.get("a key"));

	cc.shutdown();
\}

}

The output: [ key = a key, value=a value, version=1, hitCount=1, CreationTime = 1269935730249, LastAccessTime = 1269935730249 ] [ key = a key, value=a value, version=1269935730249, hitCount=1, CreationTime = 1269935730249, LastAccessTime = 1269935730249 ]

The element “a key” was updated, before update its version was “1” and after update it is the time of last update, but look at the second element constructor - the version was set to “2”, which means, that in second print version should also bo “2”. In other words, property “version” should have a value that was given in last update. In other case comparing versions will be useless, cause I don’t know anything about version “1269935730249”, for me last and most current version is “2”.

Fiona OShea 2010-03-30

We could add a plugible versioning strategy of some sort. What would you like to do?

gluck 2010-03-30

Marek

Ok, I get it.

I took your code and worked it into a test which I added to CacheTest. It documents the correct behaviour.

/** * Versioning is broken when updates are done. If an Element constructor specifying a version is used, it should * be preserved. If not the version should start at one and then be incremented. * * todo This test fails. When the implementation is corrected it will pass. This test is therefore currently marked @Ignore * * See EHC-666 */ @Test @Ignore public void testVersioningShouldBePreserved() {

    CacheManager cacheManager = CacheManager.getInstance();
    cacheManager.addCache(new Cache("mltest", 50, MemoryStoreEvictionPolicy.LRU, true, null, true, 0, 0, false, 120, null, null, 0, 2, false));
    Cache cache = cacheManager.getCache("mltest");

    Element a = new Element("a key", "a value", 1L);
    cache.put(a);
    Element aAfter = cache.get("a key");
    assertEquals(1L, aAfter.getVersion());

    LOG.info("Element after first put with specific version." + aAfter);

    //A second put of the same key, where the version is not explicitly mentioned, gets updated by the cache.
    Element b = new Element("a key", "a value");
    cache.put(b);
    Element bAfter = cache.get("a key");
    assertFalse(1L == bAfter.getVersion());
    LOG.info("Element after second put. No version." + bAfter);

    //Explicit Version should be preserved
    Element c = new Element("a key", "a value", 3L);
    cache.put(c);
    LOG.info("Element after third put with specific version." + cache.get("a key"));
    Element cAfter = cache.get("a key");
    assertEquals(3L, cAfter.getVersion());

}

gluck 2010-03-30

I have evaluated this bug. It is a real bug and an important one. I added testVersioningShouldBePreserved to CacheTest with an @Ignore annotation.

gluck 2010-03-30

This should be simple to fix but we need to make sure we don’t add anything to increase the size of Element.

Maybe a transient Boolean field called versionExplicitlySet which is set to True on Element construction when one of the version methods has been used. The on the way into the cache when Element.updateUpdateStatistics is called, it checks that value. If True then the version in Element i used, otherwise it is incremented. Either way versionExplicitlySet is set to null so there is no memory increase in Element once it is in the Cache.

Steve Harris 2010-03-30

Thanks greg, chris can you take a look

Marek Lewczuk 2010-03-31

Greg, making a transient Boolean field will not work, cause you must remember, that element can be serialized to a file - if that will happen, then after deserialization you will not be able to verify whether version was explicitly set or no. In my opinion the best what you can do is to modify getVersion in following way:

public long getVersion () { if (version == 0L) return lastUpdateTime; return version; }

Additionaly you can create new method, that tell whether version was explicitly set or not:

public boolean isVersionExplicitlySet () { return version != 0L; }

That change requires change of Element constructors - they need to pass 0 as default version.

Marek Lewczuk 2010-04-17

Do you plan to do something with that issue ? I need to know - if the behaviour won’t be changed, then it means, that I can not use versioning the way I used it.

Fiona OShea 2010-04-20

Marek, We are targeting this issue to be reviewed and worked on in a future release. Please vote on it to ensure it gets added to the todo list. Regards Fiona

Chris Dennis 2010-06-17

I agree with Marek that the version status will have to survive serialization so a transient marker is out. I also however do not like Marek’s solution since it assigns some special meaning to a user set-able value. I imagine a version of 0 is not going to be uncommon, and even if we pick some random long value for the special meaning, it’s just asking for trouble later.

Personally I think we should either:

never touch version at all, and let the user control it completely.
increment the version on every update (overwrite) of the Element (element.setVersion(oldElement.getVersion() + 1)), but otherwise leave it untouched
Leave everything as is, and not the break existing behaviors. Users who want special behavior can embed the version inside their own value object.

Opinions?

Marek Lewczuk 2010-06-18

Chris, for me current behaviour is just wrong - as I wrote in my previous comments: if user sets a version then it cannot be changed unless user decides to change it. If my proposal to use “0” as a special pointer (whether version was set by the user) is not acceptable, then I think that version should not be touched at all - only user should control it. If I want to know, whether element was changed then I have other methods like getLastUpdate() and version is not a right place to notify about those changes.

Thanks.

gluck 2010-08-30

This issue is also related to EHC-765 where a user has reported that using System.currentTimeMillis() does not work reliably if two updates occur in the same millisecond. Add to that Windows which is +- 10 ms and it is exacerbated.

I think we should take this opportunity to change versions over to numbers that:

By default start with 1 on first put
If a user specifies a starting value for the long, then we start with that
increment on each update.

That nicely resolves both JIRAs.

Fiona OShea 2010-10-12

Per Mike: This should not hold Magnum

Chris Dennis 2010-12-09

Sorry to revive what looks like a settled conversation, but implementing this solution could have a major performance impact for quite a lot of users. Implementing this has two main effects:

In order to overwrite a mapping we have to read the currently stored mapping - this might mean faulting in from disk, or reading from the TC server both of which are expensive and needless if the user doesn’t care about the version attribute (which most I think don’t).
We’ll also need to implement this behavior in every one of our store implementations since the increment of the version is intended to be atomic, and hence must be done within whatever locking the store normally implements.

I think that this solution risks imparting a performance impact on every user just for the benefit of the small number of users who use the version attribute. I would much prefer the solution proposed by Marek, that we grant the user complete control over the version attribute and to not mutate it at all internally. This prevents there being any performance impact for the bulk of users, and allows the user the flexibility to use it as they see fit.

Chris

Chris Dennis 2010-12-10

As agreed with Greg via email I fixed this as per my last comment.