<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Oct 7, 2014 at 4:17 PM, William Burns <span dir="ltr"><<a href="mailto:mudokonman@gmail.com" target="_blank">mudokonman@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span>On Tue, Oct 7, 2014 at 8:43 AM, Radim Vansa <<a href="mailto:rvansa@redhat.com" target="_blank">rvansa@redhat.com</a>> wrote:<br>
> On 10/07/2014 02:21 PM, William Burns wrote:<br>
>> On Tue, Oct 7, 2014 at 7:32 AM, Radim Vansa <<a href="mailto:rvansa@redhat.com" target="_blank">rvansa@redhat.com</a>> wrote:<br>
>>> If you have one local and one shared cache store, how should the command<br>
>>> behave?<br>
>>><br>
>>> a) distexec/MR sum of cache.withFlags(SKIP_REMOTE_LOOKUP,<br>
>>> SKIP_BACKUP_ENTRIES).size() from all nodes? (note that there's no<br>
>>> SKIP_BACKUP_ENTRIES flag right now), where this method returns<br>
>>> localStore.size() for first non-shared cache store + passivation ?<br>
>>> dataContainer.size(SKIP_BACKUP_ENTRIES) : 0)<br>
>> Calling the size method in either distexec or MR will give you<br>
>> inflated numbers as you need to pay attention to the numOwners to get<br>
>> a proper count.<br>
><br>
> That's what I meant by the SKIP_BACKUP_ENTRIES - dataContainer should be<br>
> able to report only primary-owned entries, or we have to iterate and<br>
> apply the filtering outside.<br>
<br>
</span>If we added this functionality then yes it would be promoted up to MR<br>
counting entries status though it would still have issues with rehash.<br>
As well as issues with concurrent activations and passivations.<br></blockquote><div><br></div><div>I think we can use something like OutdatedTopologyException to make sure we count each segment once, on the primary owner. But in order to verify that a particular node is the primary owner we'd have to load each cache store entry, so performance with cache stores will be pretty bad.</div><div><br></div><div>Dealing with concurrent activations/passivations will is even trickier.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<span><br>
><br>
>><br>
>>> b) distexec/MR sum of sharedStore.size() + passivation ? sum of<br>
>>> dataContainer.size(SKIP_BACKUP_ENTRIES) : 0<br>
>> Calling the size on a shared cache actually should work somewhat well<br>
>> (assuming all entries are stored in the shared cache). The problem is<br>
>> if passivation is enabled as you point out because you also have to<br>
>> check the data container which means you can also have an issue with<br>
>> concurrent activations and passivations (which you can't verify<br>
>> properly in either case without knowing the keys).<br>
>><br>
>>> c) MR that would count the entries<br>
>> This is the only reliable way to do this with MR. And unfortunately<br>
>> if a rehash occurs I am not sure if you would get inconsistent numbers<br>
>> or an Exception. In the latter at least you should be able to make<br>
>> sure that you have the proper number when it does return without<br>
>> exception. I can't say how it works with multiple loaders though, my<br>
>> guess is that it may process the entry more than once so it depends on<br>
>> if your mapper is smart enough to realize it.<br>
><br>
> I don't think that reporting incorrect size is *that* harmful - even<br>
> ConcurrentMap interface says that it's just a wild guess and when things<br>
> are changing, you can't rely on that.<br>
<br>
</span>ConcurrentMap doesn't say anything about size method actually.<br>
ConcurrentHashMap has some verbage about saying that it might not be<br>
completely correct under concurrent modification though.<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><br>
It isn't a wild guess really though for ConcurrentHashMap. The worst<br>
is that you could count a value that was there but it is now removed<br>
or you don't count a value that was recently added. Really the<br>
guarantee from CHM is that it counts each individual segment properly<br>
for a glimpse of time for that segment, the problem is that each<br>
segment could change (since they are counted at different times). But<br>
the values missing in ConcurrentHashMap are totally different than<br>
losing an entire segment due to a rehash. You could theoretically<br>
have a rehash occur right after MR started iterating and see no values<br>
for that segment or a very small subset. There is a much larger<br>
margin of error in this case for what values are seen and which are<br>
not.<br>
<span><br></span></blockquote><div><br></div><div><div>Interesting... the Map javadoc seems to assume linearizability, maybe because the original implementation was Hashtable :)</div><div><br></div><div>So there is precedent for relaxing the definition of size(). But of course some users will still expect a 0 error margin when there are no concurrent writes, so I agree we don't get a free pass to ignore rehashes and activations during get().</div></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span>
><br>
>><br>
>>> d) wrapper on distributed entry iteration with converters set to return<br>
>>> 0-sized entries<br>
>> Entry iterator can't return 0 sized entries (just the values). The<br>
>> keys are required to make sure that the count is correct and also to<br>
>> ensure that if a rehash happens in the middle it can properly continue<br>
>> to operate without having to start over. Entry iterator should work<br>
>> properly irrespective of the number of stores/loaders that are<br>
>> configured, since it keep track of already seen keys (so duplicates<br>
>> are ignored).<br>
><br>
> Ok, I was simplifying that a bit. And by the way, I don't really like<br>
> the fact that for distributed entry iteration you need to be able to<br>
> keep all keys from one segment at one moment in memory. But fine -<br>
> distributed entry iteration is probably not the right way.<br>
<br>
</span>I agree it is annoying to have to keep the keys, but it is one of the<br>
few ways to reliably get all the values without losing one. Actually<br>
this approach provides a much closer approximation to what<br>
ConcurrentHashMap provides for its size implementation, since it can't<br>
drop a segment. It is pretty much required to do it this way to do<br>
keySet, entrySet, and values where you don't have the luxury of<br>
dropping whole swaths of entries like you do with calling size()<br>
method (even if the value(s) was there the entire time).<br></blockquote><div><br></div><div>If we decide to improve size(), I'd vote to use distributed entry iterators.</div><div><br></div><div>We may be able to avoid sending all the keys to the originator when the cache doesn't have any stores. But with a store it looks like we can't avoid reading all the keys from the store, so skipping the transfer of the keys wouldn't help that much.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<span><br>
><br>
>><br>
>><br>
>>> And what about nodes with different configuration?<br>
>> Hard to know without knowing what the differences are.<br>
><br>
> I had in my mind different loaders and passivation configuration (e.g.<br>
> some node could use shared store and some don't - do we want to handle<br>
> such obscure configs? Can we design that without the need to have<br>
> complicated decision trees what to include and what not?).<br>
<br>
</span>Well the last sentence means we have to use MR or Entry Iterator since<br>
we can't call size on the shared loader. I would think that it should<br>
still work irrespective of the loader configuration (except for MR<br>
with multiple loaders). The main issue I can think of is that if<br>
everyone isn't using the shared loader that you could have stale<br>
values in the loader if you don't always have a node using the shared<br>
loader up (assuming purge at startup isn't enabled).<br></blockquote><div><br></div><div>We really shouldn't support different store/loader configurations on each node, except for minor stuff like paths.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div><div><br>
><br>
> Radim<br>
><br>
>><br>
>>> Radim<br>
>>><br>
>>> On 10/06/2014 01:57 PM, Sanne Grinovero wrote:<br>
>>>> On 6 October 2014 12:44, Tristan Tarrant <<a href="mailto:ttarrant@redhat.com" target="_blank">ttarrant@redhat.com</a>> wrote:<br>
>>>>> I think we should provide correct implementations of size() (and others)<br>
>>>>> and provide shortcut implementations using our usual Flag API (e.g.<br>
>>>>> SKIP_REMOTE_LOOKUP).<br>
>>>> Right that would be very nice. Same for CacheStore interaction: all<br>
>>>> cachestores should be included unless skipped explicitly.<br>
>>>><br>
>>>> Sanne<br>
>>>><br>
>>>>> Tristan<br>
>>>>><br>
>>>>> On 06/10/14 12:57, Sanne Grinovero wrote:<br>
>>>>>> On 3 October 2014 18:38, Dennis Reed <<a href="mailto:dereed@redhat.com" target="_blank">dereed@redhat.com</a>> wrote:<br>
>>>>>>> Since size() is defined by the ConcurrentMap interface, it already has a<br>
>>>>>>> precisely defined meaning. The only "correct" implementation is E.<br>
>>>>>> +1<br>
>> This is one of the things I have been wanting to do is actually<br>
>> implement the other Map methods across the entire cache. However to<br>
>> do a lot of these in a memory conscious way they would need to be ran<br>
>> ignoring any ongoing transactions. Actually having this requirement<br>
>> allows these methods to be implemented quite easily especially in<br>
>> conjunction with the EntryIterator. I almost made a PR for it a while<br>
>> back, but it seemed a little zealous to do at the same time and it<br>
>> didn't seem that people were pushing for it very hard (maybe that was<br>
>> a wrong assumption). Also I wasn't quite sure the transactional part<br>
>> not being functional anymore would be a deterrent.<br>
>><br>
>>>>>>> The current non-correct implementation was just because it's expensive<br>
>>>>>>> to calculate correctly. I'm not sure the current impl is really that<br>
>>>>>>> useful for anything.<br>
>>>>>> +1<br>
>>>>>><br>
>>>>>> And not just size() but many others from ConcurrentMap.<br>
>>>>>> The question is if we should drop the interface and all the methods<br>
>>>>>> which aren't efficiently implementable, or fix all those methods.<br>
>>>>>><br>
>>>>>> In the past I loved that I could inject "Infinispan superpowers" into<br>
>>>>>> an application making extensive use of Map and ConcurrentMap without<br>
>>>>>> changes, but that has been deceiving and required great care such as<br>
>>>>>> verifying that these features would not be used anywhere in the code.<br>
>>>>>> I ended up wrapping the Cache implementation in a custom adapter which<br>
>>>>>> would also implement ConcurrentMap but would throw a RuntimeException<br>
>>>>>> if any of the "unallowed" methods was called, at least I would detect<br>
>>>>>> violations safely.<br>
>>>>>><br>
>>>>>> I still think that for the time being - until a better solution is<br>
>>>>>> planned - we should throw exceptions.. alas that's an old conversation<br>
>>>>>> and it was never done.<br>
>>>>>><br>
>>>>>> Sanne<br>
>>>>>><br>
>>>>>><br>
>>>>>>> -Dennis<br>
>>>>>>><br>
>>>>>>> On 10/03/2014 03:30 AM, Radim Vansa wrote:<br>
>>>>>>>> Hi,<br>
>>>>>>>><br>
>>>>>>>> recently we had a discussion about what size() returns, but I've<br>
>>>>>>>> realized there are more things that users would like to know. My<br>
>>>>>>>> question is whether you think that they would really appreciate it, or<br>
>>>>>>>> whether it's just my QA point of view where I sometimes compute the<br>
>>>>>>>> 'checksums' of cache to see if I didn't lost anything.<br>
>>>>>>>><br>
>>>>>>>> There are those sizes:<br>
>>>>>>>> A) number of owned entries<br>
>>>>>>>> B) number of entries stored locally in memory<br>
>>>>>>>> C) number of entries stored in each local cache store<br>
>>>>>>>> D) number of entries stored in each shared cache store<br>
>>>>>>>> E) total number of entries in cache<br>
>>>>>>>><br>
>>>>>>>> So far, we can get<br>
>>>>>>>> B via withFlags(SKIP_CACHE_LOAD).size()<br>
>>>>>>>> (passivation ? B : 0) + firstNonZero(C, D) via size()<br>
>>>>>>>> E via distributed iterators / MR<br>
>>>>>>>> A via data container iteration + distribution manager query, but only<br>
>>>>>>>> without cache store<br>
>>>>>>>> C or D through<br>
>>>>>>>> getComponentRegistry().getLocalComponent(PersistenceManager.class).getStores()<br>
>>>>>>>><br>
>>>>>>>> I think that it would go along with users' expectations if size()<br>
>>>>>>>> returned E and for the rest we should have special methods on<br>
>>>>>>>> AdvancedCache. That would of course change the meaning of size(), but<br>
>>>>>>>> I'd say that finally to something that has firm meaning.<br>
>>>>>>>><br>
>>>>>>>> WDYT?<br>
>>>>>>>><br>
>>>>>>>> Radim<br>
>>>>>>>><br>
>>>>>>> _______________________________________________<br>
>>>>>>> infinispan-dev mailing list<br>
>>>>>>> <a href="mailto:infinispan-dev@lists.jboss.org" target="_blank">infinispan-dev@lists.jboss.org</a><br>
>>>>>>> <a href="https://lists.jboss.org/mailman/listinfo/infinispan-dev" target="_blank">https://lists.jboss.org/mailman/listinfo/infinispan-dev</a><br>
>>>>>> _______________________________________________<br>
>>>>>> infinispan-dev mailing list<br>
>>>>>> <a href="mailto:infinispan-dev@lists.jboss.org" target="_blank">infinispan-dev@lists.jboss.org</a><br>
>>>>>> <a href="https://lists.jboss.org/mailman/listinfo/infinispan-dev" target="_blank">https://lists.jboss.org/mailman/listinfo/infinispan-dev</a><br>
>>>>>><br>
>>>>>><br>
>>>>> _______________________________________________<br>
>>>>> infinispan-dev mailing list<br>
>>>>> <a href="mailto:infinispan-dev@lists.jboss.org" target="_blank">infinispan-dev@lists.jboss.org</a><br>
>>>>> <a href="https://lists.jboss.org/mailman/listinfo/infinispan-dev" target="_blank">https://lists.jboss.org/mailman/listinfo/infinispan-dev</a><br>
>>>> _______________________________________________<br>
>>>> infinispan-dev mailing list<br>
>>>> <a href="mailto:infinispan-dev@lists.jboss.org" target="_blank">infinispan-dev@lists.jboss.org</a><br>
>>>> <a href="https://lists.jboss.org/mailman/listinfo/infinispan-dev" target="_blank">https://lists.jboss.org/mailman/listinfo/infinispan-dev</a><br>
>>><br>
>>> --<br>
>>> Radim Vansa <<a href="mailto:rvansa@redhat.com" target="_blank">rvansa@redhat.com</a>><br>
>>> JBoss DataGrid QA<br>
>>><br>
>>> _______________________________________________<br>
>>> infinispan-dev mailing list<br>
>>> <a href="mailto:infinispan-dev@lists.jboss.org" target="_blank">infinispan-dev@lists.jboss.org</a><br>
>>> <a href="https://lists.jboss.org/mailman/listinfo/infinispan-dev" target="_blank">https://lists.jboss.org/mailman/listinfo/infinispan-dev</a><br>
>> _______________________________________________<br>
>> infinispan-dev mailing list<br>
>> <a href="mailto:infinispan-dev@lists.jboss.org" target="_blank">infinispan-dev@lists.jboss.org</a><br>
>> <a href="https://lists.jboss.org/mailman/listinfo/infinispan-dev" target="_blank">https://lists.jboss.org/mailman/listinfo/infinispan-dev</a><br>
><br>
><br>
> --<br>
> Radim Vansa <<a href="mailto:rvansa@redhat.com" target="_blank">rvansa@redhat.com</a>><br>
> JBoss DataGrid QA<br>
><br>
> _______________________________________________<br>
> infinispan-dev mailing list<br>
> <a href="mailto:infinispan-dev@lists.jboss.org" target="_blank">infinispan-dev@lists.jboss.org</a><br>
> <a href="https://lists.jboss.org/mailman/listinfo/infinispan-dev" target="_blank">https://lists.jboss.org/mailman/listinfo/infinispan-dev</a><br>
_______________________________________________<br>
infinispan-dev mailing list<br>
<a href="mailto:infinispan-dev@lists.jboss.org" target="_blank">infinispan-dev@lists.jboss.org</a><br>
<a href="https://lists.jboss.org/mailman/listinfo/infinispan-dev" target="_blank">https://lists.jboss.org/mailman/listinfo/infinispan-dev</a><br>
</div></div></blockquote></div><br></div></div>