fix: updating indexes in a more consistent manner#7575
fix: updating indexes in a more consistent manner#7575shawkins wants to merge 2 commits intofabric8io:mainfrom
Conversation
shawkins
force-pushed
the
iss7265
branch
5 times, most recently
from
d0588ea to
994c740
Compare
csviri
left a comment
csviri
left a comment
There was a problem hiding this comment.
Not sure if following all the aspects but seems to solve the original issue.
Maybe could you elaborate the downside (if any) compared to alternative (ReadWriteLock version).
thank you!
| }); | ||
| } else { | ||
| values.computeIfAbsent(indexKey == null ? this : indexKey, k -> ConcurrentHashMap.newKeySet()).add(key); | ||
| values.compute(indexKey == null ? this : indexKey, (k, v) -> v == null ? new ConcurrentHashMap<>() : v).put(key, |
There was a problem hiding this comment.
Yes, in some situations users want the ability to lookup by null appearently.
The difference in the approaches comes down to CacheImpl.put. With a ReadWriteLock, a simultaneous read while put is called will block. Here the read will not block, and you are not guarenteed to see the new index state of the item being put (at least not until the updateIndices call is finished). In any other circumstance, the behavior is the same in the sense that if the cache update "happens before" the index read, then you get the same results. This is because if an item already exists in an index, there's no ephemal removal. And there's a consistency check that ensures if another revision of that resource already exists in the cache, that we won't inappropriately return something as belonging to the index. On a delete, the item will no longer be in the cache, and nothing will be returned for it. |
|
After reviewing both approaches, I think this is the right direction. The lock-free read path is important for performance, and the optimistic consistency approach addresses the core issue from #7265 (items temporarily disappearing during same-key updates) without introducing read contention. I've asked @Desel72 to scope down #7558 to just the concurrency reproducer test ( @shawkins could you bring this out of draft once we have the reproducer test to verify against? |
closes: fabric8io#7265 Signed-off-by: Steve Hawkins <shawkins@redhat.com>
Revert ReadWriteLock changes to CacheImpl, ProcessorStore, and ProcessorStoreTest. Remove CHANGELOG entry. Keep CacheImplConcurrencyTest as a disabled reproducer for fabric8io#7265, to be enabled once fabric8io#7575 lands.
…nconsistency (#7558) * fix(informer): use ReadWriteLock in CacheImpl to prevent index read inconsistency * fix: use specific import for ReentrantReadWriteLock in ProcessorStoreTest Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: refactor concurrency test to parameterized and add CHANGELOG entry - Replaced two nearly-identical test methods with a single parameterized test using @valuesource and a shared helper method - Added CHANGELOG entry for issue #7265 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: reduce PR to concurrency reproducer test with @disabled Revert ReadWriteLock changes to CacheImpl, ProcessorStore, and ProcessorStoreTest. Remove CHANGELOG entry. Keep CacheImplConcurrencyTest as a disabled reproducer for #7265, to be enabled once #7575 lands. * fix: assert thread completion in concurrency reproducer test Check return values of doneLatch.await() and executor.awaitTermination() so that a deadlock or hung thread fails the test instead of silently passing. --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
3 participants
closes: #7265
Description
Type of change
test, version modification, documentation, etc.)
Checklist