Invalidate container cache on PutSuccess event #272

Merged
fyrchik merged 1 commit from dstepanov-yadro/frostfs-node:fix/container_not_found into master 2023-07-26 21:07:57 +00:00

For example: frostfs-cli creates container and makes polling GetContainer requests. These requests go through container cache, so not found error stores in container cache. So container cache can contain not found error when PutSuccess event received.

Closes

For example: frostfs-cli creates container and makes polling GetContainer requests. These requests go through container cache, so not found error stores in container cache. So container cache can contain not found error when PutSuccess event received. Closes #231
dstepanov-yadro requested review from storage-core-committers 2023-04-20 14:31:25 +00:00
dstepanov-yadro requested review from storage-core-developers 2023-04-20 14:31:25 +00:00
Owner

Could you mention Close #231 in the PR message?

Could you mention `Close #231` in the PR message?
fyrchik approved these changes 2023-04-21 06:51:50 +00:00
@ -135,2 +134,4 @@
cnr, err := cnrSrc.Get(ev.ID)
if err == nil {
cachedContainerLister.update(cnr.Value.Owner(), ev.ID, true)
cachedContainerStorage.set(ev.ID, cnr, nil)
Owner

I think we have a race-condition here, can this set happen before another NotFound result is in the process of being saved in cache?

I think we have a race-condition here, can this `set` happen before another `NotFound` result is in the process of being saved in cache?
Author
Member

Actually we have such race condition in TTL cache.

Actually we have such race condition in TTL cache.
Author
Member

Added lock

Added lock
fyrchik marked this conversation as resolved
Author
Member

Could you mention Close #231 in the PR message?

There are two PR's, so i close issue manualy

> Could you mention `Close #231` in the PR message? There are two PR's, so i close issue manualy
Owner

I believe only master PR affects issue state.

I believe only master PR affects issue state.
dstepanov-yadro force-pushed fix/container_not_found from fb0295c36b to df06fa862e 2023-04-21 07:59:43 +00:00 Compare
dstepanov-yadro force-pushed fix/container_not_found from df06fa862e to a600a5f4b4 2023-04-21 08:00:53 +00:00 Compare
fyrchik reviewed 2023-04-21 09:42:46 +00:00
@ -18,0 +36,4 @@
l.mtx.Lock()
if locker, found := l.lockers[key]; found {
locker.waiters++
Owner

Can we do this without locker mtx?

Can we do this without `locker` mtx?
Author
Member

l.mtx.Lock() protects

`l.mtx.Lock()` protects
Owner

Can we mention this in the comment to mtx or waiters field?

Can we mention this in the comment to `mtx` or `waiters` field?
Author
Member

hm, there are 8 lines of code only.

hm, there are 8 lines of code only.
Author
Member

comment added

comment added
Owner

It's for the kids!

It's for the kids!
fyrchik marked this conversation as resolved
@ -18,0 +56,4 @@
}
func (l *keyLock[K]) Unlock(key K) {
l.mtx.Lock()
Owner

I find it difficult to read and verify the absense of deadlocks: we have multiple mutexes and we call some Lock in Unlock, so the order of defer + Unlock() could matter in some cases.

I find it difficult to read and verify the absense of deadlocks: we have multiple mutexes and we call some `Lock` in `Unlock`, so the order of `defer + Unlock()` could matter in some cases.
Author
Member

Sorry, i do not know how to make this code more understandable. It seems like there are 10 lines of code and it should be simple. Therefore, I will be glad if you suggest options.

Sorry, i do not know how to make this code more understandable. It seems like there are 10 lines of code and it should be simple. Therefore, I will be glad if you suggest options.
@ -54,6 +115,9 @@ func newNetworkTTLCache[K comparable, V any](sz int, ttl time.Duration, netRdr n
//
// returned value should not be modified.
func (c *ttlNetCache[K, V]) get(key K) (V, error) {
c.keyLocker.Lock(key)
Owner

Do we need to block even if the valid item is in cache?

Do we need to block even if the valid item is in cache?
Author
Member

Fixed with two checks

Fixed with two checks
fyrchik marked this conversation as resolved
dstepanov-yadro force-pushed fix/container_not_found from a600a5f4b4 to 38974c4c94 2023-04-21 10:50:08 +00:00 Compare
dstepanov-yadro force-pushed fix/container_not_found from 38974c4c94 to 35214798d7 2023-04-21 11:04:16 +00:00 Compare
dstepanov-yadro force-pushed fix/container_not_found from 35214798d7 to 238cd3bfd9 2023-04-21 11:25:34 +00:00 Compare
acid-ant approved these changes 2023-04-21 13:08:29 +00:00
fyrchik requested review from carpawell 2023-04-24 09:44:12 +00:00
fyrchik approved these changes 2023-04-24 14:51:52 +00:00
@ -33,0 +74,4 @@
if locker.waiters == 1 {
delete(l.lockers, key)
}
locker.waiters--
Owner

Do this in the else branch? Or is it a deliberate decision? Don't mind either option.

Do this in the `else` branch? Or is it a deliberate decision? Don't mind either option.
Author
Member

It's deliberate decision. else is redundant.

It's deliberate decision. `else` is redundant.
fyrchik marked this conversation as resolved
carpawell approved these changes 2023-04-24 16:00:06 +00:00
dstepanov-yadro force-pushed fix/container_not_found from 238cd3bfd9 to 90fff809a5 2023-04-25 06:42:29 +00:00 Compare
dstepanov-yadro force-pushed fix/container_not_found from 90fff809a5 to 943819537a 2023-04-25 06:52:32 +00:00 Compare
dstepanov-yadro force-pushed fix/container_not_found from 943819537a to 04be9415d9 2023-04-25 06:54:23 +00:00 Compare
fyrchik merged commit 04be9415d9 into master 2023-04-25 12:36:11 +00:00
Sign in to join this conversation.
No reviewers
TrueCloudLab/storage-core-developers
No milestone
No project
No assignees
4 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: TrueCloudLab/frostfs-node#272
No description provided.