Node removal not working with Invalidation?

[brian.stansberry]

I'm seeing weird behavior in my unit tests of Hibernate/JBC integration with invalidation, so thought I'd look for JBC testsuite tests of node removal with invalidation. Didn't find any. So, wrote the following which fails at the commented point.

Basically, a node removal doesn't seem to propagate. An equivalent test with replication instead of invalidation passed.

Am I missing something obvious here? If not I'll check in the test and raise a JIRA.

 public void testPessimisticNodeRemoval() throws Exception
 {
 nodeRemovalTest(false);
 }

 public void testOptimisticNodeRemoval() throws Exception
 {
 nodeRemovalTest(true);
 }

 @SuppressWarnings("unchecked")
 private void nodeRemovalTest(boolean optimistic) throws Exception
 {
 CacheImpl<Object, Object> cache1 = null;
 CacheImpl<Object, Object> cache2 = null;
 try
 {
 cache1 = createCache(optimistic);
 cache2 = createCache(optimistic);

 Node root1 = cache1.getRoot();
 Node root2 = cache2.getRoot();

 // this fqn is relative, but since it is from the root it may as well be absolute
 Fqn<String> fqn = Fqn.fromString("/test/fqn");
 cache1.getInvocationContext().getOptionOverrides().setCacheModeLocal(true);
 root1.addChild(fqn);
 assertTrue(root1.hasChild(fqn));
 cache2.getInvocationContext().getOptionOverrides().setCacheModeLocal(true);
 root2.addChild(fqn);
 assertTrue(root2.hasChild(fqn));

 assertEquals(true, cache1.removeNode(fqn));
 assertFalse(root1.hasChild(fqn));

 // THIS ASSERTION FAILS!
 assertFalse(root2.hasChild(fqn));

 assertEquals(false, cache1.removeNode(fqn));

 finally
 {
 // stop caches, etc.
 }
 }

[manik]

Is this test in SVN?

Does it fail in both optimistic and pessimistic cases? Since in the optimistic case, as per JBCACHE-1155, the node won't be removed, but just evicted as though it had children.

Perhaps you should be testing for root2.getChild(fqn).getKeys().isEmpty() ? Ideally the node's validity should be set to false as well ...

[brian.stansberry]

Ah, yes. That makes sense. I didn't check the test in yet; wanted to post here first and see if I was conceptually off base -- which it seems I was. :-)

I'll change the test along the lines you suggest and commit it. Seems Node.isValid() won't work yet; it always returns true with a TODO to implement it. I'll add a minor JIRA to fix the test once isValid() is implemented.

I wrote a similar test with pessimistic locking and it passed fine.

[brian.stansberry]

Test is committed; minor JIRA to improve it is http://jira.jboss.com/jira/browse/JBCACHE-1199 .

This is a bit conceptually murky though, since the removeNode call actually removes the node on the cache where the call was made, but leaves it around on remote nodes. For example, a unit test that passes would look like this (assume the isValid() bit works):

assertEquals(true, cache1.removeNode(fqn));

// Node should actually be gone locally
assertFalse(root1.hasChild(fqn));

// Node should still exist but be "invalid" on remote caches
Node remoteNode = root2.getChild(fqn);
assertNotNull(remoteNode);
assertFalse(remoteNode.isValid());

assertEquals(false, cache1.removeNode(fqn));

[brian.stansberry]

Drat; there's a serious problem here when the removed node has children:

http://jira.jboss.com/jira/browse/JBCACHE-1200

[manik]

Here's a thought re: nodes being removed locally but still living as tombstones on remote nodes (as a placeholder for the version that last existed). Isn't there enough of an argument that says even the locally removed node should not be removed, but instead just have it's data removed and then marked as invalid?

The same argument for the need of the last used version will apply, since the node may be re-added anywhere else in the cluster, potentially with an older version.

Agreed that this does become a problem with regards to memory footprint, at least until eviction kicks in ...

[brian.stansberry]

Agreed; if leaving tombstones around on remote nodes makes sense, it makes sense to do it locally as well.

I was trying to think how this could break things; e.g. app removes some data and then tries to recreate it. Say an admin creates a "user" account entity; realizes he's mucked it up, deletes it and starts over with the same id. He could get version conflicts when he starts over.

But:

1) With the present approach of tombstones on remote nodes, that problem already exists, if the admin happens to work on a different node when he starts over.

2) If implicit data versioning is used, it wouldn't be a problem.

3) If explicit data versioning is used, it could be a problem. But perhaps that can be handled in the Hibernate/JBC integration, which separately handles cache insertions of newly created entities.

[manik]

I was trying to think how this could break things; e.g. app removes some data and then tries to recreate it. Say an admin creates a "user" account entity; realizes he's mucked it up, deletes it and starts over with the same id. He could get version conflicts when he starts over.

Well, not necessarily. Version != identity. He would be allowed to add it again, the version would increment without a problem. Where it *does* step in is if 2 admins attempt to create the same id, but that is a valid conflict anyway.

3) If explicit data versioning is used, it could be a problem. But perhaps that can be handled in the Hibernate/JBC integration, which separately handles cache insertions of newly created entities.

Again, it shouldn't be a problem if used correctly. Explicit versioning should only be used if there is an external "true representation of state" (such as a database) which also maintains versioning for that state. Explicit versioning is only meant to be a way to expose the versioning maintained by the database.

[brian.stansberry]

In my example above, identity == userId, which would presumably be unchanged. If they used some system with synthetic primary keys and a uniqueness constraint on the userId, then no problem.

Re: explicit versioning being meant to represent database versioning, true enough, but using a simple increment counter is a pretty common way to do database versioning.

Note that all of my discussion here isn't meant to advocate against keeping a tombstone on the local node; just trying to identify problem areas and see how significant. Seems like this is a pretty small corner case that's preventable by using a synthetic primary key or a non-repeatable db version.

[manik]

re: identity: It doesn't matter if the user id doesn't change. E.g.:

1) Created user (id=1, version=1)
2) Removed user (id=1, version=2) - a tombstone
3) CReated user again (id=1, version=3)

The above should be perfectly valid, even if steps 1 - 3 occur on different cache instances in the cluster.

[manik]

Re: External versioning

Agreed, even a simple counter is a valid external version, but one that is maintained *inside* the database in question - either using some db-specific versioning API or a simple table in the db and using a SQL increment function - and hence consistent across a cluster.

[brian.stansberry]

Doh, of course. So, our corner case is now down to really stupid usages where they don't properly use the version.

And even there the cost is they just have to wait a while for the node to be evicted.

You're probably laughing at me for all this, but I bet you a beer that someday this thread gets pointed to in a support case. :-)