Can we move this back to infinispan-dev, pls? On 9 Aug 2013, at 16:29, Mircea Markus <mmarkus(a)redhat.com&gt; wrote: > > On 6 Aug 2013, at 18:13, William Burns <wburns(a)redhat.com&gt; wrote: > >>>> Based on the feedback I received and after doing some prototyping, here's >>>> the new CacheLoader API I came up >>>> with:https://github.com/mmarkus/infinispan/tree/t_ISPN-3290/CS_redesign/c... >>>> >>>> I know everybody's quite busy but can you please take some time and review >>>> this? It's a very important API chance and this would help to getting it >>>> off on the right foot. >>>> >>>> It should cover all the gathered requirements: >>>> >>>> - support for non-distributed transaction cache stores (1PC) and support >>>> for XA capable cache store >>>> [Decision: all the transaction support is delegated to Infinispan. The >>>> cache store SPI is much simplified.] >>> >>> +1 >>> >>>> >>>> - support iteration over all the keys/entries in the store >>>> - needed for efficient Map/Reduce integration >>>> - needed for efficient implementation of Cache.keySet(), Cache.entrySet(), >>>> Cache.values() methods >>>> [look in BulkCacheLoader] >>> >>> I think you meant for bulkLoadKeys() to take in a KeyFilter, not a >>> Collection? >> >> My guess is Mircea was going for an overloaded method for bulkLoadKeys where we want both one that takes a Collection and one that takes a KeyFilter? If so it seems to me it would be simpler to just have a single method that takes KeyFilter only, but then have another class like KeyFilters that has various static factory methods that can take a Iterable or Iterator for example so we don't have too many methods on the loader itself. > > I think we'll end up needing both actually, as bulkLoadAll with a collection still makes sense. E.g. for JDBC queries it's easier to build a WHERE clause and select all the elements in one go. > > Sanne ha a good alternative suggestion to the the bulkLoadAll: > > public process(KeyFilter, j.u.c.Executor, CacheLoaderTask clt); > > and > > interface CacheLoaderTask { > //return false if don't need to process any longer > boolean process (CacheLoaderEntry cle); > } > > interface CacheLoaderEntry { > Object getKey(); > ICV getInternalCacheValue(); > //.. ongoing discussion about some other byte[] based methods > } > > > This would allow the CacheStore to iterate over the entries in parallel, whilst still allowing sequential iteration. Pretty awesome. > >> >> Also removeAll should probably take a KeyFilter similarly. > > +1 > >> >> I noticed there is an overloaded bulkLoad which takes no arguments, which I assume just streams/loads all entries from the cache store. Do we need to have similarly overloaded methods for bulkLoadKeys? If not than do we even need the no arg bulkLoad method and just have null/AllKeyFilter be passed in instead? Also similarly would we need clear method then? Just trying to think if we can make the interface have as few methods as needed. >> >> Some methods don't have types on them. I am guessing we want to put Object type just to prevent random warnings for user and CacheStore implementations. > > I'm not sure parametrized types are very useful for stores, I'll think about it a bit. > > Cheers, > -- > Mircea Markus > Infinispan lead (www.infinispan.org) > > > > -- Manik Surtani