Mailing-List: contact turbine-jcs-dev-help@jakarta.apache.org; run by ezmlm
Subject: Re: [jcs] fixes
From: James Taylor <james@jamestaylor.org>
To: Turbine JCS Developers List <turbine-jcs-dev@jakarta.apache.org>
In-Reply-To: <200308071644.33392.christian.kreutzfeldt@gmx.de>
References: <200308071644.33392.christian.kreutzfeldt@gmx.de>
Content-Type: text/plain
Organization: 
Message-Id: <1060268525.9059.7.camel@bubblehouse.local>
Mime-Version: 1.0
Date: 07 Aug 2003 11:02:06 -0400
Content-Transfer-Encoding: 7bit


> 1. 
> I found out that you use quite a lot of inner classes

Well, I for one may be too fond of inner classes, but it seems more
clear to me that way it is, at least in the case of the EventQueue stuff

> 
> 2. 
> When I call 
> CompositeCacheManager.getInstance().getCache("indexedCache").dispose();
> CompositeCache#dispose(boolean) is called. Here the alive flag is set to 
> false. Unfortunately IndexedDiskCache#doUpdate( ICacheElement ) relies on 
> that flag. 
> If the flag is set to false, the save operation never gets called. And that is 
> what happens, when I try to dispose the cache. So, I have to wait for the 
> QProcessor (which I also would move into an own class) to process the whole 
> queue before I can call dispose.
> 
> What about the following idea. I added a method boolean isEmpty() to the 
> ICacheEventQueue class which returns the result of tail == head which 
> indicates that the queue is empty. When I call dispose at the 
> AbstractDiskCache, the method dispose waits as long as 
> ICacheEventQueue#isEmpty returns true. This leads to a clean shutdown - in 
> the case of the indexed disk cache

Have you tried this? I'd be happy to look at a patch (but what I'd
really like is one or more unit tests that demonstrate the problem!)

> 3. 
> As mentioned in (2), the alive flag is from my point of view, a kind of a show 
> stopper when it comes when it comes to the final clean up. It prevents some 
> highly required methods from being run eg. the save process of the keys and 
> its associated values.
> From my point of view, only one method in the whole dispose process should 
> have have the right to set alive to false. And I think it should be done by 
> that method that handles the dispose request, which is in my case 
> IndexedDiskCache#doDispose. Therefore I kicked the line alive = false from 
> the following classes:
> 
> AbstractDiskCache#dispose() - I also added a check that waits for the 
> QProcessor to finish the event queue
> 
> CompositeCache#dispose(boolean)
> 
> Additionally, I added the method isEmpty to the following classes. This method 
> is invoked by AbstractDiskCache#dispose() which waits till the last element 
> has been processed by the cache processor.
> 
> ICacheEventQueue - added isEmpty to the interface
> CacheEventQueue - implemented the interface method

Ditto

> 4. 
> Concerning the properties loader. Why do you only supply a method that loads 
> the properties via the class loader? I added for myself a method that 
> retrieves the config file from every position you want. (eg. RemoteUtils)

No good reason for this. Happy to see more flexibility. I never changed
it because I wanted to see a more comprehensive cleanup of the config
process (better encapsulation, abstraction away from properties)

> 5. 
> It would be good to have the RMI stubs and skeletons generated automatically. 
> I had to do this by hand, never knowing which classes need to be processed. 
> Beside that I added an Exception to IRemoteCacheService#setGroupKeys because
> the compiler was complaining about the lack of this.
> This also caused a changement in RemoteCache where getGroupKeys needs to catch 
> the exception.

Yup, that would be good.

> 6. 
> The dispose operation seems to be a problem with the remote cache too. When it 
> comes to the dispose operation, an event is added to the queue. Since this is 
> the same problem as with the IndexedDiskCache, I replaced the line 
> 
> qList[i].addDisposeEvent in RemoteCacheServer#release with
> cacheDesc.cache.dispose()
> 
> The dispose operation is then in charge of what happens then. In the case of 
> the AbstractDiskCache, the dispose waits until the queue is empty.
> 
> 7. 
> The RemoteCacheServerFactory needs a checkup since the main-method causes a 
> lot of trouble. The reason for the trouble is the parsing of the input 
> parameters. It would be cool to have something like in C/C++ tools where you 
> can use this standard operation. If you want to have something like that, I 
> could have a look on that.
> 
> 8. 
> I replaced some code fragments like rca.LOCAL in RemoteCacheFactory by 
> RemoteCacheAttributes.LOCAL since it produces warnings.

(No comments on Remote stuff since I still know very little about it)

-- jt