Verify that ZKLock is not engaged on capacity clean#11131
Conversation
- Do not remove entry if it still has a zk lock from cache when cacheCapacity limit has been reached. - If the oldest entry in the cache can't be removed because it is locked. Find the next oldest entry and remove it - Add test that validates the fix Resolves: spring-projects#11113 **Auto-cherry-pick to 7.0.x & 6.5.x**
artembilan
left a comment
artembilan
left a comment
There was a problem hiding this comment.
Can we go with Fixes: sentence instead ?
Otherwise our Backport bot won’t create required issues:
I think silent eviction from the cache will lead to the same dead lock on ZK entry again. I mean we have just obtained but not locked. So, it got evicted. Then we lock a bit later and obtain the same key again and got dead lock.
We may consider to put into cache back on lock() call or so.
But feels more like we are fighting ghosts and walking in loops.
I would say let’s just leave cache logic without iterator removal.
And probably let’s change the default strategy for removal flag !
| if (!eldest.getValue().isAcquiredInThisProcess()) { | ||
| return true; | ||
| } | ||
| Iterator<Entry<String, ZkLock>> iterator = entrySet().iterator(); |
There was a problem hiding this comment.
I just assume that iterator returns entries from oldest first.
Therefore, the first next() gives us exactly the eldest entry we’ve just rejected.
And the next one: I’ll just believe you that we can call that remove() from the current remove logic.
- Remove override of bounded, now default bounded value is true
| boolean acquired = lock1.tryLock(10, TimeUnit.SECONDS); | ||
|
|
||
| // Fail the test if the lock was not acquired within the timeframe | ||
| assertThat(acquired).isTrue(); |
There was a problem hiding this comment.
I think this really should go to the try..finally.
And probably without any comments.
We also can have an assertion that lock1 is same as lock.
| // cache is full | ||
| lock.lock(); | ||
| try { | ||
| registry.obtain("test2"); |
There was a problem hiding this comment.
Let's add here comment like:
Although cache is full, this does not evict an old entry because it is locked yet
| public boolean bounded() { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Every class member should be surrounded with blank lines.
Much easier to ready the code and add new members.
- Update testLockEvictionWithAnotherLockOnTheKey comments to reflect eviction failure
|
|
||
| try { | ||
| assertThat(acquired).isTrue(); | ||
| assertThat(lock1).isEqualTo(lock); |
There was a problem hiding this comment.
Why equal but not same?
According to the cache purpose, the get has to return the same object.
- When the verifying duplicate locks
2 participants
Resolves: #11113
Auto-cherry-pick to 7.0.x & 6.5.x