Hi, recently we had a discussion about what size() returns, but I've realized there are more things that users would like to know. My question is whether you think that they would really appreciate it, or whether it's just my QA point of view where I sometimes compute the 'checksums' of cache to see if I didn't lost anything. There are those sizes: A) number of owned entries B) number of entries stored locally in memory C) number of entries stored in each local cache store D) number of entries stored in each shared cache store E) total number of entries in cache So far, we can get B via withFlags(SKIP_CACHE_LOAD).size() (passivation ? B : 0) + firstNonZero(C, D) via size() E via distributed iterators / MR A via data container iteration + distribution manager query, but only without cache store C or D through getComponentRegistry().getLocalComponent(PersistenceManager.class).getStores() I think that it would go along with users' expectations if size() returned E and for the rest we should have special methods on AdvancedCache. That would of course change the meaning of size(), but I'd say that finally to something that has firm meaning. WDYT? Radim

On 3 October 2014 18:38, Dennis Reed <dereed(a)redhat.com&gt; wrote: > Since size() is defined by the ConcurrentMap interface, it already has a > precisely defined meaning. The only "correct" implementation is E. +1 > The current non-correct implementation was just because it's expensive > to calculate correctly. I'm not sure the current impl is really that > useful for anything. +1 And not just size() but many others from ConcurrentMap. The question is if we should drop the interface and all the methods which aren't efficiently implementable, or fix all those methods. In the past I loved that I could inject "Infinispan superpowers" into an application making extensive use of Map and ConcurrentMap without changes, but that has been deceiving and required great care such as verifying that these features would not be used anywhere in the code. I ended up wrapping the Cache implementation in a custom adapter which would also implement ConcurrentMap but would throw a RuntimeException if any of the "unallowed" methods was called, at least I would detect violations safely. I still think that for the time being - until a better solution is planned - we should throw exceptions.. alas that's an old conversation and it was never done. Sanne > -Dennis > > On 10/03/2014 03:30 AM, Radim Vansa wrote: >> Hi, >> >> recently we had a discussion about what size() returns, but I've >> realized there are more things that users would like to know. My >> question is whether you think that they would really appreciate it, or >> whether it's just my QA point of view where I sometimes compute the >> 'checksums' of cache to see if I didn't lost anything. >> >> There are those sizes: >> A) number of owned entries >> B) number of entries stored locally in memory >> C) number of entries stored in each local cache store >> D) number of entries stored in each shared cache store >> E) total number of entries in cache >> >> So far, we can get >> B via withFlags(SKIP_CACHE_LOAD).size() >> (passivation ? B : 0) + firstNonZero(C, D) via size() >> E via distributed iterators / MR >> A via data container iteration + distribution manager query, but only >> without cache store >> C or D through >> getComponentRegistry().getLocalComponent(PersistenceManager.class).getStores() >> >> I think that it would go along with users' expectations if size() >> returned E and for the rest we should have special methods on >> AdvancedCache. That would of course change the meaning of size(), but >> I'd say that finally to something that has firm meaning. >> >> WDYT? >> >> Radim >> > _______________________________________________ > 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

On 6 October 2014 12:44, Tristan Tarrant <ttarrant(a)redhat.com&gt; wrote: > I think we should provide correct implementations of size() (and others) > and provide shortcut implementations using our usual Flag API (e.g. > SKIP_REMOTE_LOOKUP). Right that would be very nice. Same for CacheStore interaction: all cachestores should be included unless skipped explicitly. Sanne > Tristan > > On 06/10/14 12:57, Sanne Grinovero wrote: >> On 3 October 2014 18:38, Dennis Reed <dereed(a)redhat.com&gt; wrote: >>> Since size() is defined by the ConcurrentMap interface, it already has a >>> precisely defined meaning. The only "correct" implementation is E. >> +1 >> >>> The current non-correct implementation was just because it's expensive >>> to calculate correctly. I'm not sure the current impl is really that >>> useful for anything. >> +1 >> >> And not just size() but many others from ConcurrentMap. >> The question is if we should drop the interface and all the methods >> which aren't efficiently implementable, or fix all those methods. >> >> In the past I loved that I could inject "Infinispan superpowers" into >> an application making extensive use of Map and ConcurrentMap without >> changes, but that has been deceiving and required great care such as >> verifying that these features would not be used anywhere in the code. >> I ended up wrapping the Cache implementation in a custom adapter which >> would also implement ConcurrentMap but would throw a RuntimeException >> if any of the "unallowed" methods was called, at least I would detect >> violations safely. >> >> I still think that for the time being - until a better solution is >> planned - we should throw exceptions.. alas that's an old conversation >> and it was never done. >> >> Sanne >> >> >>> -Dennis >>> >>> On 10/03/2014 03:30 AM, Radim Vansa wrote: >>>> Hi, >>>> >>>> recently we had a discussion about what size() returns, but I've >>>> realized there are more things that users would like to know. My >>>> question is whether you think that they would really appreciate it, or >>>> whether it's just my QA point of view where I sometimes compute the >>>> 'checksums' of cache to see if I didn't lost anything. >>>> >>>> There are those sizes: >>>> A) number of owned entries >>>> B) number of entries stored locally in memory >>>> C) number of entries stored in each local cache store >>>> D) number of entries stored in each shared cache store >>>> E) total number of entries in cache >>>> >>>> So far, we can get >>>> B via withFlags(SKIP_CACHE_LOAD).size() >>>> (passivation ? B : 0) + firstNonZero(C, D) via size() >>>> E via distributed iterators / MR >>>> A via data container iteration + distribution manager query, but only >>>> without cache store >>>> C or D through >>>> getComponentRegistry().getLocalComponent(PersistenceManager.class).getStores() >>>> >>>> I think that it would go along with users' expectations if size() >>>> returned E and for the rest we should have special methods on >>>> AdvancedCache. That would of course change the meaning of size(), but >>>> I'd say that finally to something that has firm meaning. >>>> >>>> WDYT? >>>> >>>> Radim >>>> >>> _______________________________________________ >>> 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 >> >> > _______________________________________________ > 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

On Tue, Oct 7, 2014 at 7:32 AM, Radim Vansa <rvansa(a)redhat.com&gt; wrote: > If you have one local and one shared cache store, how should the command > behave? > > a) distexec/MR sum of cache.withFlags(SKIP_REMOTE_LOOKUP, > SKIP_BACKUP_ENTRIES).size() from all nodes? (note that there's no > SKIP_BACKUP_ENTRIES flag right now), where this method returns > localStore.size() for first non-shared cache store + passivation ? > dataContainer.size(SKIP_BACKUP_ENTRIES) : 0) Calling the size method in either distexec or MR will give you inflated numbers as you need to pay attention to the numOwners to get a proper count.

> b) distexec/MR sum of sharedStore.size() + passivation ? sum of > dataContainer.size(SKIP_BACKUP_ENTRIES) : 0 Calling the size on a shared cache actually should work somewhat well (assuming all entries are stored in the shared cache). The problem is if passivation is enabled as you point out because you also have to check the data container which means you can also have an issue with concurrent activations and passivations (which you can't verify properly in either case without knowing the keys). > c) MR that would count the entries This is the only reliable way to do this with MR. And unfortunately if a rehash occurs I am not sure if you would get inconsistent numbers or an Exception. In the latter at least you should be able to make sure that you have the proper number when it does return without exception. I can't say how it works with multiple loaders though, my guess is that it may process the entry more than once so it depends on if your mapper is smart enough to realize it.

> d) wrapper on distributed entry iteration with converters set to return > 0-sized entries Entry iterator can't return 0 sized entries (just the values). The keys are required to make sure that the count is correct and also to ensure that if a rehash happens in the middle it can properly continue to operate without having to start over. Entry iterator should work properly irrespective of the number of stores/loaders that are configured, since it keep track of already seen keys (so duplicates are ignored).

> And what about nodes with different configuration? Hard to know without knowing what the differences are.

> Radim > > On 10/06/2014 01:57 PM, Sanne Grinovero wrote: >> On 6 October 2014 12:44, Tristan Tarrant <ttarrant(a)redhat.com&gt; wrote: >>> I think we should provide correct implementations of size() (and others) >>> and provide shortcut implementations using our usual Flag API (e.g. >>> SKIP_REMOTE_LOOKUP). >> Right that would be very nice. Same for CacheStore interaction: all >> cachestores should be included unless skipped explicitly. >> >> Sanne >> >>> Tristan >>> >>> On 06/10/14 12:57, Sanne Grinovero wrote: >>>> On 3 October 2014 18:38, Dennis Reed <dereed(a)redhat.com&gt; wrote: >>>>> Since size() is defined by the ConcurrentMap interface, it already has a >>>>> precisely defined meaning. The only "correct" implementation is E. >>>> +1 This is one of the things I have been wanting to do is actually implement the other Map methods across the entire cache. However to do a lot of these in a memory conscious way they would need to be ran ignoring any ongoing transactions. Actually having this requirement allows these methods to be implemented quite easily especially in conjunction with the EntryIterator. I almost made a PR for it a while back, but it seemed a little zealous to do at the same time and it didn't seem that people were pushing for it very hard (maybe that was a wrong assumption). Also I wasn't quite sure the transactional part not being functional anymore would be a deterrent. >>>>> The current non-correct implementation was just because it's expensive >>>>> to calculate correctly. I'm not sure the current impl is really that >>>>> useful for anything. >>>> +1 >>>> >>>> And not just size() but many others from ConcurrentMap. >>>> The question is if we should drop the interface and all the methods >>>> which aren't efficiently implementable, or fix all those methods. >>>> >>>> In the past I loved that I could inject "Infinispan superpowers" into >>>> an application making extensive use of Map and ConcurrentMap without >>>> changes, but that has been deceiving and required great care such as >>>> verifying that these features would not be used anywhere in the code. >>>> I ended up wrapping the Cache implementation in a custom adapter which >>>> would also implement ConcurrentMap but would throw a RuntimeException >>>> if any of the "unallowed" methods was called, at least I would detect >>>> violations safely. >>>> >>>> I still think that for the time being - until a better solution is >>>> planned - we should throw exceptions.. alas that's an old conversation >>>> and it was never done. >>>> >>>> Sanne >>>> >>>> >>>>> -Dennis >>>>> >>>>> On 10/03/2014 03:30 AM, Radim Vansa wrote: >>>>>> Hi, >>>>>> >>>>>> recently we had a discussion about what size() returns, but I've >>>>>> realized there are more things that users would like to know. My >>>>>> question is whether you think that they would really appreciate it, or >>>>>> whether it's just my QA point of view where I sometimes compute the >>>>>> 'checksums' of cache to see if I didn't lost anything. >>>>>> >>>>>> There are those sizes: >>>>>> A) number of owned entries >>>>>> B) number of entries stored locally in memory >>>>>> C) number of entries stored in each local cache store >>>>>> D) number of entries stored in each shared cache store >>>>>> E) total number of entries in cache >>>>>> >>>>>> So far, we can get >>>>>> B via withFlags(SKIP_CACHE_LOAD).size() >>>>>> (passivation ? B : 0) + firstNonZero(C, D) via size() >>>>>> E via distributed iterators / MR >>>>>> A via data container iteration + distribution manager query, but only >>>>>> without cache store >>>>>> C or D through >>>>>> getComponentRegistry().getLocalComponent(PersistenceManager.class).getStores() >>>>>> >>>>>> I think that it would go along with users' expectations if size() >>>>>> returned E and for the rest we should have special methods on >>>>>> AdvancedCache. That would of course change the meaning of size(), but >>>>>> I'd say that finally to something that has firm meaning. >>>>>> >>>>>> WDYT? >>>>>> >>>>>> Radim >>>>>> >>>>> _______________________________________________ >>>>> 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 >>>> >>>> >>> _______________________________________________ >>> 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 > > -- > Radim Vansa <rvansa(a)redhat.com&gt; > JBoss DataGrid QA > > _______________________________________________ > 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

Considering all these very valid concerns I'd return on my proposal for throwing runtime exceptions via an (optional) decorator. I'd have such a decorator in place by default, so that we make it very clear that - while you can remove it - the behaviour of such methods is "unusual" and that a user would be better off avoiding them unless he's into the advanced stuff. As said before, that worked very well for me in the past and it was great that - even while I did know - I had a safety guard to highlight unintended refactorings by others on my team who didn't know the black art of using Infinispan correctly. Sanne On 7 October 2014 13:43, Radim Vansa <rvansa(a)redhat.com&gt; wrote: > On 10/07/2014 02:21 PM, William Burns wrote: >> On Tue, Oct 7, 2014 at 7:32 AM, Radim Vansa <rvansa(a)redhat.com&gt; wrote: >>> If you have one local and one shared cache store, how should the command >>> behave? >>> >>> a) distexec/MR sum of cache.withFlags(SKIP_REMOTE_LOOKUP, >>> SKIP_BACKUP_ENTRIES).size() from all nodes? (note that there's no >>> SKIP_BACKUP_ENTRIES flag right now), where this method returns >>> localStore.size() for first non-shared cache store + passivation ? >>> dataContainer.size(SKIP_BACKUP_ENTRIES) : 0) >> Calling the size method in either distexec or MR will give you >> inflated numbers as you need to pay attention to the numOwners to get >> a proper count. > That's what I meant by the SKIP_BACKUP_ENTRIES - dataContainer should be > able to report only primary-owned entries, or we have to iterate and > apply the filtering outside. > >>> b) distexec/MR sum of sharedStore.size() + passivation ? sum of >>> dataContainer.size(SKIP_BACKUP_ENTRIES) : 0 >> Calling the size on a shared cache actually should work somewhat well >> (assuming all entries are stored in the shared cache). The problem is >> if passivation is enabled as you point out because you also have to >> check the data container which means you can also have an issue with >> concurrent activations and passivations (which you can't verify >> properly in either case without knowing the keys). >> >>> c) MR that would count the entries >> This is the only reliable way to do this with MR. And unfortunately >> if a rehash occurs I am not sure if you would get inconsistent numbers >> or an Exception. In the latter at least you should be able to make >> sure that you have the proper number when it does return without >> exception. I can't say how it works with multiple loaders though, my >> guess is that it may process the entry more than once so it depends on >> if your mapper is smart enough to realize it. > I don't think that reporting incorrect size is *that* harmful - even > ConcurrentMap interface says that it's just a wild guess and when things > are changing, you can't rely on that. > >>> d) wrapper on distributed entry iteration with converters set to return >>> 0-sized entries >> Entry iterator can't return 0 sized entries (just the values). The >> keys are required to make sure that the count is correct and also to >> ensure that if a rehash happens in the middle it can properly continue >> to operate without having to start over. Entry iterator should work >> properly irrespective of the number of stores/loaders that are >> configured, since it keep track of already seen keys (so duplicates >> are ignored). > Ok, I was simplifying that a bit. And by the way, I don't really like > the fact that for distributed entry iteration you need to be able to > keep all keys from one segment at one moment in memory. But fine - > distributed entry iteration is probably not the right way. > >> >>> And what about nodes with different configuration? >> Hard to know without knowing what the differences are. > I had in my mind different loaders and passivation configuration (e.g. > some node could use shared store and some don't - do we want to handle > such obscure configs? Can we design that without the need to have > complicated decision trees what to include and what not?). > > Radim > >>> Radim >>> >>> On 10/06/2014 01:57 PM, Sanne Grinovero wrote: >>>> On 6 October 2014 12:44, Tristan Tarrant <ttarrant(a)redhat.com&gt; wrote: >>>>> I think we should provide correct implementations of size() (and others) >>>>> and provide shortcut implementations using our usual Flag API (e.g. >>>>> SKIP_REMOTE_LOOKUP). >>>> Right that would be very nice. Same for CacheStore interaction: all >>>> cachestores should be included unless skipped explicitly. >>>> >>>> Sanne >>>> >>>>> Tristan >>>>> >>>>> On 06/10/14 12:57, Sanne Grinovero wrote: >>>>>> On 3 October 2014 18:38, Dennis Reed <dereed(a)redhat.com&gt; wrote: >>>>>>> Since size() is defined by the ConcurrentMap interface, it already has a >>>>>>> precisely defined meaning. The only "correct" implementation is E. >>>>>> +1 >> This is one of the things I have been wanting to do is actually >> implement the other Map methods across the entire cache. However to >> do a lot of these in a memory conscious way they would need to be ran >> ignoring any ongoing transactions. Actually having this requirement >> allows these methods to be implemented quite easily especially in >> conjunction with the EntryIterator. I almost made a PR for it a while >> back, but it seemed a little zealous to do at the same time and it >> didn't seem that people were pushing for it very hard (maybe that was >> a wrong assumption). Also I wasn't quite sure the transactional part >> not being functional anymore would be a deterrent. >> >>>>>>> The current non-correct implementation was just because it's expensive >>>>>>> to calculate correctly. I'm not sure the current impl is really that >>>>>>> useful for anything. >>>>>> +1 >>>>>> >>>>>> And not just size() but many others from ConcurrentMap. >>>>>> The question is if we should drop the interface and all the methods >>>>>> which aren't efficiently implementable, or fix all those methods. >>>>>> >>>>>> In the past I loved that I could inject "Infinispan superpowers" into >>>>>> an application making extensive use of Map and ConcurrentMap without >>>>>> changes, but that has been deceiving and required great care such as >>>>>> verifying that these features would not be used anywhere in the code. >>>>>> I ended up wrapping the Cache implementation in a custom adapter which >>>>>> would also implement ConcurrentMap but would throw a RuntimeException >>>>>> if any of the "unallowed" methods was called, at least I would detect >>>>>> violations safely. >>>>>> >>>>>> I still think that for the time being - until a better solution is >>>>>> planned - we should throw exceptions.. alas that's an old conversation >>>>>> and it was never done. >>>>>> >>>>>> Sanne >>>>>> >>>>>> >>>>>>> -Dennis >>>>>>> >>>>>>> On 10/03/2014 03:30 AM, Radim Vansa wrote: >>>>>>>> Hi, >>>>>>>> >>>>>>>> recently we had a discussion about what size() returns, but I've >>>>>>>> realized there are more things that users would like to know. My >>>>>>>> question is whether you think that they would really appreciate it, or >>>>>>>> whether it's just my QA point of view where I sometimes compute the >>>>>>>> 'checksums' of cache to see if I didn't lost anything. >>>>>>>> >>>>>>>> There are those sizes: >>>>>>>> A) number of owned entries >>>>>>>> B) number of entries stored locally in memory >>>>>>>> C) number of entries stored in each local cache store >>>>>>>> D) number of entries stored in each shared cache store >>>>>>>> E) total number of entries in cache >>>>>>>> >>>>>>>> So far, we can get >>>>>>>> B via withFlags(SKIP_CACHE_LOAD).size() >>>>>>>> (passivation ? B : 0) + firstNonZero(C, D) via size() >>>>>>>> E via distributed iterators / MR >>>>>>>> A via data container iteration + distribution manager query, but only >>>>>>>> without cache store >>>>>>>> C or D through >>>>>>>> getComponentRegistry().getLocalComponent(PersistenceManager.class).getStores() >>>>>>>> >>>>>>>> I think that it would go along with users' expectations if size() >>>>>>>> returned E and for the rest we should have special methods on >>>>>>>> AdvancedCache. That would of course change the meaning of size(), but >>>>>>>> I'd say that finally to something that has firm meaning. >>>>>>>> >>>>>>>> WDYT? >>>>>>>> >>>>>>>> Radim >>>>>>>> >>>>>>> _______________________________________________ >>>>>>> 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 >>>>>> >>>>>> >>>>> _______________________________________________ >>>>> 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 >>> -- >>> Radim Vansa <rvansa(a)redhat.com&gt; >>> JBoss DataGrid QA >>> >>> _______________________________________________ >>> 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 > > -- > Radim Vansa <rvansa(a)redhat.com&gt; > JBoss DataGrid QA > > _______________________________________________ > 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

On 7 October 2014 14:42, Radim Vansa <rvansa(a)redhat.com&gt; wrote: > Considering the frequency of "How do I get the number of entries in > cache", "How do I get all keys" on all forums, I think that backing to > runtime exception would not satisfy the users. Correct but when they get to ask the right question, that's usually after several hours of debugging and swearing. With an immediate exception, they get immediate advice and hopefully a hint of where to look in the docs for the special methods like statistics, disabling the exception-throwing decorator, or implement their own M/R job with the exact flags they need. _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

On 10/07/2014 02:21 PM, William Burns wrote: > On Tue, Oct 7, 2014 at 7:32 AM, Radim Vansa <rvansa(a)redhat.com&gt; wrote: >> If you have one local and one shared cache store, how should the command >> behave? >> >> a) distexec/MR sum of cache.withFlags(SKIP_REMOTE_LOOKUP, >> SKIP_BACKUP_ENTRIES).size() from all nodes? (note that there's no >> SKIP_BACKUP_ENTRIES flag right now), where this method returns >> localStore.size() for first non-shared cache store + passivation ? >> dataContainer.size(SKIP_BACKUP_ENTRIES) : 0) > Calling the size method in either distexec or MR will give you > inflated numbers as you need to pay attention to the numOwners to get > a proper count. That's what I meant by the SKIP_BACKUP_ENTRIES - dataContainer should be able to report only primary-owned entries, or we have to iterate and apply the filtering outside.

> >> b) distexec/MR sum of sharedStore.size() + passivation ? sum of >> dataContainer.size(SKIP_BACKUP_ENTRIES) : 0 > Calling the size on a shared cache actually should work somewhat well > (assuming all entries are stored in the shared cache). The problem is > if passivation is enabled as you point out because you also have to > check the data container which means you can also have an issue with > concurrent activations and passivations (which you can't verify > properly in either case without knowing the keys). > >> c) MR that would count the entries > This is the only reliable way to do this with MR. And unfortunately > if a rehash occurs I am not sure if you would get inconsistent numbers > or an Exception. In the latter at least you should be able to make > sure that you have the proper number when it does return without > exception. I can't say how it works with multiple loaders though, my > guess is that it may process the entry more than once so it depends on > if your mapper is smart enough to realize it. I don't think that reporting incorrect size is *that* harmful - even ConcurrentMap interface says that it's just a wild guess and when things are changing, you can't rely on that.

> >> d) wrapper on distributed entry iteration with converters set to return >> 0-sized entries > Entry iterator can't return 0 sized entries (just the values). The > keys are required to make sure that the count is correct and also to > ensure that if a rehash happens in the middle it can properly continue > to operate without having to start over. Entry iterator should work > properly irrespective of the number of stores/loaders that are > configured, since it keep track of already seen keys (so duplicates > are ignored). Ok, I was simplifying that a bit. And by the way, I don't really like the fact that for distributed entry iteration you need to be able to keep all keys from one segment at one moment in memory. But fine - distributed entry iteration is probably not the right way.

> > >> And what about nodes with different configuration? > Hard to know without knowing what the differences are. I had in my mind different loaders and passivation configuration (e.g. some node could use shared store and some don't - do we want to handle such obscure configs? Can we design that without the need to have complicated decision trees what to include and what not?).

Radim > >> Radim >> >> On 10/06/2014 01:57 PM, Sanne Grinovero wrote: >>> On 6 October 2014 12:44, Tristan Tarrant <ttarrant(a)redhat.com&gt; wrote: >>>> I think we should provide correct implementations of size() (and others) >>>> and provide shortcut implementations using our usual Flag API (e.g. >>>> SKIP_REMOTE_LOOKUP). >>> Right that would be very nice. Same for CacheStore interaction: all >>> cachestores should be included unless skipped explicitly. >>> >>> Sanne >>> >>>> Tristan >>>> >>>> On 06/10/14 12:57, Sanne Grinovero wrote: >>>>> On 3 October 2014 18:38, Dennis Reed <dereed(a)redhat.com&gt; wrote: >>>>>> Since size() is defined by the ConcurrentMap interface, it already has a >>>>>> precisely defined meaning. The only "correct" implementation is E. >>>>> +1 > This is one of the things I have been wanting to do is actually > implement the other Map methods across the entire cache. However to > do a lot of these in a memory conscious way they would need to be ran > ignoring any ongoing transactions. Actually having this requirement > allows these methods to be implemented quite easily especially in > conjunction with the EntryIterator. I almost made a PR for it a while > back, but it seemed a little zealous to do at the same time and it > didn't seem that people were pushing for it very hard (maybe that was > a wrong assumption). Also I wasn't quite sure the transactional part > not being functional anymore would be a deterrent. > >>>>>> The current non-correct implementation was just because it's expensive >>>>>> to calculate correctly. I'm not sure the current impl is really that >>>>>> useful for anything. >>>>> +1 >>>>> >>>>> And not just size() but many others from ConcurrentMap. >>>>> The question is if we should drop the interface and all the methods >>>>> which aren't efficiently implementable, or fix all those methods. >>>>> >>>>> In the past I loved that I could inject "Infinispan superpowers" into >>>>> an application making extensive use of Map and ConcurrentMap without >>>>> changes, but that has been deceiving and required great care such as >>>>> verifying that these features would not be used anywhere in the code. >>>>> I ended up wrapping the Cache implementation in a custom adapter which >>>>> would also implement ConcurrentMap but would throw a RuntimeException >>>>> if any of the "unallowed" methods was called, at least I would detect >>>>> violations safely. >>>>> >>>>> I still think that for the time being - until a better solution is >>>>> planned - we should throw exceptions.. alas that's an old conversation >>>>> and it was never done. >>>>> >>>>> Sanne >>>>> >>>>> >>>>>> -Dennis >>>>>> >>>>>> On 10/03/2014 03:30 AM, Radim Vansa wrote: >>>>>>> Hi, >>>>>>> >>>>>>> recently we had a discussion about what size() returns, but I've >>>>>>> realized there are more things that users would like to know. My >>>>>>> question is whether you think that they would really appreciate it, or >>>>>>> whether it's just my QA point of view where I sometimes compute the >>>>>>> 'checksums' of cache to see if I didn't lost anything. >>>>>>> >>>>>>> There are those sizes: >>>>>>> A) number of owned entries >>>>>>> B) number of entries stored locally in memory >>>>>>> C) number of entries stored in each local cache store >>>>>>> D) number of entries stored in each shared cache store >>>>>>> E) total number of entries in cache >>>>>>> >>>>>>> So far, we can get >>>>>>> B via withFlags(SKIP_CACHE_LOAD).size() >>>>>>> (passivation ? B : 0) + firstNonZero(C, D) via size() >>>>>>> E via distributed iterators / MR >>>>>>> A via data container iteration + distribution manager query, but only >>>>>>> without cache store >>>>>>> C or D through >>>>>>> getComponentRegistry().getLocalComponent(PersistenceManager.class).getStores() >>>>>>> >>>>>>> I think that it would go along with users' expectations if size() >>>>>>> returned E and for the rest we should have special methods on >>>>>>> AdvancedCache. That would of course change the meaning of size(), but >>>>>>> I'd say that finally to something that has firm meaning. >>>>>>> >>>>>>> WDYT? >>>>>>> >>>>>>> Radim >>>>>>> >>>>>> _______________________________________________ >>>>>> 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 >>>>> >>>>> >>>> _______________________________________________ >>>> 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 >> >> -- >> Radim Vansa <rvansa(a)redhat.com&gt; >> JBoss DataGrid QA >> >> _______________________________________________ >> 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 -- Radim Vansa <rvansa(a)redhat.com&gt; JBoss DataGrid QA _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

I think we should provide correct implementations of size() (and others) and provide shortcut implementations using our usual Flag API (e.g. SKIP_REMOTE_LOOKUP).

Tristan On 06/10/14 12:57, Sanne Grinovero wrote: > On 3 October 2014 18:38, Dennis Reed <dereed(a)redhat.com&gt; wrote: >> Since size() is defined by the ConcurrentMap interface, it already has a >> precisely defined meaning. The only "correct" implementation is E. > +1 > >> The current non-correct implementation was just because it's expensive >> to calculate correctly. I'm not sure the current impl is really that >> useful for anything. > +1 > > And not just size() but many others from ConcurrentMap. > The question is if we should drop the interface and all the methods > which aren't efficiently implementable, or fix all those methods. > > In the past I loved that I could inject "Infinispan superpowers" into > an application making extensive use of Map and ConcurrentMap without > changes, but that has been deceiving and required great care such as > verifying that these features would not be used anywhere in the code. > I ended up wrapping the Cache implementation in a custom adapter which > would also implement ConcurrentMap but would throw a RuntimeException > if any of the "unallowed" methods was called, at least I would detect > violations safely. > > I still think that for the time being - until a better solution is > planned - we should throw exceptions.. alas that's an old conversation > and it was never done. > > Sanne > > >> -Dennis >> >> On 10/03/2014 03:30 AM, Radim Vansa wrote: >>> Hi, >>> >>> recently we had a discussion about what size() returns, but I've >>> realized there are more things that users would like to know. My >>> question is whether you think that they would really appreciate it, or >>> whether it's just my QA point of view where I sometimes compute the >>> 'checksums' of cache to see if I didn't lost anything. >>> >>> There are those sizes: >>> A) number of owned entries >>> B) number of entries stored locally in memory >>> C) number of entries stored in each local cache store >>> D) number of entries stored in each shared cache store >>> E) total number of entries in cache >>> >>> So far, we can get >>> B via withFlags(SKIP_CACHE_LOAD).size() >>> (passivation ? B : 0) + firstNonZero(C, D) via size() >>> E via distributed iterators / MR >>> A via data container iteration + distribution manager query, but only >>> without cache store >>> C or D through >>> getComponentRegistry().getLocalComponent(PersistenceManager.class).getStores() >>> >>> I think that it would go along with users' expectations if size() >>> returned E and for the rest we should have special methods on >>> AdvancedCache. That would of course change the meaning of size(), but >>> I'd say that finally to something that has firm meaning. >>> >>> WDYT? >>> >>> Radim >>> >> _______________________________________________ >> 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 > > _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

Hi, recently we had a discussion about what size() returns, but I've realized there are more things that users would like to know. My question is whether you think that they would really appreciate it, or whether it's just my QA point of view where I sometimes compute the 'checksums' of cache to see if I didn't lost anything. There are those sizes: A) number of owned entries B) number of entries stored locally in memory C) number of entries stored in each local cache store D) number of entries stored in each shared cache store E) total number of entries in cache So far, we can get B via withFlags(SKIP_CACHE_LOAD).size() (passivation ? B : 0) + firstNonZero(C, D) via size() E via distributed iterators / MR A via data container iteration + distribution manager query, but only without cache store C or D through getComponentRegistry().getLocalComponent(PersistenceManager.class).getStores() I think that it would go along with users' expectations if size() returned E and for the rest we should have special methods on AdvancedCache. That would of course change the meaning of size(), but I'd say that finally to something that has firm meaning. WDYT?

Radim -- Radim Vansa <rvansa(a)redhat.com&gt; JBoss DataGrid QA _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

On Wed, Oct 8, 2014 at 5:03 PM, Mircea Markus <mmarkus(a)redhat.com&gt; wrote: On Oct 3, 2014, at 9:30, Radim Vansa <rvansa(a)redhat.com&gt; wrote: > Hi, > > recently we had a discussion about what size() returns, but I've > realized there are more things that users would like to know. My > question is whether you think that they would really appreciate it, or > whether it's just my QA point of view where I sometimes compute the > 'checksums' of cache to see if I didn't lost anything. > > There are those sizes: > A) number of owned entries > B) number of entries stored locally in memory > C) number of entries stored in each local cache store > D) number of entries stored in each shared cache store > E) total number of entries in cache > > So far, we can get > B via withFlags(SKIP_CACHE_LOAD).size() > (passivation ? B : 0) + firstNonZero(C, D) via size() > E via distributed iterators / MR > A via data container iteration + distribution manager query, but only > without cache store > C or D through > getComponentRegistry().getLocalComponent(PersistenceManager.class).getStores() > > I think that it would go along with users' expectations if size() > returned E and for the rest we should have special methods on > AdvancedCache. That would of course change the meaning of size(), but > I'd say that finally to something that has firm meaning. > > WDYT? There was a lot of arguments in past whether size() and other methods that operate over all the elements (keySet, values) are useful because: - they are approximate (data changes during iteration) - they are very resource consuming and might be miss-used (this is the reason we chosen to use size() with its current local semantic) These methods (size, keys, values) are useful for people and I think we were not wise to implement them only on top of the local data: this is like preferring efficiency over correctness. This also created a lot of confusion with our users, question like size() doesn't return the correct value being asked regularly. I totally agree that size() returns E (i.e. everything that is stored within the grid, including persistence) and it's performance implications to be documented accordingly. For keySet and values - we should stop implementing them (throw exception) and point users to Will's distributed iterator which is a nicer way to achieve the desired behavior. We can also implement keySet() and values() on top of the distributed entry iterator and document that using the iterator directly is better.

So it seems we would want to change this for 7.0 if possible since it would be a bigger change for something like 7.1 and 8.0 would be even further out. I should be able to put this together for CR2.

It seems that we want to implement keySet, values and entrySet methods using the entry iterator approach. It is however unclear for the size method if we want to use MR entry counting and not worry about the rehash and passivation issues since it is just an estimation anyways. Or if we want to also use the entry iterator which should be closer approximation but will require more network overhead and memory usage.

Also we didn't really talk about the fact that these methods would ignore ongoing transactions and if that is a concern or not.

- Will On Wed, Oct 8, 2014 at 10:13 AM, Mircea Markus <mmarkus(a)redhat.com&gt; wrote: > > On Oct 8, 2014, at 15:11, Dan Berindei <dan.berindei(a)gmail.com&gt; wrote: > >> >> On Wed, Oct 8, 2014 at 5:03 PM, Mircea Markus <mmarkus(a)redhat.com&gt; wrote: >> On Oct 3, 2014, at 9:30, Radim Vansa <rvansa(a)redhat.com&gt; wrote: >> >> > Hi, >> > >> > recently we had a discussion about what size() returns, but I've >> > realized there are more things that users would like to know. My >> > question is whether you think that they would really appreciate it, or >> > whether it's just my QA point of view where I sometimes compute the >> > 'checksums' of cache to see if I didn't lost anything. >> > >> > There are those sizes: >> > A) number of owned entries >> > B) number of entries stored locally in memory >> > C) number of entries stored in each local cache store >> > D) number of entries stored in each shared cache store >> > E) total number of entries in cache >> > >> > So far, we can get >> > B via withFlags(SKIP_CACHE_LOAD).size() >> > (passivation ? B : 0) + firstNonZero(C, D) via size() >> > E via distributed iterators / MR >> > A via data container iteration + distribution manager query, but only >> > without cache store >> > C or D through >> > getComponentRegistry().getLocalComponent(PersistenceManager.class).getStores() >> > >> > I think that it would go along with users' expectations if size() >> > returned E and for the rest we should have special methods on >> > AdvancedCache. That would of course change the meaning of size(), but >> > I'd say that finally to something that has firm meaning. >> > >> > WDYT? >> >> There was a lot of arguments in past whether size() and other methods that operate over all the elements (keySet, values) are useful because: >> - they are approximate (data changes during iteration) >> - they are very resource consuming and might be miss-used (this is the reason we chosen to use size() with its current local semantic) >> >> These methods (size, keys, values) are useful for people and I think we were not wise to implement them only on top of the local data: this is like preferring efficiency over correctness. This also created a lot of confusion with our users, question like size() doesn't return the correct value being asked regularly. I totally agree that size() returns E (i.e. everything that is stored within the grid, including persistence) and it's performance implications to be documented accordingly. For keySet and values - we should stop implementing them (throw exception) and point users to Will's distributed iterator which is a nicer way to achieve the desired behavior. >> >> We can also implement keySet() and values() on top of the distributed entry iterator and document that using the iterator directly is better. > > Yes, that's what I meant as well. > > Cheers, > -- > Mircea Markus > Infinispan lead (www.infinispan.org) > > > > > > _______________________________________________ > 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

On Wed, Oct 8, 2014 at 10:57 AM, Dan Berindei <dan.berindei(a)gmail.com&gt; wrote: > > > On Wed, Oct 8, 2014 at 5:42 PM, William Burns <mudokonman(a)gmail.com&gt; wrote: >> >> So it seems we would want to change this for 7.0 if possible since it >> would be a bigger change for something like 7.1 and 8.0 would be even >> further out. I should be able to put this together for CR2. > > > I'm not 100% convinced that we need it for 7.x. For 8.0 I would recommend > removing the size() method altogether, and providing some looser > "statistics" instead. Yeah I guess I don't know enough about the demand for these methods or what people wanted to use them for to know what kind of priority they should be given. It sounds like you are talking about decoupling from the Map/ConcurrentMap interface completely then, right? So we would also eliminate the other bulk methods (keySet, values, entrySet)?

> >> >> >> It seems that we want to implement keySet, values and entrySet methods >> using the entry iterator approach. >> >> It is however unclear for the size method if we want to use MR entry >> counting and not worry about the rehash and passivation issues since >> it is just an estimation anyways. Or if we want to also use the entry >> iterator which should be closer approximation but will require more >> network overhead and memory usage. > > > +1 to use the entry iterator from me, ignoring state transfer we can get > some pretty wild fluctuations in the size of the cache. That is personally my feeling as well, but I tend to err more on the side of correctness to begin with. > We could use a distributed task for Cache.isEmpty() instead of size() == 0, > though. Yes that should be a good optimization either way. > >> >> >> Also we didn't really talk about the fact that these methods would >> ignore ongoing transactions and if that is a concern or not. >> > > It might be a concern for the Hibernate 2LC impl, it was their TCK that > prompted the last round of discussions about clear(). Although I wonder how much these methods are even used since they only work for Local, Replication or Invalidation caches in their current state (and didn't even use loaders until 6.0).

> > We haven't talked about what size(), keySet() and values() should return for > an invalidation cache either... I forget, does the distributed entry > iterator work with invalidation caches? It works the same as a local cache so only the local node contents are returned. Replicated does the same thing, distributed is the only special case. This was the only thing that made sense to me, but if you have any ideas that would be great to hear for possibly enhancing Invalidation iteration.

> > >> >> - Will >> >> On Wed, Oct 8, 2014 at 10:13 AM, Mircea Markus <mmarkus(a)redhat.com&gt; wrote: >> > >> > On Oct 8, 2014, at 15:11, Dan Berindei <dan.berindei(a)gmail.com&gt; wrote: >> > >> >> >> >> On Wed, Oct 8, 2014 at 5:03 PM, Mircea Markus <mmarkus(a)redhat.com&gt; >> >> wrote: >> >> On Oct 3, 2014, at 9:30, Radim Vansa <rvansa(a)redhat.com&gt; wrote: >> >> >> >> > Hi, >> >> > >> >> > recently we had a discussion about what size() returns, but I've >> >> > realized there are more things that users would like to know. My >> >> > question is whether you think that they would really appreciate it, >> >> > or >> >> > whether it's just my QA point of view where I sometimes compute the >> >> > 'checksums' of cache to see if I didn't lost anything. >> >> > >> >> > There are those sizes: >> >> > A) number of owned entries >> >> > B) number of entries stored locally in memory >> >> > C) number of entries stored in each local cache store >> >> > D) number of entries stored in each shared cache store >> >> > E) total number of entries in cache >> >> > >> >> > So far, we can get >> >> > B via withFlags(SKIP_CACHE_LOAD).size() >> >> > (passivation ? B : 0) + firstNonZero(C, D) via size() >> >> > E via distributed iterators / MR >> >> > A via data container iteration + distribution manager query, but only >> >> > without cache store >> >> > C or D through >> >> > >> >> > getComponentRegistry().getLocalComponent(PersistenceManager.class).getStores() >> >> > >> >> > I think that it would go along with users' expectations if size() >> >> > returned E and for the rest we should have special methods on >> >> > AdvancedCache. That would of course change the meaning of size(), but >> >> > I'd say that finally to something that has firm meaning. >> >> > >> >> > WDYT? >> >> >> >> There was a lot of arguments in past whether size() and other methods >> >> that operate over all the elements (keySet, values) are useful because: >> >> - they are approximate (data changes during iteration) >> >> - they are very resource consuming and might be miss-used (this is the >> >> reason we chosen to use size() with its current local semantic) >> >> >> >> These methods (size, keys, values) are useful for people and I think we >> >> were not wise to implement them only on top of the local data: this is like >> >> preferring efficiency over correctness. This also created a lot of confusion >> >> with our users, question like size() doesn't return the correct value being >> >> asked regularly. I totally agree that size() returns E (i.e. everything that >> >> is stored within the grid, including persistence) and it's performance >> >> implications to be documented accordingly. For keySet and values - we should >> >> stop implementing them (throw exception) and point users to Will's >> >> distributed iterator which is a nicer way to achieve the desired behavior. >> >> >> >> We can also implement keySet() and values() on top of the distributed >> >> entry iterator and document that using the iterator directly is better. >> > >> > Yes, that's what I meant as well. >> > >> > Cheers, >> > -- >> > Mircea Markus >> > Infinispan lead (www.infinispan.org) >> > >> > >> > >> > >> > >> > _______________________________________________ >> > 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 > > > > _______________________________________________ > 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

>>>> Also we didn't really talk about the fact that these methods would >>>> ignore ongoing transactions and if that is a concern or not. >>>> >>> >>> It might be a concern for the Hibernate 2LC impl, it was their TCK that >>> prompted the last round of discussions about clear(). >> >> Although I wonder how much these methods are even used since they only >> work for Local, Replication or Invalidation caches in their current >> state (and didn't even use loaders until 6.0). > > > There is some more information about the test in the mailing list discussion > [1] > There's also a JIRA for clear() [2] > > I think 2LC almost never uses distribution, so size() being local-only > didn't matter, but making it non-tx could cause problems - at least for that > particular test. I had toyed around with the following idea before, but I never thought of it in the scope of the size method solely, but I have a solution that would work mostly for transactional caches. Essentially the size method would always operate in a READ_COMMITTED like state, using REPEATABLE_READ doesn't seem feasible since we can't keep all the contents in memory. Essentially the iterator would be ran and for each key that is found it checks the context to see if it is there. If the context entry is marked as removed it doesn't count the key, if the key is there it marks the key as found and counts it, and if it is not found it counts it. Then after iteration it finds all the keys in the context that were not found and also adds them to the count. This way it doesn't need to store additional memory (besides iteration costs) as all the context information is in memory.

My original thought was to also make the EntryIterator transactional in the same way which also means the keySet, entrySet and values methods could do the same things. The main reason stumbling block I had was the fact that the iterator and various collections returned could be used outside of the ongoing transaction which didn't seem to make much sense to me. But maybe these should be changed to be more like backing maps which HashMap, ConcurrentHashMap etc use for their methods, where instead it would pick up the transaction if there is one in the current thread and if there is no transaction just start an implicit one.

This however was a big change from how these collections work currently in that they are in memory copies only. What do you guys think?

On Fri, Oct 10, 2014 at 9:01 PM, Mircea Markus <mmarkus(a)redhat.com&gt; wrote: On Oct 10, 2014, at 17:30, William Burns <mudokonman(a)gmail.com&gt; wrote: >>>>> Also we didn't really talk about the fact that these methods would >>>>> ignore ongoing transactions and if that is a concern or not. >>>>> >>>> >>>> It might be a concern for the Hibernate 2LC impl, it was their TCK that >>>> prompted the last round of discussions about clear(). >>> >>> Although I wonder how much these methods are even used since they only >>> work for Local, Replication or Invalidation caches in their current >>> state (and didn't even use loaders until 6.0). >> >> >> There is some more information about the test in the mailing list discussion >> [1] >> There's also a JIRA for clear() [2] >> >> I think 2LC almost never uses distribution, so size() being local-only >> didn't matter, but making it non-tx could cause problems - at least for that >> particular test. > > I had toyed around with the following idea before, but I never thought > of it in the scope of the size method solely, but I have a solution > that would work mostly for transactional caches. Essentially the size > method would always operate in a READ_COMMITTED like state, using > REPEATABLE_READ doesn't seem feasible since we can't keep all the > contents in memory. Essentially the iterator would be ran and for > each key that is found it checks the context to see if it is there. > If the context entry is marked as removed it doesn't count the key, if > the key is there it marks the key as found and counts it, and if it is > not found it counts it. Then after iteration it finds all the keys in > the context that were not found and also adds them to the count. This > way it doesn't need to store additional memory (besides iteration > costs) as all the context information is in memory. sounds good to me. Mircea, you have to decide whether you want the precise estimation using the entry iterator or the loose estimation using dataContainer.size() :) I guess we can't make size() read everything into the invocation context, so READ_COMMITTED is all we can provide if we want to keep size() transactional. Maybe we don't really need it though... Will, could you investigate the failing test that started the clear() thread [1] to see if it really needs size() to be transactional?

> > My original thought was to also make the EntryIterator transactional > in the same way which also means the keySet, entrySet and values > methods could do the same things. The main reason stumbling block I > had was the fact that the iterator and various collections returned > could be used outside of the ongoing transaction which didn't seem to > make much sense to me. But maybe these should be changed to be more > like backing maps which HashMap, ConcurrentHashMap etc use for their > methods, where instead it would pick up the transaction if there is > one in the current thread and if there is no transaction just start an > implicit one. or if they are outside of a transaction to deny progress I don't think it's fair to require an explicit transaction for every entrySet(). It should be possible to start an iteration without a transaction, and only to invalidate an iteration started from an explicit transaction the moment the transaction is committed/rolled back (although it would complicate rules a bit). And what happens if the user writes to the cache while it's iterating through the cache-backed collection? Should the user see the new entry in the iteration, or not? I don't think you can figure out at the end of the iteration which keys were included without keeping all the keys on the originator.

> This however was a big change from how these > collections work currently in that they are in memory copies only. > > What do you guys think? I think that keeping track of the context entries is a better way of iterating so +1. As you mentioned, we should also make it clear that RC semantic applies. Cheers, -- Mircea Markus Infinispan lead (www.infinispan.org) _______________________________________________ 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

On 10/13/2014 05:55 PM, Mircea Markus wrote: > On Oct 13, 2014, at 14:06, Dan Berindei <dan.berindei(a)gmail.com&gt; wrote: > >> >> On Fri, Oct 10, 2014 at 9:01 PM, Mircea Markus <mmarkus(a)redhat.com&gt; wrote: >> >> On Oct 10, 2014, at 17:30, William Burns <mudokonman(a)gmail.com&gt; wrote: >> >>>>>>> Also we didn't really talk about the fact that these methods would >>>>>>> ignore ongoing transactions and if that is a concern or not. >>>>>>> >>>>>> It might be a concern for the Hibernate 2LC impl, it was their TCK that >>>>>> prompted the last round of discussions about clear(). >>>>> Although I wonder how much these methods are even used since they only >>>>> work for Local, Replication or Invalidation caches in their current >>>>> state (and didn't even use loaders until 6.0). >>>> >>>> There is some more information about the test in the mailing list discussion >>>> [1] >>>> There's also a JIRA for clear() [2] >>>> >>>> I think 2LC almost never uses distribution, so size() being local-only >>>> didn't matter, but making it non-tx could cause problems - at least for that >>>> particular test. >>> I had toyed around with the following idea before, but I never thought >>> of it in the scope of the size method solely, but I have a solution >>> that would work mostly for transactional caches. Essentially the size >>> method would always operate in a READ_COMMITTED like state, using >>> REPEATABLE_READ doesn't seem feasible since we can't keep all the >>> contents in memory. Essentially the iterator would be ran and for >>> each key that is found it checks the context to see if it is there. >>> If the context entry is marked as removed it doesn't count the key, if >>> the key is there it marks the key as found and counts it, and if it is >>> not found it counts it. Then after iteration it finds all the keys in >>> the context that were not found and also adds them to the count. This >>> way it doesn't need to store additional memory (besides iteration >>> costs) as all the context information is in memory. >> sounds good to me. >> >> Mircea, you have to decide whether you want the precise estimation using the entry iterator or the loose estimation using dataContainer.size() :) >> >> I guess we can't make size() read everything into the invocation context, so READ_COMMITTED is all we can provide if we want to keep size() transactional. Maybe we don't really need it though... Will, could you investigate the failing test that started the clear() thread [1] to see if it really needs size() to be transactional? > I'm okay with both approaches TBH, both are much better than what we currently have. The accurate one is more costly but seems to be the solution of choice so let's go for it. > >> >>> My original thought was to also make the EntryIterator transactional >>> in the same way which also means the keySet, entrySet and values >>> methods could do the same things. The main reason stumbling block I >>> had was the fact that the iterator and various collections returned >>> could be used outside of the ongoing transaction which didn't seem to >>> make much sense to me. But maybe these should be changed to be more >>> like backing maps which HashMap, ConcurrentHashMap etc use for their >>> methods, where instead it would pick up the transaction if there is >>> one in the current thread and if there is no transaction just start an >>> implicit one. >> or if they are outside of a transaction to deny progress >> >> I don't think it's fair to require an explicit transaction for every entrySet(). It should be possible to start an iteration without a transaction, and only to invalidate an iteration started from an explicit transaction the moment the transaction is committed/rolled back (although it would complicate rules a bit). >> >> And what happens if the user writes to the cache while it's iterating through the cache-backed collection? Should the user see the new entry in the iteration, or not? I don't think you can figure out at the end of the iteration which keys were included without keeping all the keys on the originator. > If the modification is done outside the iterator one might expect an ConcurrentModificationException, as it is the case with some JDK iterators. -1 We're aiming at high performance cache with a lot of changes while the operation is executed. This way, the iteration would never complete, unless you explicitly switch the cache to read only mode (either through Infinispan operation or in application).

I think that adding isCacheModified() or isTopologyChanged() to the iterator would make sense, if that's not too complicated to implement. Though, if we want non-disturbed iteration, snapshot isolation is the only answer.

On Tue, Oct 14, 2014 at 3:33 AM, Dan Berindei <dan.berindei(a)gmail.com&gt; wrote: > > > On Tue, Oct 14, 2014 at 9:55 AM, Radim Vansa <rvansa(a)redhat.com&gt; wrote: >> >> On 10/13/2014 05:55 PM, Mircea Markus wrote: >> > On Oct 13, 2014, at 14:06, Dan Berindei <dan.berindei(a)gmail.com&gt; wrote: >> > >> >> >> >> On Fri, Oct 10, 2014 at 9:01 PM, Mircea Markus <mmarkus(a)redhat.com&gt; >> >> wrote: >> >> >> >> On Oct 10, 2014, at 17:30, William Burns <mudokonman(a)gmail.com&gt; wrote: >> >> >> >>>>>>> Also we didn't really talk about the fact that these methods would >> >>>>>>> ignore ongoing transactions and if that is a concern or not. >> >>>>>>> >> >>>>>> It might be a concern for the Hibernate 2LC impl, it was their TCK >> >>>>>> that >> >>>>>> prompted the last round of discussions about clear(). >> >>>>> Although I wonder how much these methods are even used since they >> >>>>> only >> >>>>> work for Local, Replication or Invalidation caches in their current >> >>>>> state (and didn't even use loaders until 6.0). >> >>>> >> >>>> There is some more information about the test in the mailing list >> >>>> discussion >> >>>> [1] >> >>>> There's also a JIRA for clear() [2] >> >>>> >> >>>> I think 2LC almost never uses distribution, so size() being >> >>>> local-only >> >>>> didn't matter, but making it non-tx could cause problems - at least >> >>>> for that >> >>>> particular test. >> >>> I had toyed around with the following idea before, but I never thought >> >>> of it in the scope of the size method solely, but I have a solution >> >>> that would work mostly for transactional caches. Essentially the size >> >>> method would always operate in a READ_COMMITTED like state, using >> >>> REPEATABLE_READ doesn't seem feasible since we can't keep all the >> >>> contents in memory. Essentially the iterator would be ran and for >> >>> each key that is found it checks the context to see if it is there. >> >>> If the context entry is marked as removed it doesn't count the key, if >> >>> the key is there it marks the key as found and counts it, and if it is >> >>> not found it counts it. Then after iteration it finds all the keys in >> >>> the context that were not found and also adds them to the count. This >> >>> way it doesn't need to store additional memory (besides iteration >> >>> costs) as all the context information is in memory. >> >> sounds good to me. >> >> >> >> Mircea, you have to decide whether you want the precise estimation >> >> using the entry iterator or the loose estimation using dataContainer.size() >> >> :) >> >> >> >> I guess we can't make size() read everything into the invocation >> >> context, so READ_COMMITTED is all we can provide if we want to keep size() >> >> transactional. Maybe we don't really need it though... Will, could you >> >> investigate the failing test that started the clear() thread [1] to see if >> >> it really needs size() to be transactional? >> > I'm okay with both approaches TBH, both are much better than what we >> > currently have. The accurate one is more costly but seems to be the solution >> > of choice so let's go for it. >> > >> >> >> >>> My original thought was to also make the EntryIterator transactional >> >>> in the same way which also means the keySet, entrySet and values >> >>> methods could do the same things. The main reason stumbling block I >> >>> had was the fact that the iterator and various collections returned >> >>> could be used outside of the ongoing transaction which didn't seem to >> >>> make much sense to me. But maybe these should be changed to be more >> >>> like backing maps which HashMap, ConcurrentHashMap etc use for their >> >>> methods, where instead it would pick up the transaction if there is >> >>> one in the current thread and if there is no transaction just start an >> >>> implicit one. >> >> or if they are outside of a transaction to deny progress >> >> >> >> I don't think it's fair to require an explicit transaction for every >> >> entrySet(). It should be possible to start an iteration without a >> >> transaction, and only to invalidate an iteration started from an explicit >> >> transaction the moment the transaction is committed/rolled back (although it >> >> would complicate rules a bit). >> >> >> >> And what happens if the user writes to the cache while it's iterating >> >> through the cache-backed collection? Should the user see the new entry in >> >> the iteration, or not? I don't think you can figure out at the end of the >> >> iteration which keys were included without keeping all the keys on the >> >> originator. >> > If the modification is done outside the iterator one might expect an >> > ConcurrentModificationException, as it is the case with some JDK iterators. >> >> -1 We're aiming at high performance cache with a lot of changes while >> the operation is executed. This way, the iteration would never complete, >> unless you explicitly switch the cache to read only mode (either through >> Infinispan operation or in application). > > > I was referring only to changes made in the same transaction, not changes > made by other transactions. But you make a good point, we can't throw a > ConcurrentModificationException if the user for writes in the same > transaction and ignore other transactions. > >> >> >> I think that adding isCacheModified() or isTopologyChanged() to the >> iterator would make sense, if that's not too complicated to implement. >> Though, if we want non-disturbed iteration, snapshot isolation is the >> only answer. > > > isCacheModified() is probably too costly to implement. > isTopologyChanged() could be done, but I'm not sure what's the use case, as > the entry iterator abstracts topology changes from the user. > > I don't think we want undisturbed iteration, at least not at this point. > Personally, I just want to have a good story on why the iteration behaves in > a certain way. By my standards, explaining that changes made by other > transactions may completely/partially/not at all be visible in the iteration > is fine, explaining that changes made by the same transaction may or may not > be visible is not. Sorry I didn't respond earlier. But these commands would check the transaction context before returning the value to the user. This requires a user interaction for this to occur, so we can guarantee they will always see their updated value if they have one in the transaction (even if one is ran in between the iteration). The big thing is whether or not another transaction's update is seen when we don't have an update for that key (that will be occur if the segment is completed before the update or not). There should be no need to tell if the cache modified or the topology changed (the former would require a very high impact for performance to tell with a DIST cache). To be honest the wrapper classes would just be delegating to the Cache for the vast majority of operations (get, remove, contains etc.). It would only be when someone specifically uses the iterator on the various collections that the distributed iterator would even be used. This way the various collections would be backing maps, like HashMap and ConcurrentHashMap have, just they have to check the transaction as well. The values collection would be extremely limited in its supported methods though, pretty much only to iteration and size. > > > Cheers > Dan > > > _______________________________________________ > infinispan-dev mailing list > infinispan-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/infinispan-dev

So it seems we would want to change this for 7.0 if possible since it would be a bigger change for something like 7.1 and 8.0 would be even further out. I should be able to put this together for CR2. It seems that we want to implement keySet, values and entrySet methods using the entry iterator approach. It is however unclear for the size method if we want to use MR entry counting and not worry about the rehash and passivation issues since it is just an estimation anyways. Or if we want to also use the entry iterator which should be closer approximation but will require more network overhead and memory usage. Also we didn't really talk about the fact that these methods would ignore ongoing transactions and if that is a concern or not. - Will On Wed, Oct 8, 2014 at 10:13 AM, Mircea Markus <mmarkus(a)redhat.com&gt; wrote: > On Oct 8, 2014, at 15:11, Dan Berindei <dan.berindei(a)gmail.com&gt; wrote: > >> On Wed, Oct 8, 2014 at 5:03 PM, Mircea Markus <mmarkus(a)redhat.com&gt; wrote: >> On Oct 3, 2014, at 9:30, Radim Vansa <rvansa(a)redhat.com&gt; wrote: >> >>> Hi, >>> >>> recently we had a discussion about what size() returns, but I've >>> realized there are more things that users would like to know. My >>> question is whether you think that they would really appreciate it, or >>> whether it's just my QA point of view where I sometimes compute the >>> 'checksums' of cache to see if I didn't lost anything. >>> >>> There are those sizes: >>> A) number of owned entries >>> B) number of entries stored locally in memory >>> C) number of entries stored in each local cache store >>> D) number of entries stored in each shared cache store >>> E) total number of entries in cache >>> >>> So far, we can get >>> B via withFlags(SKIP_CACHE_LOAD).size() >>> (passivation ? B : 0) + firstNonZero(C, D) via size() >>> E via distributed iterators / MR >>> A via data container iteration + distribution manager query, but only >>> without cache store >>> C or D through >>> getComponentRegistry().getLocalComponent(PersistenceManager.class).getStores() >>> >>> I think that it would go along with users' expectations if size() >>> returned E and for the rest we should have special methods on >>> AdvancedCache. That would of course change the meaning of size(), but >>> I'd say that finally to something that has firm meaning. >>> >>> WDYT? >> There was a lot of arguments in past whether size() and other methods that operate over all the elements (keySet, values) are useful because: >> - they are approximate (data changes during iteration) >> - they are very resource consuming and might be miss-used (this is the reason we chosen to use size() with its current local semantic) >> >> These methods (size, keys, values) are useful for people and I think we were not wise to implement them only on top of the local data: this is like preferring efficiency over correctness. This also created a lot of confusion with our users, question like size() doesn't return the correct value being asked regularly. I totally agree that size() returns E (i.e. everything that is stored within the grid, including persistence) and it's performance implications to be documented accordingly. For keySet and values - we should stop implementing them (throw exception) and point users to Will's distributed iterator which is a nicer way to achieve the desired behavior. >> >> We can also implement keySet() and values() on top of the distributed entry iterator and document that using the iterator directly is better. > Yes, that's what I meant as well. > > Cheers, > -- > Mircea Markus > Infinispan lead (www.infinispan.org) > > > > > > _______________________________________________ > 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

On Wed, Oct 8, 2014 at 6:19 PM, Radim Vansa <rvansa(a)redhat.com&gt; wrote: > > Users expect that size() will be constant-time (or linear to cluster > size), and generally fast operation. I'd prefer to keep it that way. > Though, even the MR way (used for HotRod size() now) needs to crawl > through all the entries locally. They might expect that, but there is nothing in the Map API suggesting it. > > > 'Heretic, not very well though of and changing too many things' idea: > what about having data container segment-aware? Then you'd just bcast > SizeCommand with given topologyId and sum up sizes of primary-owned > segments... It's not a complete solution, but at least that would enable > to get the number of locally owned entries quite fast. Though, you can't > do that easily with cache stores (without changing SPI). We could create a separate DataContainer for each segment. But would it really be worth the trouble? I don't know of anyone using size() for something other than checking that their data was properly loaded into the cache, and they don't need a super-fast size() for that.

> > > Regarding cache stores, IMO we're damned anyway: when calling > cacheStore.size(), it can report more entries as those haven't been > expired yet, it can report less entries as those can be expired due to > [1]. Or, we'll enumerate all the entries, and that's going to be slow > (btw., [1] reminded me that we should enumerate both datacontainer AND > cachestores even if passivation is not enabled). Exactly, we need to iterate all the entries from the stores if we want something remotely accurate (although I had forgotten about expiration also being a problem). Otherwise we could just leave size() as it is now, it's pretty fast :) > > > Radim > > [1] https://issues.jboss.org/browse/ISPN-3202 > > On 10/08/2014 04:42 PM, William Burns wrote: > > So it seems we would want to change this for 7.0 if possible since it > > would be a bigger change for something like 7.1 and 8.0 would be even > > further out. I should be able to put this together for CR2. > > > > It seems that we want to implement keySet, values and entrySet methods > > using the entry iterator approach. > > > > It is however unclear for the size method if we want to use MR entry > > counting and not worry about the rehash and passivation issues since > > it is just an estimation anyways. Or if we want to also use the entry > > iterator which should be closer approximation but will require more > > network overhead and memory usage. > > > > Also we didn't really talk about the fact that these methods would > > ignore ongoing transactions and if that is a concern or not. > > > > - Will > > > > On Wed, Oct 8, 2014 at 10:13 AM, Mircea Markus <mmarkus(a)redhat.com&gt; > > wrote: > >> On Oct 8, 2014, at 15:11, Dan Berindei <dan.berindei(a)gmail.com&gt; wrote: > >> > >>> On Wed, Oct 8, 2014 at 5:03 PM, Mircea Markus <mmarkus(a)redhat.com&gt; > >>> wrote: > >>> On Oct 3, 2014, at 9:30, Radim Vansa <rvansa(a)redhat.com&gt; wrote: > >>> > >>>> Hi, > >>>> > >>>> recently we had a discussion about what size() returns, but I've > >>>> realized there are more things that users would like to know. My > >>>> question is whether you think that they would really appreciate it, > >>>> or > >>>> whether it's just my QA point of view where I sometimes compute the > >>>> 'checksums' of cache to see if I didn't lost anything. > >>>> > >>>> There are those sizes: > >>>> A) number of owned entries > >>>> B) number of entries stored locally in memory > >>>> C) number of entries stored in each local cache store > >>>> D) number of entries stored in each shared cache store > >>>> E) total number of entries in cache > >>>> > >>>> So far, we can get > >>>> B via withFlags(SKIP_CACHE_LOAD).size() > >>>> (passivation ? B : 0) + firstNonZero(C, D) via size() > >>>> E via distributed iterators / MR > >>>> A via data container iteration + distribution manager query, but only > >>>> without cache store > >>>> C or D through > >>>> > >>>> getComponentRegistry().getLocalComponent(PersistenceManager.class).getStores() > >>>> > >>>> I think that it would go along with users' expectations if size() > >>>> returned E and for the rest we should have special methods on > >>>> AdvancedCache. That would of course change the meaning of size(), but > >>>> I'd say that finally to something that has firm meaning. > >>>> > >>>> WDYT? > >>> There was a lot of arguments in past whether size() and other methods > >>> that operate over all the elements (keySet, values) are useful because: > >>> - they are approximate (data changes during iteration) > >>> - they are very resource consuming and might be miss-used (this is the > >>> reason we chosen to use size() with its current local semantic) > >>> > >>> These methods (size, keys, values) are useful for people and I think > >>> we were not wise to implement them only on top of the local data: this is > >>> like preferring efficiency over correctness. This also created a lot of > >>> confusion with our users, question like size() doesn't return the correct > >>> value being asked regularly. I totally agree that size() returns E (i.e. > >>> everything that is stored within the grid, including persistence) and it's > >>> performance implications to be documented accordingly. For keySet and values > >>> - we should stop implementing them (throw exception) and point users to > >>> Will's distributed iterator which is a nicer way to achieve the desired > >>> behavior. > >>> > >>> We can also implement keySet() and values() on top of the distributed > >>> entry iterator and document that using the iterator directly is better. > >> Yes, that's what I meant as well. > >> > >> Cheers, > >> -- > >> Mircea Markus > >> Infinispan lead (www.infinispan.org) > >> > >> > >> > >> > >> > >> _______________________________________________ > >> 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 > > > -- > Radim Vansa <rvansa(a)redhat.com&gt; > JBoss DataGrid QA > > _______________________________________________ > 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

Users expect that size() will be constant-time (or linear to cluster size), and generally fast operation. I'd prefer to keep it that way. Though, even the MR way (used for HotRod size() now) needs to crawl through all the entries locally.

'Heretic, not very well though of and changing too many things' idea: what about having data container segment-aware? Then you'd just bcast SizeCommand with given topologyId and sum up sizes of primary-owned segments... It's not a complete solution, but at least that would enable to get the number of locally owned entries quite fast. Though, you can't do that easily with cache stores (without changing SPI).

Regarding cache stores, IMO we're damned anyway: when calling cacheStore.size(), it can report more entries as those haven't been expired yet, it can report less entries as those can be expired due to [1]. Or, we'll enumerate all the entries, and that's going to be slow (btw., [1] reminded me that we should enumerate both datacontainer AND cachestores even if passivation is not enabled). Radim [1] https://issues.jboss.org/browse/ISPN-3202 On 10/08/2014 04:42 PM, William Burns wrote: > So it seems we would want to change this for 7.0 if possible since it > would be a bigger change for something like 7.1 and 8.0 would be even > further out. I should be able to put this together for CR2. > > It seems that we want to implement keySet, values and entrySet methods > using the entry iterator approach. > > It is however unclear for the size method if we want to use MR entry > counting and not worry about the rehash and passivation issues since > it is just an estimation anyways. Or if we want to also use the entry > iterator which should be closer approximation but will require more > network overhead and memory usage. > > Also we didn't really talk about the fact that these methods would > ignore ongoing transactions and if that is a concern or not. > > - Will > > On Wed, Oct 8, 2014 at 10:13 AM, Mircea Markus <mmarkus(a)redhat.com&gt; wrote: >> On Oct 8, 2014, at 15:11, Dan Berindei <dan.berindei(a)gmail.com&gt; wrote: >> >>> On Wed, Oct 8, 2014 at 5:03 PM, Mircea Markus <mmarkus(a)redhat.com&gt; wrote: >>> On Oct 3, 2014, at 9:30, Radim Vansa <rvansa(a)redhat.com&gt; wrote: >>> >>>> Hi, >>>> >>>> recently we had a discussion about what size() returns, but I've >>>> realized there are more things that users would like to know. My >>>> question is whether you think that they would really appreciate it, or >>>> whether it's just my QA point of view where I sometimes compute the >>>> 'checksums' of cache to see if I didn't lost anything. >>>> >>>> There are those sizes: >>>> A) number of owned entries >>>> B) number of entries stored locally in memory >>>> C) number of entries stored in each local cache store >>>> D) number of entries stored in each shared cache store >>>> E) total number of entries in cache >>>> >>>> So far, we can get >>>> B via withFlags(SKIP_CACHE_LOAD).size() >>>> (passivation ? B : 0) + firstNonZero(C, D) via size() >>>> E via distributed iterators / MR >>>> A via data container iteration + distribution manager query, but only >>>> without cache store >>>> C or D through >>>> getComponentRegistry().getLocalComponent(PersistenceManager.class).getStores() >>>> >>>> I think that it would go along with users' expectations if size() >>>> returned E and for the rest we should have special methods on >>>> AdvancedCache. That would of course change the meaning of size(), but >>>> I'd say that finally to something that has firm meaning. >>>> >>>> WDYT? >>> There was a lot of arguments in past whether size() and other methods that operate over all the elements (keySet, values) are useful because: >>> - they are approximate (data changes during iteration) >>> - they are very resource consuming and might be miss-used (this is the reason we chosen to use size() with its current local semantic) >>> >>> These methods (size, keys, values) are useful for people and I think we were not wise to implement them only on top of the local data: this is like preferring efficiency over correctness. This also created a lot of confusion with our users, question like size() doesn't return the correct value being asked regularly. I totally agree that size() returns E (i.e. everything that is stored within the grid, including persistence) and it's performance implications to be documented accordingly. For keySet and values - we should stop implementing them (throw exception) and point users to Will's distributed iterator which is a nicer way to achieve the desired behavior. >>> >>> We can also implement keySet() and values() on top of the distributed entry iterator and document that using the iterator directly is better. >> Yes, that's what I meant as well. >> >> Cheers, >> -- >> Mircea Markus >> Infinispan lead (www.infinispan.org) >> >> >> >> >> >> _______________________________________________ >> 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 -- Radim Vansa <rvansa(a)redhat.com&gt; JBoss DataGrid QA _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

On Wed, Oct 8, 2014 at 11:19 AM, Radim Vansa <rvansa(a)redhat.com&gt; wrote: > Users expect that size() will be constant-time (or linear to cluster > size), and generally fast operation. I'd prefer to keep it that way. > Though, even the MR way (used for HotRod size() now) needs to crawl > through all the entries locally. Many in memory collections require O(n) to do size such as ConcurrentLinkedQueue, so I wouldn't say size should always be expected to be constant time or O(c) where c is # of nodes. Granted a user can expect anything they want.

> 'Heretic, not very well though of and changing too many things' idea: > what about having data container segment-aware? Then you'd just bcast > SizeCommand with given topologyId and sum up sizes of primary-owned > segments... It's not a complete solution, but at least that would enable > to get the number of locally owned entries quite fast. Though, you can't > do that easily with cache stores (without changing SPI). > > Regarding cache stores, IMO we're damned anyway: when calling > cacheStore.size(), it can report more entries as those haven't been > expired yet, it can report less entries as those can be expired due to > [1]. Or, we'll enumerate all the entries, and that's going to be slow > (btw., [1] reminded me that we should enumerate both datacontainer AND > cachestores even if passivation is not enabled). This is precisely what the distributed iterator does. And also support for expired entries was recently integrated as I missed that in the original implementation [a] [a] https://issues.jboss.org/browse/ISPN-4643 > Radim > > [1] https://issues.jboss.org/browse/ISPN-3202 > > On 10/08/2014 04:42 PM, William Burns wrote: >> So it seems we would want to change this for 7.0 if possible since it >> would be a bigger change for something like 7.1 and 8.0 would be even >> further out. I should be able to put this together for CR2. >> >> It seems that we want to implement keySet, values and entrySet methods >> using the entry iterator approach. >> >> It is however unclear for the size method if we want to use MR entry >> counting and not worry about the rehash and passivation issues since >> it is just an estimation anyways. Or if we want to also use the entry >> iterator which should be closer approximation but will require more >> network overhead and memory usage. >> >> Also we didn't really talk about the fact that these methods would >> ignore ongoing transactions and if that is a concern or not. >> >> - Will >> >> On Wed, Oct 8, 2014 at 10:13 AM, Mircea Markus <mmarkus(a)redhat.com&gt; wrote: >>> On Oct 8, 2014, at 15:11, Dan Berindei <dan.berindei(a)gmail.com&gt; wrote: >>> >>>> On Wed, Oct 8, 2014 at 5:03 PM, Mircea Markus <mmarkus(a)redhat.com&gt; wrote: >>>> On Oct 3, 2014, at 9:30, Radim Vansa <rvansa(a)redhat.com&gt; wrote: >>>> >>>>> Hi, >>>>> >>>>> recently we had a discussion about what size() returns, but I've >>>>> realized there are more things that users would like to know. My >>>>> question is whether you think that they would really appreciate it, or >>>>> whether it's just my QA point of view where I sometimes compute the >>>>> 'checksums' of cache to see if I didn't lost anything. >>>>> >>>>> There are those sizes: >>>>> A) number of owned entries >>>>> B) number of entries stored locally in memory >>>>> C) number of entries stored in each local cache store >>>>> D) number of entries stored in each shared cache store >>>>> E) total number of entries in cache >>>>> >>>>> So far, we can get >>>>> B via withFlags(SKIP_CACHE_LOAD).size() >>>>> (passivation ? B : 0) + firstNonZero(C, D) via size() >>>>> E via distributed iterators / MR >>>>> A via data container iteration + distribution manager query, but only >>>>> without cache store >>>>> C or D through >>>>> getComponentRegistry().getLocalComponent(PersistenceManager.class).getStores() >>>>> >>>>> I think that it would go along with users' expectations if size() >>>>> returned E and for the rest we should have special methods on >>>>> AdvancedCache. That would of course change the meaning of size(), but >>>>> I'd say that finally to something that has firm meaning. >>>>> >>>>> WDYT? >>>> There was a lot of arguments in past whether size() and other methods that operate over all the elements (keySet, values) are useful because: >>>> - they are approximate (data changes during iteration) >>>> - they are very resource consuming and might be miss-used (this is the reason we chosen to use size() with its current local semantic) >>>> >>>> These methods (size, keys, values) are useful for people and I think we were not wise to implement them only on top of the local data: this is like preferring efficiency over correctness. This also created a lot of confusion with our users, question like size() doesn't return the correct value being asked regularly. I totally agree that size() returns E (i.e. everything that is stored within the grid, including persistence) and it's performance implications to be documented accordingly. For keySet and values - we should stop implementing them (throw exception) and point users to Will's distributed iterator which is a nicer way to achieve the desired behavior. >>>> >>>> We can also implement keySet() and values() on top of the distributed entry iterator and document that using the iterator directly is better. >>> Yes, that's what I meant as well. >>> >>> Cheers, >>> -- >>> Mircea Markus >>> Infinispan lead (www.infinispan.org) >>> >>> >>> >>> >>> >>> _______________________________________________ >>> 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 > > -- > Radim Vansa <rvansa(a)redhat.com&gt; > JBoss DataGrid QA > > _______________________________________________ > 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

What's wrong with sum(Datacontainer.size())/numOwners ? Tristan On 10/10/14 16:03, Radim Vansa wrote: > On 10/10/2014 02:38 PM, William Burns wrote: >> On Wed, Oct 8, 2014 at 11:19 AM, Radim Vansa <rvansa(a)redhat.com&gt; wrote: >>> Users expect that size() will be constant-time (or linear to cluster >>> size), and generally fast operation. I'd prefer to keep it that way. >>> Though, even the MR way (used for HotRod size() now) needs to crawl >>> through all the entries locally. >> Many in memory collections require O(n) to do size such as >> ConcurrentLinkedQueue, so I wouldn't say size should always be >> expected to be constant time or O(c) where c is # of nodes. Granted a >> user can expect anything they want. > OK, I stand corrected. Moreover, I was generalizing myself to all users, > a common mistake :) > > Anyway, monitoring tools love nice charts, and I can imagine monitoring > software polling every 1 second to update that cool chart with cache > size. Do we want a fast but imprecise variant of this operation in some > statistics class? > > Radim > >>> 'Heretic, not very well though of and changing too many things' idea: >>> what about having data container segment-aware? Then you'd just bcast >>> SizeCommand with given topologyId and sum up sizes of primary-owned >>> segments... It's not a complete solution, but at least that would enable >>> to get the number of locally owned entries quite fast. Though, you can't >>> do that easily with cache stores (without changing SPI). >>> >>> Regarding cache stores, IMO we're damned anyway: when calling >>> cacheStore.size(), it can report more entries as those haven't been >>> expired yet, it can report less entries as those can be expired due to >>> [1]. Or, we'll enumerate all the entries, and that's going to be slow >>> (btw., [1] reminded me that we should enumerate both datacontainer AND >>> cachestores even if passivation is not enabled). >> This is precisely what the distributed iterator does. And also >> support for expired entries was recently integrated as I missed that >> in the original implementation [a] >> >> [a] https://issues.jboss.org/browse/ISPN-4643 >> >>> Radim >>> >>> [1] https://issues.jboss.org/browse/ISPN-3202 >>> >>> On 10/08/2014 04:42 PM, William Burns wrote: >>>> So it seems we would want to change this for 7.0 if possible since it >>>> would be a bigger change for something like 7.1 and 8.0 would be even >>>> further out. I should be able to put this together for CR2. >>>> >>>> It seems that we want to implement keySet, values and entrySet methods >>>> using the entry iterator approach. >>>> >>>> It is however unclear for the size method if we want to use MR entry >>>> counting and not worry about the rehash and passivation issues since >>>> it is just an estimation anyways. Or if we want to also use the entry >>>> iterator which should be closer approximation but will require more >>>> network overhead and memory usage. >>>> >>>> Also we didn't really talk about the fact that these methods would >>>> ignore ongoing transactions and if that is a concern or not. >>>> >>>> - Will >>>> >>>> On Wed, Oct 8, 2014 at 10:13 AM, Mircea Markus <mmarkus(a)redhat.com&gt; wrote: >>>>> On Oct 8, 2014, at 15:11, Dan Berindei <dan.berindei(a)gmail.com&gt; wrote: >>>>> >>>>>> On Wed, Oct 8, 2014 at 5:03 PM, Mircea Markus <mmarkus(a)redhat.com&gt; wrote: >>>>>> On Oct 3, 2014, at 9:30, Radim Vansa <rvansa(a)redhat.com&gt; wrote: >>>>>> >>>>>>> Hi, >>>>>>> >>>>>>> recently we had a discussion about what size() returns, but I've >>>>>>> realized there are more things that users would like to know. My >>>>>>> question is whether you think that they would really appreciate it, or >>>>>>> whether it's just my QA point of view where I sometimes compute the >>>>>>> 'checksums' of cache to see if I didn't lost anything. >>>>>>> >>>>>>> There are those sizes: >>>>>>> A) number of owned entries >>>>>>> B) number of entries stored locally in memory >>>>>>> C) number of entries stored in each local cache store >>>>>>> D) number of entries stored in each shared cache store >>>>>>> E) total number of entries in cache >>>>>>> >>>>>>> So far, we can get >>>>>>> B via withFlags(SKIP_CACHE_LOAD).size() >>>>>>> (passivation ? B : 0) + firstNonZero(C, D) via size() >>>>>>> E via distributed iterators / MR >>>>>>> A via data container iteration + distribution manager query, but only >>>>>>> without cache store >>>>>>> C or D through >>>>>>> getComponentRegistry().getLocalComponent(PersistenceManager.class).getStores() >>>>>>> >>>>>>> I think that it would go along with users' expectations if size() >>>>>>> returned E and for the rest we should have special methods on >>>>>>> AdvancedCache. That would of course change the meaning of size(), but >>>>>>> I'd say that finally to something that has firm meaning. >>>>>>> >>>>>>> WDYT? >>>>>> There was a lot of arguments in past whether size() and other methods that operate over all the elements (keySet, values) are useful because: >>>>>> - they are approximate (data changes during iteration) >>>>>> - they are very resource consuming and might be miss-used (this is the reason we chosen to use size() with its current local semantic) >>>>>> >>>>>> These methods (size, keys, values) are useful for people and I think we were not wise to implement them only on top of the local data: this is like preferring efficiency over correctness. This also created a lot of confusion with our users, question like size() doesn't return the correct value being asked regularly. I totally agree that size() returns E (i.e. everything that is stored within the grid, including persistence) and it's performance implications to be documented accordingly. For keySet and values - we should stop implementing them (throw exception) and point users to Will's distributed iterator which is a nicer way to achieve the desired behavior. >>>>>> >>>>>> We can also implement keySet() and values() on top of the distributed entry iterator and document that using the iterator directly is better. >>>>> Yes, that's what I meant as well. >>>>> >>>>> Cheers, >>>>> -- >>>>> Mircea Markus >>>>> Infinispan lead (www.infinispan.org) >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> _______________________________________________ >>>>> 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 >>> -- >>> Radim Vansa <rvansa(a)redhat.com&gt; >>> JBoss DataGrid QA >>> >>> _______________________________________________ >>> 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 _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

On Oct 10, 2014, at 15:18, Radim Vansa <rvansa(a)redhat.com&gt; wrote: > That we should expose that as one method, not forcing people to > implement the sum() themselves. Hmm, isn't the method you mention cache.size() ? :-)

> > And possibly cachestores, again. AdvancedCacheLoader has size. > > Radim > > On 10/10/2014 04:06 PM, Tristan Tarrant wrote: >> What's wrong with sum(Datacontainer.size())/numOwners ? >> >> Tristan >> >> On 10/10/14 16:03, Radim Vansa wrote: >>> On 10/10/2014 02:38 PM, William Burns wrote: >>>> On Wed, Oct 8, 2014 at 11:19 AM, Radim Vansa <rvansa(a)redhat.com&gt; wrote: >>>>> Users expect that size() will be constant-time (or linear to cluster >>>>> size), and generally fast operation. I'd prefer to keep it that way. >>>>> Though, even the MR way (used for HotRod size() now) needs to crawl >>>>> through all the entries locally. >>>> Many in memory collections require O(n) to do size such as >>>> ConcurrentLinkedQueue, so I wouldn't say size should always be >>>> expected to be constant time or O(c) where c is # of nodes. Granted a >>>> user can expect anything they want. >>> OK, I stand corrected. Moreover, I was generalizing myself to all users, >>> a common mistake :) >>> >>> Anyway, monitoring tools love nice charts, and I can imagine monitoring >>> software polling every 1 second to update that cool chart with cache >>> size. Do we want a fast but imprecise variant of this operation in some >>> statistics class? >>> >>> Radim >>> >>>>> 'Heretic, not very well though of and changing too many things' idea: >>>>> what about having data container segment-aware? Then you'd just bcast >>>>> SizeCommand with given topologyId and sum up sizes of primary-owned >>>>> segments... It's not a complete solution, but at least that would enable >>>>> to get the number of locally owned entries quite fast. Though, you can't >>>>> do that easily with cache stores (without changing SPI). >>>>> >>>>> Regarding cache stores, IMO we're damned anyway: when calling >>>>> cacheStore.size(), it can report more entries as those haven't been >>>>> expired yet, it can report less entries as those can be expired due to >>>>> [1]. Or, we'll enumerate all the entries, and that's going to be slow >>>>> (btw., [1] reminded me that we should enumerate both datacontainer AND >>>>> cachestores even if passivation is not enabled). >>>> This is precisely what the distributed iterator does. And also >>>> support for expired entries was recently integrated as I missed that >>>> in the original implementation [a] >>>> >>>> [a] https://issues.jboss.org/browse/ISPN-4643 >>>> >>>>> Radim >>>>> >>>>> [1] https://issues.jboss.org/browse/ISPN-3202 >>>>> >>>>> On 10/08/2014 04:42 PM, William Burns wrote: >>>>>> So it seems we would want to change this for 7.0 if possible since it >>>>>> would be a bigger change for something like 7.1 and 8.0 would be even >>>>>> further out. I should be able to put this together for CR2. >>>>>> >>>>>> It seems that we want to implement keySet, values and entrySet methods >>>>>> using the entry iterator approach. >>>>>> >>>>>> It is however unclear for the size method if we want to use MR entry >>>>>> counting and not worry about the rehash and passivation issues since >>>>>> it is just an estimation anyways. Or if we want to also use the entry >>>>>> iterator which should be closer approximation but will require more >>>>>> network overhead and memory usage. >>>>>> >>>>>> Also we didn't really talk about the fact that these methods would >>>>>> ignore ongoing transactions and if that is a concern or not. >>>>>> >>>>>> - Will >>>>>> >>>>>> On Wed, Oct 8, 2014 at 10:13 AM, Mircea Markus <mmarkus(a)redhat.com&gt; wrote: >>>>>>> On Oct 8, 2014, at 15:11, Dan Berindei <dan.berindei(a)gmail.com&gt; wrote: >>>>>>> >>>>>>>> On Wed, Oct 8, 2014 at 5:03 PM, Mircea Markus <mmarkus(a)redhat.com&gt; wrote: >>>>>>>> On Oct 3, 2014, at 9:30, Radim Vansa <rvansa(a)redhat.com&gt; wrote: >>>>>>>> >>>>>>>>> Hi, >>>>>>>>> >>>>>>>>> recently we had a discussion about what size() returns, but I've >>>>>>>>> realized there are more things that users would like to know. My >>>>>>>>> question is whether you think that they would really appreciate it, or >>>>>>>>> whether it's just my QA point of view where I sometimes compute the >>>>>>>>> 'checksums' of cache to see if I didn't lost anything. >>>>>>>>> >>>>>>>>> There are those sizes: >>>>>>>>> A) number of owned entries >>>>>>>>> B) number of entries stored locally in memory >>>>>>>>> C) number of entries stored in each local cache store >>>>>>>>> D) number of entries stored in each shared cache store >>>>>>>>> E) total number of entries in cache >>>>>>>>> >>>>>>>>> So far, we can get >>>>>>>>> B via withFlags(SKIP_CACHE_LOAD).size() >>>>>>>>> (passivation ? B : 0) + firstNonZero(C, D) via size() >>>>>>>>> E via distributed iterators / MR >>>>>>>>> A via data container iteration + distribution manager query, but only >>>>>>>>> without cache store >>>>>>>>> C or D through >>>>>>>>> getComponentRegistry().getLocalComponent(PersistenceManager.class).getStores() >>>>>>>>> >>>>>>>>> I think that it would go along with users' expectations if size() >>>>>>>>> returned E and for the rest we should have special methods on >>>>>>>>> AdvancedCache. That would of course change the meaning of size(), but >>>>>>>>> I'd say that finally to something that has firm meaning. >>>>>>>>> >>>>>>>>> WDYT? >>>>>>>> There was a lot of arguments in past whether size() and other methods that operate over all the elements (keySet, values) are useful because: >>>>>>>> - they are approximate (data changes during iteration) >>>>>>>> - they are very resource consuming and might be miss-used (this is the reason we chosen to use size() with its current local semantic) >>>>>>>> >>>>>>>> These methods (size, keys, values) are useful for people and I think we were not wise to implement them only on top of the local data: this is like preferring efficiency over correctness. This also created a lot of confusion with our users, question like size() doesn't return the correct value being asked regularly. I totally agree that size() returns E (i.e. everything that is stored within the grid, including persistence) and it's performance implications to be documented accordingly. For keySet and values - we should stop implementing them (throw exception) and point users to Will's distributed iterator which is a nicer way to achieve the desired behavior. >>>>>>>> >>>>>>>> We can also implement keySet() and values() on top of the distributed entry iterator and document that using the iterator directly is better. >>>>>>> Yes, that's what I meant as well. >>>>>>> >>>>>>> Cheers, >>>>>>> -- >>>>>>> Mircea Markus >>>>>>> Infinispan lead (www.infinispan.org) >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> _______________________________________________ >>>>>>> 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 >>>>> -- >>>>> Radim Vansa <rvansa(a)redhat.com&gt; >>>>> JBoss DataGrid QA >>>>> >>>>> _______________________________________________ >>>>> 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 >> _______________________________________________ >> infinispan-dev mailing list >> infinispan-dev(a)lists.jboss.org >> https://lists.jboss.org/mailman/listinfo/infinispan-dev > > > -- > Radim Vansa <rvansa(a)redhat.com&gt; > JBoss DataGrid QA > > _______________________________________________ > infinispan-dev mailing list > infinispan-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/infinispan-dev Cheers, -- Mircea Markus Infinispan lead (www.infinispan.org) _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

On Oct 10, 2014, at 15:25, Dan Berindei <dan.berindei(a)gmail.com&gt; wrote: > On Fri, Oct 10, 2014 at 5:20 PM, Mircea Markus <mmarkus(a)redhat.com&gt; wrote: > > On Oct 10, 2014, at 15:18, Radim Vansa <rvansa(a)redhat.com&gt; wrote: > > > That we should expose that as one method, not forcing people to > > implement the sum() themselves. > > Hmm, isn't the method you mention cache.size() ? :-) > > Nope, because we decided to make cache.size() precise-but-slow :) It's not possible to make it precise unless we provide snapshot isolation /MVCC support.

of the data. And definitely way better than what we currently have. (Looking at CHM.size() they offer an "accurate" size of the map by counting it in a loop and making sure that the size is reproducible. I don't think that's accurate in the general case, though, as you might count intermediate sizes in that loop).

I meant as a quick implementation of that, not that we want to tell users to do it.

So it seems we would want to change this for 7.0 if possible since it would be a bigger change for something like 7.1 and 8.0 would be even further out. I should be able to put this together for CR2.