Hi Sanne. Thanks for your comments. please see inline... Cheers, Pedro On 10/05/2013 09:15 PM, Sanne Grinovero wrote: > > Hi Pedro, > looks like you're diving in some good fun :-) > BTW please keep the dev discussions on the mailing list, adding it. > > inline : > > On 4 October 2013 22:01, Pedro Ruivo <pedro(a)infinispan.org&gt; wrote: >> >> Hi, >> >> Sanne I need your expertise in here. I'm afraid that the problem is in >> FileListOperations :( >> I think the FileListOperations implementation needs a transactional cache >> with strong consistency... >> >> I'm 99% sure that it is originating the java.lang.AssertionError: file >> XPTO >> does not exist. I find out that we have multiple threads adding and >> removing >> files from the list. The scenario in [1] we see 2 threads loading the key >> from the cache loader and one thread adds a file and other removes. the >> thread that removes is the last one to commit and the file list is >> updated >> to an old state. When it tries to updat an index, I got the assertion >> error. > > > Nice, looks like you're on something. > I've never seen specifically an AssertionError, looks like you have a > new test. Could you share it? yes of course: https://github.com/pruivo/infinispan/blob/a4483d08b92d301350823c7fd42725c... so far, only the tests with eviction are failing...

> > Let's step back a second and consider the Cache usage from the point > of view of FileListOperations. > Note that even if you have two threads writing at the same time, as > long as they are on the same node they will be adding/removing > elements from the same instance of a ConcurrentHashMap. > Since it's the same instance, it doesn't matter which thread will do > the put operation as last: it will push the correct state. > (there is an assumptions here, but we can forget about those for the > sake of this debugging: same node -> fine as there is an external > lock, no other node is allowed to write at the same time) > 100% agreed with you but with cache store, we no longer ensure that 2 (or more) threads are pointing to the same instance of Concurrent Hash Set. With eviction, the entries are removed from in-memory container and persisted in the cache store. The scenario I've described, 2 threads are trying to add/remove a file and the file list does not exist in-memory. So, each thread will read from cache store and deserialize the byte array. In the end, each thread will have a pointer for different instances of ConcurrentHashSet but with the same elements. And when this happens, we lost one of the operation.

Also, the problem is easily reproduce when you enable the storeAsBinary for values because each cache.get will deserialize the byte array and create different instances.

That's why I think we would need a transaction.

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

> > Also, the problem is easily reproduce when you enable the storeAsBinary for > values because each cache.get will deserialize the byte array and create > different instances. That's another option that I've never used on a cache meant to store a Lucene index as it doesn't make sense for the purpose. But you're right that's an assumption I haven't thought of: # Action 4: What do you think of promoting that as an illegal configuration? created ISPN-3593 > > That's why I think we would need a transaction. -1 the Directory can not drive a transaction as we hope some day in the future to include changes to the index in a user transaction. Cheers, Sanne _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org <mailto:infinispan-dev@lists.jboss.org> https://lists.jboss.org/mailman/listinfo/infinispan-dev

On 6 October 2013 00:01, Pedro Ruivo <pedro(a)infinispan.org&gt; wrote: > Hi Sanne. > > Thanks for your comments. please see inline... > > Cheers, > Pedro > > > On 10/05/2013 09:15 PM, Sanne Grinovero wrote: >> >> Hi Pedro, >> looks like you're diving in some good fun :-) >> BTW please keep the dev discussions on the mailing list, adding it. >> >> inline : >> >> On 4 October 2013 22:01, Pedro Ruivo <pedro(a)infinispan.org&gt; wrote: >>> >>> Hi, >>> >>> Sanne I need your expertise in here. I'm afraid that the problem is in >>> FileListOperations :( >>> I think the FileListOperations implementation needs a transactional cache >>> with strong consistency... >>> >>> I'm 99% sure that it is originating the java.lang.AssertionError: file >>> XPTO >>> does not exist. I find out that we have multiple threads adding and >>> removing >>> files from the list. The scenario in [1] we see 2 threads loading the key >>> from the cache loader and one thread adds a file and other removes. the >>> thread that removes is the last one to commit and the file list is >>> updated >>> to an old state. When it tries to updat an index, I got the assertion >>> error. >> >> >> Nice, looks like you're on something. >> I've never seen specifically an AssertionError, looks like you have a >> new test. Could you share it? > > > yes of course: > https://github.com/pruivo/infinispan/blob/a4483d08b92d301350823c7fd42725c... > > so far, only the tests with eviction are failing... Some of the failures you're seeing are caused by internal "assert" keyword in Lucene's code, which have the purpose of verifying the "filesystem" is going to be synched properly. These assertions don't apply when using our storage: we don't need this synch to happen: in fact if it weren't because of the assertions the whole method would be a no-op as it finally delegates all logic to a method in the Infinispan Directory which is a no-op. In other words, these are misleading failures and we'd need to avoid the TestNG "feature" of enabling assertions in this case. Still, even if the stacktrace is misleading, I agree on your diagnosis below. Could you reproduce the problem without involving also the Query framework?

I'd hope that such a test could be independent and live solely in the lucene-directory module; in practice if you can only reproduce it with the query module it makes me suspicious that we're actually debugging a race condition in the initialization of the two services: a race between the query initialization thread needing to check the index state (so potentially triggering a load from cachestore), and the thread performing the cachestore preload. (I see your test also fails without preload, but just wondering if that might be an additional complexity).

>> >> Let's step back a second and consider the Cache usage from the point >> of view of FileListOperations. >> Note that even if you have two threads writing at the same time, as >> long as they are on the same node they will be adding/removing >> elements from the same instance of a ConcurrentHashMap. >> Since it's the same instance, it doesn't matter which thread will do >> the put operation as last: it will push the correct state. >> (there is an assumptions here, but we can forget about those for the >> sake of this debugging: same node -> fine as there is an external >> lock, no other node is allowed to write at the same time) >> > > 100% agreed with you but with cache store, we no longer ensure that 2 (or > more) threads are pointing to the same instance of Concurrent Hash Set. > > With eviction, the entries are removed from in-memory container and > persisted in the cache store. The scenario I've described, 2 threads are > trying to add/remove a file and the file list does not exist in-memory. So, > each thread will read from cache store and deserialize the byte array. In > the end, each thread will have a pointer for different instances of > ConcurrentHashSet but with the same elements. And when this happens, we lost > one of the operation. I'm seeing more than a couple of different smelly behaviors interacting here: 1## The single instance ConcurrentHashSet I guess this could be re-thought as it's making some strong assumptions, but considering this service can't be transactional I'd rather explore other solutions first as I think the following changes should be good enough. 2## Eviction not picking the right entry This single key is literally read for each and every performed query, and all writes as well. Each write, will write on this key. Even with eviction being enabled on the cache, I would never expect this key to be actually evicted! # Action 1: Open an issue to investigate the eviction choice: the strategy seems to be making a very poor job (or maybe it's just that maxEntries(10) is too low and makes LIRS degenerate into insane choices).

# Action 2: I think that for now we could disallow usage of eviction on the metadata cache. I didn't have tests using it, as I wouldn't recommended such a configuration as these entries are very hot and very small: viable to make it an illegal option?

3## The CacheLoader loading the same entry multiple times in parallel Kudos for finding out that there are situations in which we deal with multiple different instances of ConcurrentHashSet! Still, I think that Infinispan core is terribly wrong in this case: from the client code POV a new CHS is created with a put-if-absent atomic operation, and I will assume there that core will check/load any enabled cachestore as well. To handle multiple GET operations in parallel, or even in parallel with preload or the client's put-if-absent operation, I would *not* expect Infinispan core to storm the CacheStore implementation with multiple load operations on the same put: a lock should be hold on the potential key during such a load operation.

If your observation is right, this could also be one of the reasons for so many complaints on the CacheStore performance: under these circumstances - which I'd guesstimate are quite common - we're doing lots of unnecessary IO, potentially stressing the slower storage devices. This could even have dramatic effects if there are frequent requests for entries for which the returned value is a null. # Action 3: Investigate and open a JIRA about the missing locking on CacheStore load operations. If this where resolves, we would still have a guarantee of single instance, am I correct?

> > Also, the problem is easily reproduce when you enable the storeAsBinary for > values because each cache.get will deserialize the byte array and create > different instances. That's another option that I've never used on a cache meant to store a Lucene index as it doesn't make sense for the purpose. But you're right that's an assumption I haven't thought of: # Action 4: What do you think of promoting that as an illegal configuration? created ISPN-3593

> > That's why I think we would need a transaction. -1 the Directory can not drive a transaction as we hope some day in the future to include changes to the index in a user transaction.

Cheers, Sanne

Proposal: let's be more aggressive on validation. - make preload mandatory (for the metadata only) - eviction not allowed (for the metadata only) Note that I don't think we're putting much of a restriction to users, we're more likely helping with good guidance on how these caches are supposed to work. I think it's a good thing to narrow down the possible configuration options and make it clear what we expect people to use for this purpose.

>> 3## The CacheLoader loading the same entry multiple times in parallel >> Kudos for finding out that there are situations in which we deal with >> multiple different instances of ConcurrentHashSet! Still, I think that >> Infinispan core is terribly wrong in this case: >> from the client code POV a new CHS is created with a put-if-absent >> atomic operation, and I will assume there that core will check/load >> any enabled cachestore as well. >> To handle multiple GET operations in parallel, or even in parallel >> with preload or the client's put-if-absent operation, I would *not* >> expect Infinispan core to storm the CacheStore implementation with >> multiple load operations on the same put: a lock should be hold on the >> potential key during such a load operation. > > > I didn't understand what you mean here ^^. Infinispan only tries to load > from a cache store once per operation (get, put, remove, etc...). > > however, I think we have a big windows in which multiple operations can load > the same key from the store. This happens because we only write to the data > container after the operation end. I think that's a very important consideration. Imagine a REST cache is built on Infinispan, using a filesystem based CacheStore, and we receive a million requests per second for a key "X1", which doesn't exist. Since each request is not going to find it in the data container, each request is triggering a CacheStore load. Do you want to enqueue a million IO operations to the disk? I'd much rather have some locking in memory, that will prevent nut just disks from struggling but also allow the CPUs to context switch to perform more useful tasks: being blocked on a lock is a good thing in this case, rather than allowing "parallel processing of pointless requests". That's just parallel energy burning, and potentially DOSing yourself regarding more interesting requests which could be handled instead. To translate it into an implementation design, you need to lock the "X1" key before attempting to load it from the CacheStore, and when the lock is acquired the data container needs to be re-checked as it might have been loaded by a parallel thread. Or alternatively, lock the key before attempting to read from the data container: that's not too bad, it's a very brief local only lock.

Worth a different thread? Not that I don't know how Infinispan core is implementing this today, I'm just understanding from you that it's not as good as I'd expected it. This might also be the reason for some people to have recorded a very bad performance when writing stress tests on the CacheStore, and of course such a benchmark would highlight too much IO going on.. but it's not how fast we do the IO which is the problem if we can just avoid some of them. > > >> >> If your observation is right, this could also be one of the reasons >> for so many complaints on the CacheStore performance: under these >> circumstances - which I'd guesstimate are quite common - we're doing >> lots of unnecessary IO, potentially stressing the slower storage >> devices. This could even have dramatic effects if there are frequent >> requests for entries for which the returned value is a null. >> >> # Action 3: Investigate and open a JIRA about the missing locking on >> CacheStore load operations. >> >> If this where resolves, we would still have a guarantee of single >> instance, am I correct? >> > > cache stores acquire the read lock to load a key. I'm not sure if exclusive > locking a key would help in anything. See above: allowing parallelism on loads is not a good idea as it will storm the CacheStore with redundant requests. Sanne

On 7 October 2013 13:20, Pedro Ruivo <pedro(a)infinispan.org&gt; wrote: > > > On 10/07/2013 11:43 AM, Sanne Grinovero wrote: >> >> >> Proposal: >> let's be more aggressive on validation. >> - make preload mandatory (for the metadata only) >> - eviction not allowed (for the metadata only) >> >> Note that I don't think we're putting much of a restriction to users, >> we're more likely helping >> with good guidance on how these caches are supposed to work. >> I think it's a good thing to narrow down the possible configuration >> options >> and make it clear what we expect people to use for this purpose. > > > it's good enough for me. :) > > >> >> >>>> 3## The CacheLoader loading the same entry multiple times in parallel >>>> Kudos for finding out that there are situations in which we deal with >>>> multiple different instances of ConcurrentHashSet! Still, I think that >>>> Infinispan core is terribly wrong in this case: >>>> from the client code POV a new CHS is created with a put-if-absent >>>> atomic operation, and I will assume there that core will check/load >>>> any enabled cachestore as well. >>>> To handle multiple GET operations in parallel, or even in parallel >>>> with preload or the client's put-if-absent operation, I would *not* >>>> expect Infinispan core to storm the CacheStore implementation with >>>> multiple load operations on the same put: a lock should be hold on the >>>> potential key during such a load operation. >>> >>> >>> >>> I didn't understand what you mean here ^^. Infinispan only tries to load >>> from a cache store once per operation (get, put, remove, etc...). >>> >>> however, I think we have a big windows in which multiple operations can >>> load >>> the same key from the store. This happens because we only write to the >>> data >>> container after the operation end. >> >> >> I think that's a very important consideration. >> >> Imagine a REST cache is built on Infinispan, using a filesystem based >> CacheStore, and we receive a million requests per second >> for a key "X1", which doesn't exist. >> Since each request is not going to find it in the data container, each >> request is triggering a CacheStore load. >> Do you want to enqueue a million IO operations to the disk? >> I'd much rather have some locking in memory, that will prevent nut >> just disks from struggling but also allow the CPUs to context switch >> to perform more useful tasks: being blocked on a lock is a good thing >> in this case, rather than allowing "parallel processing of pointless >> requests". That's just parallel energy burning, and potentially DOSing >> yourself regarding more interesting requests which could be handled >> instead. >> >> To translate it into an implementation design, you need to lock the >> "X1" key before attempting to load it from the CacheStore, and when >> the lock is acquired the data container needs to be re-checked as it >> might have been loaded by a parallel thread. >> Or alternatively, lock the key before attempting to read from the data >> container: that's not too bad, it's a very brief local only lock. > > > you have good point here, but IMO this is a high risk operation because we > are getting close in the final release. and I don't want to make the cache > store unusable... I understand there is risk, but this should have been done long ago. AFAIR one of the goals for 6.0 is to improve performance of CacheStores, this is one of the many changes going in that direction. Besides, I wouldn't be too worried as the CacheStores API is entirely new and most people will only provide feedback after the Final release, so this is not an entirely unfortunate timing. > > but, let's see what others think about it. > > my 1 cent pseudo-code suggestion: > > lock(key) { > value = dataContainer.get(key) > if value is not null; then return value > loaded = cacheLoader.load(key) > if loaded is null; then return null > existing = dataContainer.putIfAbsent(key, loaded) > if existing is null; then return loaded; otherwise existing > > } +1 for something similar. Not sure why you need a putIfAbsent since you have the lock, but that's a detail.

An alternative with a lockfree fast-path: value = dataContainer.get(key) if (value!=null) return value lock(key) { value = dataContainer.get(key) if value is not null; then return value loaded = cacheLoader.load(key) if loaded is null; then return null existing = dataContainer.put(key, loaded) return loaded }

Wouldn't be much of a benefit for the example I made above until we switch to using a placeholder token (tombstone) for null values, but there is great value in this pattern for existing entries. Sanne