[jcs] fixes

[Christian Kreutzfeldt <ch...@gmx.de>]

Hi!

I just started playing around with jcs again and I came across a problem
that I figured out when I used jcs the last time. If my mind serves me right,
we had a short discussion about that months ago. It is about the 
IndexedDiskCache.

When I place elements in that cache and call

CompositeCacheManager.getInstance().getCache("indexedCache").dispose();

(where indexedCache is referenced to the IndexedDiskCache implementation in 
the cache.ccf)

none of the elements gets saved to the disk. Okay, the time, my testing 
program is running, is quite short, but overall I expect the cache to save my 
items. I just started skimming through the source code to figure out how the 
control flow is organized in this project.
During that process, I came acress some things which I found that they would 
be easier to understand if they get changed. I hope it is not too affronting 
to tell you my opinion :-)

1. 
I found out that you use quite a lot of inner classes. Wouldn't it be good      
to extract these to own java files? You would probably need some new packages 
to keep track of all the classes and structures, but it would help to 
understand the flow of control.

For example, all these Event-Classes (Put, Remove etc) in CacheEventQueue 
could be easily extracted, the CacheListener in AbstractDiskCache. Especially 
the last class is a little bit tricky to understand because the listener 
class calls a method doUpdate which is referenced as method of 
AbstractDiskCache. Why don't you pass a parent object of type 
AbstractDiskCache as constructor parameter and call doUpdate on that object?

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

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

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)

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.

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.

If you are interested in my source code, I could mail it to you.

With regards,
  Christian Kreutzfeldt

Re: [jcs] fixes

[James Taylor <ja...@jamestaylor.org>]

> 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