It seems to me that these attributes may not be common to all cache stores. For some stores, the purging can be executed as part of other process anyway and it is not desired to purge the store on-demand. Some stores may not be able to purge in parallel at all (for example some single-file store where you just can't jump in the middle of file) or would be highly inefficient. Rather than emitting warning upon improper configuration, I'd let these parameters to implementation and make the purging service optional. Radim On 08/14/2013 12:59 PM, Mircea Markus wrote: > Hi, > > I'm not sure these config attributes are needed. > > - *purgeThreads* configures the number of threads that run the storage purging (removal of expired entries from the storage). The more threads the faster the purging processes. Is there a reason for putting effort in making the purging fast (and parallel) though? The cache store implementations check if an entry is expired before returning it anyway.

> Actually I'll move the code in CacheLoader interceptor to make sure this happens for all stores. > - *purgeSynchronously*. I think the reason for this parameter is that the purging is invoked by the eviction thread. If the purging takes long then eviction is delayed. This is false by default (I doubt users change this btw) so there's a different thread than the eviction thread that runs the purging. I'd rather remove the config option and always run the purging in its own thread. > > Opinions? > > Cheers, _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

I agree with you, Sanne.

But I think from a config perspective, this does need an overhaul.

I think the correct approach - given some of the newly forming ideas on the CacheLoader/CacheStore SPI we've been having - is to not specify whether purging is synchronous or not

and not to specify a number of threads, but instead to point to a named executor. E.g., the way we define executors for async notification, async transport, etc.

- M On 14 Aug 2013, at 15:05, Sanne Grinovero <sanne(a)infinispan.org&gt; wrote: > On 14 August 2013 12:27, Radim Vansa <rvansa(a)redhat.com&gt; wrote: >> It seems to me that these attributes may not be common to all cache >> stores. For some stores, the purging can be executed as part of other >> process anyway and it is not desired to purge the store on-demand. Some >> stores may not be able to purge in parallel at all (for example some >> single-file store where you just can't jump in the middle of file) or >> would be highly inefficient. Rather than emitting warning upon improper >> configuration, I'd let these parameters to implementation and make the >> purging service optional. >> >> Radim >> >> On 08/14/2013 12:59 PM, Mircea Markus wrote: >>> Hi, >>> >>> I'm not sure these config attributes are needed. >>> >>> - *purgeThreads* configures the number of threads that run the storage purging (removal of expired entries from the storage). The more threads the faster the purging processes. Is there a reason for putting effort in making the purging fast (and parallel) though? The cache store implementations check if an entry is expired before returning it anyway. > > That's not enough.You might have lots of large entries which need to > be cleaned up in the CacheStore which are never hit by a get > operation, you still need to delete them regularly. > > On a similar reasoning, multiple threads might be needed if the > cleanup is slower than the amount of garbage the system is able to > store - agreed that's quite an extreme case, but still a possibility > for example on efficient append-only systems.. so I tend to agree with > Radim in saying that these could be CacheStore specific details. > >>> Actually I'll move the code in CacheLoader interceptor to make sure this happens for all stores. >>> - *purgeSynchronously*. I think the reason for this parameter is that the purging is invoked by the eviction thread. If the purging takes long then eviction is delayed. This is false by default (I doubt users change this btw) so there's a different thread than the eviction thread that runs the purging. I'd rather remove the config option and always run the purging in its own thread. >>> >>> Opinions? >>> >>> Cheers, >> >> _______________________________________________ >> infinispan-dev mailing list >> infinispan-dev(a)lists.jboss.org >> https://lists.jboss.org/mailman/listinfo/infinispan-dev > > _______________________________________________ > infinispan-dev mailing list > infinispan-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/infinispan-dev -- Manik Surtani _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

I agree with you, Sanne. But I think from a config perspective, this does need an overhaul. I think the correct approach - given some of the newly forming ideas on the CacheLoader/CacheStore SPI we've been having - is to not specify whether purging is synchronous or not, and not to specify a number of threads, but instead to point to a named executor.

E.g., the way we define executors for async notification, async transport, etc. - M On 14 Aug 2013, at 15:05, Sanne Grinovero <sanne(a)infinispan.org&gt; wrote: > On 14 August 2013 12:27, Radim Vansa <rvansa(a)redhat.com&gt; wrote: >> It seems to me that these attributes may not be common to all cache >> stores. For some stores, the purging can be executed as part of other >> process anyway and it is not desired to purge the store on-demand. Some >> stores may not be able to purge in parallel at all (for example some >> single-file store where you just can't jump in the middle of file) or >> would be highly inefficient. Rather than emitting warning upon improper >> configuration, I'd let these parameters to implementation and make the >> purging service optional. >> >> Radim >> >> On 08/14/2013 12:59 PM, Mircea Markus wrote: >>> Hi, >>> >>> I'm not sure these config attributes are needed. >>> >>> - *purgeThreads* configures the number of threads that run the storage purging (removal of expired entries from the storage). The more threads the faster the purging processes. Is there a reason for putting effort in making the purging fast (and parallel) though? The cache store implementations check if an entry is expired before returning it anyway. > > That's not enough.You might have lots of large entries which need to > be cleaned up in the CacheStore which are never hit by a get > operation, you still need to delete them regularly. > > On a similar reasoning, multiple threads might be needed if the > cleanup is slower than the amount of garbage the system is able to > store - agreed that's quite an extreme case, but still a possibility > for example on efficient append-only systems.. so I tend to agree with > Radim in saying that these could be CacheStore specific details.

> >>> Actually I'll move the code in CacheLoader interceptor to make sure this happens for all stores. >>> - *purgeSynchronously*. I think the reason for this parameter is that the purging is invoked by the eviction thread. If the purging takes long then eviction is delayed. This is false by default (I doubt users change this btw) so there's a different thread than the eviction thread that runs the purging. I'd rather remove the config option and always run the purging in its own thread. >>> >>> Opinions? >>> >>> Cheers, >> >> _______________________________________________ >> infinispan-dev mailing list >> infinispan-dev(a)lists.jboss.org >> https://lists.jboss.org/mailman/listinfo/infinispan-dev > > _______________________________________________ > infinispan-dev mailing list > infinispan-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/infinispan-dev -- Manik Surtani _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev