[kepler-dev] Comments on Kepler Object Manager interface.

[Kevin Ruland]

Hi Chad.

>> 5) Possible CacheItemInvalidationPolicies:
>>   - Session: does not persist past the session
>>   - NoCache: does not cache at all.  This would be useful for the
>> QuickSearch results which might should always reexecute.
>>   - ByTime:  Compute the invalidation date some time in the future, eg
>> "1 day" or "1 week"
>>   - Never:  Never expires.  This might be painful.
>
>
> Some "never" items will be needed.  for instance, local kar files.

This would be for kar files which are somehow automatically downloaded
through Kepler?  I don't think the kar file contents should be part of
the caching system and in fact if the classloader is persued they
wouldn't exist on the filesystem at all.

>>
>> 6) Default InvalidationPolicies could be incorporated into the LSID
>> Resolver/Factory mechanism.  Then it could be configured using url
>> pattern matching from the config.xml file or other such thing.
>
>
> I don't think the lsid resolver is the place to implement invalidation
> policies.  the lsid spec is pretty narrow in its scope and i was
> trying to adhere to that.  i've stretched it a bit with the idea of a
> 'localhost' lsid.
>
It would only be a place to assigne the default invalidation policy
given the developer does not override it.  Someplace an lsid is
translated into a concrete class which does something.  This is where I
thought the default invalidation policy could be generated.  I haven't
seen the lsid spec so cannot comment on if this would be a problem. 
I'll try to track it down so I can speak intelligently in the future.

>>
>> 7) the base CacheItem object should only encapsulate its interaction
>> with the ObjectManager.  Any higher-order information should be left to
>> derived types.  I suggest the following:
>>
>> public abstract CacheItem {
>>
>>   public LSID getIdentifier();
>>   public CacheItemInvalidationPolicy getInvalidationPolicy();
>>   public void setInvalidationPolicy( CacheItemInvalidationPolicy ); /*
>> Exception free?  If the caller requests an extension of the
>> invalidation, is this a problem? */
>>   public CacheItemLockingPolicy getLockingPolicy();
>>   public void setLockingPolicy( CacheItemLockingPolicy ); /* Exceptions
>> possible if trying to loosen the locking policy? */
>>
>> }
>>
>> In particular the object should not provide *any* access to the
>> underlying object representation.  I do not believe the methods:
>> getItemAsFile() or getItemAsInputStream() belong in this class.  These
>> should be moved into a derived type which provides a Java IO interface
>> to a CacheItem.  Such a class could be "FileCacheItem", which would
>> provide seperate Input and Output methods, or "RawFileCacheItem" which
>> would provide access to the cached object's file name.  Also,
>> RawFileCacheItem should always use an Exclusive lock since it's premise
>> is Kepler does not control access.
>>
>> I do not think the constructor from LSID should be available in
>> CacheItem.  Constructing a new CacheItem should be only available from
>> ObjectManager who can delegate as appropriate to the LSID resolver.
>>
>> The static int cache type enumeration is a problem because it is a
>> static registration of the different cache item types.  Instead of using
>> such a mechanism, one should rely on "instanceof" to determine the
>> underlying representation if necessary.
>
>
> I was trying to wrap the functionality of the DataItemCache with the
> ObjectManager.  In doing so, i was trying to hide the storage object
> within the cache.  At the time, I felt that having a different class
> for each type of storage item was unneeded complexity.  If I'm proven
> to be wrong, we might have to change some parts of this.
>
This suggestion is to try to seperate the concerns of managing cached
objects from using cached objects.  The ObjectManager really shouldn't
care what was put into the cache.  There are a number of ways to make
such seperation work, one is to use inheritence.  This might be a
problem since Java prohibits multiple inheritence.  If this design is
tried, I wouldn't be surprised if we had both a CachedItemInterface and
AbstractCachedItem for a default implementation.  The AbstractCachedItem
could be derived from at the root of any inheritence trees (such as
those which currently exist in org.ecoinformatics.seek.datasource). 
I've seen this done before and it works but is slightly clunky.

I'm not familiar with DataItemCache, but have looked at DataCacheObject
and its derived types.  DataCacheObject is already an abstract class
from which concrete cached object must be derived.  The DataCacheObject
had methods for getFileName() & getLocalFileName().  It is these methods
which I think belong one layer up.

>>
>>
>> 9)  Lock policy violations:  I think we should have some exceptions
>> related to concurrent access which get raised under certain
>> circumstances.  This exception should be a RuntimeException because
>> checking this exception will provide no benefit -- At least that's what
>> I'm thinking right now.
>>
>> /* Should extend RuntimeException because there's nothing the caller can
>> do with this.
>>  * it represents an underlying programming error.
>>  */
>> class CacheConcurrentAccessException extends RuntimeException { }
>>
>> Either an exception hierarchy should be used, or simple types and
>> strings.  Exceptions should be raised in the following conditions:
>>
>> ObjectManager.getCacheItem(LSID) for a RawFileCacheItem when the
>> underlying object has already been retrieved.
>>
>> ObjectManager.AcquireReadLock( CacheItem ) when the corresponding
>> CacheItem satisfies:
>>  -  ReadExclusive lock policy and Read Lock already acquired, or
>>  -  Write Lock already acquired.  (Writes are always exclusive and
>> cannot support readers.)
>>
>> ObjectManger.AcquireWriteLock( CacheItem ) when the corresponding
>> CacheItem satisfies:
>>   - Write Lock already acquired.
>>   - Any currently opened readers exist.  (This means ObjectManager needs
>> to count Readers through the use of AcquireReadLock, even when using
>> ReadShared locking policy).
>
>
> i think we just need to work on the overall thread safety/locking for
> objectmanger.  There's basically nothing in there now that provides
> thread safety.
>
There are two different kinds of thread-safety which must be addressed. 
The first is for the ObjectManager itself which is providing brokering
services to the cached objects.  The second is thread-safety for each
individual object which is cached.  It would obviously be better if the
cached object level locks are seperated from the ObjectManager.  I don't
think this would be very hard to do.

Kevin