[JBCACHE-538] Integrating transferred persistent state under

[brian.stansberry]

JBCACHE-538 deals with the fact that in the BR context, transferred state has to be integrated ina different place than indicated by the Fqns included with the transferred state, e.g.

if DataOwner 192.168.1.2:5555 transferred state for a node with Fqn /a, the recipient should integrate it into node /_BUDDY_BACKUP_/192.168.1.2:5555/a

I've realized that my implementation of this for 1.4.0.Alpha was incomplete; the in-memory state was handled correctly, but the persistent state was not handled, and would have been stored in the persistent store under /a.

This is not trivial to fix, because storage of persistent state transfers is done via a call to CacheLoader.storeEntireState(byte[]), where the byte[] is the state transferred from the other node. The CacheLoader API does not include a method that says "store this byte[], but make sure the data ends up under this Fqn".

ExtendedCacheLoader does expose a more useful method:

void storeState(byte[] state, Fqn subtree) throws Exception;

the subtree parameter is there so the ExtendedCacheLoader can pass it to any marshalling RegionManager to find a classloader to use to unmarshal the state.

The described behavior of the method could be changed so that if the transferred state does not already sit under the given Fqn, the ExtendedCacheLoader should integrate it under that Fqn.

Unfortunately, doing that means changing the prescribed behavior of an
already existing interface, e.g. from:

/**
 * Store the given portion of the cache tree's state in secondary storage.
 * Overwrite whatever is currently in secondary storage.
 *
 * @param state the state to store
 * @param subtree Fqn naming the root (i.e. highest level parent) node of
 * the subtree included in <code>state</code>.
 */
 void storeState(byte[] state, Fqn subtree) throws Exception;

to

/**
 * Store the given portion of the cache tree's state in secondary storage.
 * Overwrite whatever is currently in secondary storage. Ensure that
 * the state is integrated under the given <code>subtree</code>.
 *
 * @param state the state to store
 * @param subtree Fqn naming the root (i.e. highest level parent) node of
 * the subtree included in <code>state</code>. If the Fqns
 * of the data included in <code>state</code> are not
 * already children of <code>subtree</code>, then their
 * Fqns should be altered to make them children of
 * <code>subtree</code> before they are persisted.
 */
 void storeState(byte[] state, Fqn subtree) throws Exception;

Going with this solution has major downsides

1) If BR is used, the configured cache loader must implement ExtendedCacheLoader. Otherwise, we throw an ISE during startService.
2) We're changing behavior of an existing interface.

An alternative is to create yet another interface "BuddyReplicationCacheLoader extends ExtendedCacheLoader", with the only difference being the new Javadoc requirement. This way at least, users with existing custom ExtendedCacheLoader impls will know they need to change behavior to use their cache loader with BR.

Also, the existing DelegatingCacheLoaders don't implement ExtendedCacheLoader. Actually, looking at them they don't properly support the state transfer methods from CacheLoader either.

Thoughts? I should be able to implement either of the above tomorrow (except for dealing with DelegatingCacheLoader) if we decide to go that route.

[manik]

I'm not too keen with yet another cache loader interface - and in fact I hope to consolidate these interfaces in Habanero.

Can we not use this existing interface if we assume that the node transmitting the state modifies the Fqns accordingly - in BuddyManager:

public static Fqn getBackupFqn(Object buddyGroupName, Fqn fqn)

This way the node receiving state doesn't need to know about or care about modifying Fqns.

[brian.stansberry]

+1 re: consolidating in Habanero. We have the flexibility there to redefine the API meaning.

If we put the responsibility of modifying the Fqns on the state sender side, we have the same problem w/ the CacheLoader interface, just moved to the loadEntireState()/loadState(Fqn) calls. The basic problem is that the CacheLoader interface contract doesn't cover doctoring Fqns.

In the StateTransferIntegrator we could take the byte[] w/ the persistent state, deserialize it, fix the Fqns, reserialize it and pass it to the CacheLoader. But, besides being the nastiest thing I've heard in a long time, it's not valid either as the CacheLoader API says nothing about what the format of the byte[] is -- that's up to the CacheLoader implementation.

[manik]

If we put the responsibility of modifying the Fqns on the state sender side, we have the same problem w/ the CacheLoader interface, just moved to the loadEntireState()/loadState(Fqn) calls. The basic problem is that the CacheLoader interface contract doesn't cover doctoring Fqns.

In the StateTransferIntegrator we could take the byte[] w/ the persistent state, deserialize it, fix the Fqns, reserialize it and pass it to the CacheLoader. But, besides being the nastiest thing I've heard in a long time, it's not valid either as the CacheLoader API says nothing about what the format of the byte[] is -- that's up to the CacheLoader implementation.

Good point. And you're right, having the state transfer integrator doctor the byte stream is very nasty.

[brian.stansberry]

Any further thoughts on this? Getting the persistent state transfer right would be a good thing to have in Beta2.

[manik]

I guess we go with the extended cache loader route then. We would have to update the Javadocs and make it clear that the symantics of this method implementation has changed, and throw an exception if a cache loader (as opposed to extended cache loader) is configured alongside BR.

Sucks a little, but I don't see any other way around this. Be sure to post a comment about this in the Habanero interfaces thread so we cover something to this effect in the new CacheLoader interface.

[brian.stansberry]

I'm considering whether ChainingCacheLoader should implement ExtendedCacheLoader as well. My instinct says yes; should be simple -- the loadState(Fqn) and storeState(byte[], Fqn) would work just like loadEntireState()/storeEntireState(), except the call would only be passed to cache loaders in the chain that implement ExtendedCacheLoader.

The question then becomes, how does the TreeCache know whether ExtendedCacheLoader is really supported? Probably best is to expose that as an attribute of CacheLoaderManager, similar to what we do with CacheLoaderManager.isFetchPersistentState().

Without objection or a better idea, I'll implement it this way in the morning.

[manik]

I'm considering whether ChainingCacheLoader should implement ExtendedCacheLoader as well. My instinct says yes; should be simple -- the loadState(Fqn) and storeState(byte[], Fqn) would work just like loadEntireState()/storeEntireState(), except the call would only be passed to cache loaders in the chain that implement ExtendedCacheLoader.

Good point. It should. And if the underlying cache loaders are not 'extended' cache loaders, don't pass the loadState/storeState methods to them, or throw an exception?

The question then becomes, how does the TreeCache know whether ExtendedCacheLoader is really supported? Probably best is to expose that as an attribute of CacheLoaderManager, similar to what we do with CacheLoaderManager.isFetchPersistentState().

Yes, the CacheLoaderManager can test whether the elements of the chain are all extended cache loaders or not.

[brian.stansberry]

And if the underlying cache loaders are not 'extended' cache loaders, don't pass the loadState/storeState methods to them, or throw an exception?

Last night I was thinking skip the 'non-extended' loaders, but that seems wrong. If a cache loader is configured with FetchPersistentState as true, it needs to be able to handle the cache's state transfer requirements. If it can't, the user can just set FetchPersistentState to false for that cache loader. So, I'd say throw an Exception.

Yes, the CacheLoaderManager can test whether the elements of the chain are all extended cache loaders or not.

So, I'd say CacheLoaderManager returns true to "isExtendedCacheLoader" if either:

1) It would return false to isFetchPersistentState. ChainingCacheLoader does implement ExtendedCacheLoader, and no ExtendedCacheLoader calls through the chain would fail because none of the cache loaders will try to handle persistent state calls.

2) It would return true to isFetchPersistent state and *all* cache loaders in the chain that are configured to handle persistent state are ExtendedCacheLoaders.

[manik]

So, I'd say CacheLoaderManager returns true to "isExtendedCacheLoader" if either:

1) It would return false to isFetchPersistentState. ChainingCacheLoader does implement ExtendedCacheLoader, and no ExtendedCacheLoader calls through the chain would fail because none of the cache loaders will try to handle persistent state calls.

2) It would return true to isFetchPersistent state and *all* cache loaders in the chain that are configured to handle persistent state are ExtendedCacheLoaders.

Almost. With chaining cache loaders, we only fetch persistent state from *one* underlying cache loader (handled transparently by the chaining cache loader). If you configure more than one cache loader with fetchPersistentState to true, I throw an exception.

So perhaps we only need to enforce ExtendedCacheLoader on the cache loader configured with fetchPersistentState to true?

[brian.stansberry]

So perhaps we only need to enforce ExtendedCacheLoader on the cache loader configured with fetchPersistentState to true?

Yep, we're on the same page :)

[manik]

NIce to know. :-) +1 from me then.