fix: address more concurrency concerns in caching client #114
Conversation
aaron-steinfeld
requested review from
a team,
avinashkolluru,
satish-mittal and
tim-mwangi
Codecov Report
@@ Coverage Diff @@
## main #114 +/- ##
============================================
- Coverage 59.78% 59.77% -0.02%
Complexity 295 295
============================================
Files 39 39
Lines 2979 2978 -1
Branches 368 368
============================================
- Hits 1781 1780 -1
Misses 1026 1026
Partials 172 172
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
This comment has been minimized.
This comment has been minimized.
| // Acquire write lock to ensure no more modification of this update | ||
| Lock lock = pendingUpdateStripedLock.get(entityKey).writeLock(); | ||
| // Make sure no current additions | ||
| Lock lock = pendingUpdateStripedLock.get(entityKey); |
There was a problem hiding this comment.
L133 seems redundant since cache update is already done within L132. Can be removed.
There was a problem hiding this comment.
two different uses of the word cache.
Single<Entity> updateResult =
EntityDataCachingClient.this.createOrUpdateEntity(entityKey, condition).cache();
Caches in the Single - that is, all subscribers will share a result rather than re-executing
( http://reactivex.io/RxJava/3.x/javadoc/io/reactivex/rxjava3/core/Single.html#cache-- )
EntityDataCachingClient.this.cache.put(entityKey, updateResult);
puts the single itself into the guava cache for future callers
There was a problem hiding this comment.
Yes, but if we go inside:
private Single<Entity> createOrUpdateEntity(
EntityKey entityKey, UpsertCondition upsertCondition) {
Single<Entity> updateResult =
this.upsertEntityWithoutCaching(entityKey, upsertCondition).cache();
EntityDataCachingClient.this.cache.put(entityKey, updateResult);
return updateResult;
}
that is already adding the single within the guava cache, right?
There was a problem hiding this comment.
Thanks for following up on this, my mind is clearly all over the place 🙁 - addressed.
This comment has been minimized.
This comment has been minimized.
Description
Switching from a RW lock to mutual exclusion since adding updates is also not thread safe.