<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Oct 7, 2013 at 2:30 AM, Sanne Grinovero <span dir="ltr"><<a href="mailto:sanne@infinispan.org" target="_blank">sanne@infinispan.org</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div>On 6 October 2013 00:01, Pedro Ruivo <<a href="mailto:pedro@infinispan.org" target="_blank">pedro@infinispan.org</a>> wrote:<br>
> Hi Sanne.<br>
><br>
> Thanks for your comments. please see inline...<br>
><br>
> Cheers,<br>
> Pedro<br>
><br>
><br>
> On 10/05/2013 09:15 PM, Sanne Grinovero wrote:<br>
>><br>
>> Hi Pedro,<br>
>> looks like you're diving in some good fun :-)<br>
>> BTW please keep the dev discussions on the mailing list, adding it.<br>
>><br>
>> inline :<br>
>><br>
>> On 4 October 2013 22:01, Pedro Ruivo <<a href="mailto:pedro@infinispan.org" target="_blank">pedro@infinispan.org</a>> wrote:<br>
>>><br>
>>> Hi,<br>
>>><br>
>>> Sanne I need your expertise in here. I'm afraid that the problem is in<br>
>>> FileListOperations :(<br>
>>> I think the FileListOperations implementation needs a transactional cache<br>
>>> with strong consistency...<br>
>>><br>
>>> I'm 99% sure that it is originating the java.lang.AssertionError: file<br>
>>> XPTO<br>
>>> does not exist. I find out that we have multiple threads adding and<br>
>>> removing<br>
>>> files from the list. The scenario in [1] we see 2 threads loading the key<br>
>>> from the cache loader and one thread adds a file and other removes. the<br>
>>> thread that removes is the last one to commit and the file list is<br>
>>> updated<br>
>>> to an old state. When it tries to updat an index, I got the assertion<br>
>>> error.<br>
>><br>
>><br>
>> Nice, looks like you're on something.<br>
>> I've never seen specifically an AssertionError, looks like you have a<br>
>> new test. Could you share it?<br>
><br>
><br>
> yes of course:<br>
> <a href="https://github.com/pruivo/infinispan/blob/a4483d08b92d301350823c7fd42725c339a65c7b/query/src/test/java/org/infinispan/query/cacheloaders/CacheStoreTest.java" target="_blank">https://github.com/pruivo/infinispan/blob/a4483d08b92d301350823c7fd42725c339a65c7b/query/src/test/java/org/infinispan/query/cacheloaders/CacheStoreTest.java</a><br>
><br>
> so far, only the tests with eviction are failing...<br>
<br>
</div></div>Some of the failures you're seeing are caused by internal "assert"<br>
keyword in Lucene's code, which have the purpose of verifying the<br>
"filesystem" is going to be synched properly.<br>
These assertions don't apply when using our storage: we don't need<br>
this synch to happen: in fact if it weren't because of the assertions<br>
the whole method would be a no-op as it finally delegates all logic to<br>
a method in the Infinispan Directory which is a no-op.<br>
<br>
In other words, these are misleading failures and we'd need to avoid<br>
the TestNG "feature" of enabling assertions in this case.<br>
<br>
Still, even if the stacktrace is misleading, I agree on your diagnosis below.<br>
<br>
Could you reproduce the problem without involving also the Query framework?<br>
I'd hope that such a test could be independent and live solely in the<br>
lucene-directory module; in practice if you can only reproduce it with<br>
the query module it makes me suspicious that we're actually debugging<br>
a race condition in the initialization of the two services: a race<br>
between the query initialization thread needing to check the index<br>
state (so potentially triggering a load from cachestore), and the<br>
thread performing the cachestore preload.<br>
(I see your test also fails without preload, but just wondering if<br>
that might be an additional complexity).<br>
<div><br>
>><br>
>> Let's step back a second and consider the Cache usage from the point<br>
>> of view of FileListOperations.<br>
>> Note that even if you have two threads writing at the same time, as<br>
>> long as they are on the same node they will be adding/removing<br>
>> elements from the same instance of a ConcurrentHashMap.<br>
>> Since it's the same instance, it doesn't matter which thread will do<br>
>> the put operation as last: it will push the correct state.<br>
>> (there is an assumptions here, but we can forget about those for the<br>
>> sake of this debugging: same node -> fine as there is an external<br>
>> lock, no other node is allowed to write at the same time)<br>
>><br>
><br>
> 100% agreed with you but with cache store, we no longer ensure that 2 (or<br>
> more) threads are pointing to the same instance of Concurrent Hash Set.<br>
><br>
> With eviction, the entries are removed from in-memory container and<br>
> persisted in the cache store. The scenario I've described, 2 threads are<br>
> trying to add/remove a file and the file list does not exist in-memory. So,<br>
> each thread will read from cache store and deserialize the byte array. In<br>
> the end, each thread will have a pointer for different instances of<br>
> ConcurrentHashSet but with the same elements. And when this happens, we lost<br>
> one of the operation.<br>
<br>
</div>I'm seeing more than a couple of different smelly behaviors interacting here:<br>
<br>
1## The single instance ConcurrentHashSet<br>
I guess this could be re-thought as it's making some strong<br>
assumptions, but considering this service can't be transactional I'd<br>
rather explore other solutions first as I think the following changes<br>
should be good enough.<br>
<br>
2## Eviction not picking the right entry<br>
This single key is literally read for each and every performed query,<br>
and all writes as well. Each write, will write on this key.<br>
Even with eviction being enabled on the cache, I would never expect<br>
this key to be actually evicted!<br>
<br>
# Action 1: Open an issue to investigate the eviction choice: the<br>
strategy seems to be making a very poor job (or maybe it's just that<br>
maxEntries(10) is too low and makes LIRS degenerate into insane<br>
choices).<br></blockquote><div><br></div><div>That well may be the case: the default concurrency level is 32, so with a capacity of 10 you will have just 1 entry per segment. If there is more than one entry in the same segment and they are both "hot", they will take turns being evicted.<br>
<br></div><div>The only thing we could change here is to make LIRS/LRU work for the entire container at once, but right now I don't think they're scalable enough.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
# Action 2: I think that for now we could disallow usage of eviction<br>
on the metadata cache. I didn't have tests using it, as I wouldn't<br>
recommended such a configuration as these entries are very hot and<br>
very small: viable to make it an illegal option?<br>
<br>
3## The CacheLoader loading the same entry multiple times in parallel<br>
Kudos for finding out that there are situations in which we deal with<br>
multiple different instances of ConcurrentHashSet! Still, I think that<br>
Infinispan core is terribly wrong in this case:<br>
from the client code POV a new CHS is created with a put-if-absent<br>
atomic operation, and I will assume there that core will check/load<br>
any enabled cachestore as well.<br>
To handle multiple GET operations in parallel, or even in parallel<br>
with preload or the client's put-if-absent operation, I would *not*<br>
expect Infinispan core to storm the CacheStore implementation with<br>
multiple load operations on the same put: a lock should be hold on the<br>
potential key during such a load operation.<br>
<br>
If your observation is right, this could also be one of the reasons<br>
for so many complaints on the CacheStore performance: under these<br>
circumstances - which I'd guesstimate are quite common - we're doing<br>
lots of unnecessary IO, potentially stressing the slower storage<br>
devices. This could even have dramatic effects if there are frequent<br>
requests for entries for which the returned value is a null.<br>
<br>
# Action 3: Investigate and open a JIRA about the missing locking on<br>
CacheStore load operations.<br>
<br>
If this where resolves, we would still have a guarantee of single<br>
instance, am I correct?<br></blockquote><div><br></div><div>I don't think so.<br><br></div><div>Consider this scenario:<br><br></div><div>1. thread 1: set1 = cache.get(key)<br></div><div>2. eviction thread: evict/passivate<br>
</div>
<div>3. thread 2: set2 = cache.get(key)<br></div><br></div><div class="gmail_quote">We don't know if a thread is still holding a reference to a value during eviction, so we can't guarantee that there will always be a single instance of that value.<br>
<br></div><div class="gmail_quote">OTOH, if you need this guarantee, and you don't really need eviction, wouldn't it be simpler to use a plain ConcurrentHashMap instead?<br><br></div><div class="gmail_quote">
<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><br>
><br>
> Also, the problem is easily reproduce when you enable the storeAsBinary for<br>
> values because each cache.get will deserialize the byte array and create<br>
> different instances.<br>
<br>
</div>That's another option that I've never used on a cache meant to store a<br>
Lucene index as it doesn't make sense for the purpose.<br>
But you're right that's an assumption I haven't thought of:<br>
<br>
# Action 4: What do you think of promoting that as an illegal<br>
configuration? created ISPN-3593<br>
<div><br>
><br>
> That's why I think we would need a transaction.<br>
<br>
</div>-1 the Directory can not drive a transaction as we hope some day in<br>
the future to include changes to the index in a user transaction.<br>
<br>
Cheers,<br>
Sanne<br>
<div><div>_______________________________________________<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>