Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Container cache invalidation might not work exactly as we expect #1313

Closed
alexvanin opened this issue Apr 12, 2022 · 2 comments · Fixed by #1314
Closed

Container cache invalidation might not work exactly as we expect #1313

alexvanin opened this issue Apr 12, 2022 · 2 comments · Fixed by #1314
Assignees
@cthulhu-rider
Labels
question Further information is requested

Comments

@alexvanin
Copy link
Contributor

We use container cache to avoid extra RPCs to blockchain node. Containers can be removed and it leads to a cache invalidation, see:

neofs-node/cmd/neofs-node/container.go

Line 594 in 606dfa3

m.containers.InvalidateContainer(containerID)

Containers are not created or deleted immediately, but clients usually want to lock execution until operation is finished. To do that apps perform polling using container.Get (see pool package in SDK). Polling starts right after container.Delete execution which basically reverts invalidation.

Can we do something with it from the Storage side? I guess it would be really nice to avoid cache update of invalidated container for some time, but it looks like overhead, to be honest.

/cc @KirillovDenis
@alexvanin
Copy link
Contributor Author

alexvanin commented Apr 12, 2022 •
edited
Loading

I admit that there is no better way to handle than we have it now, but maybe someone has thoughts about this as well.

@cthulhu-rider
Copy link
Contributor

So now besides TX approval delay we have node's cache TTL delay. Specifically, I would consider the operation of deleting a container to be almost insensitive to delays.

@cthulhu-rider cthulhu-rider self-assigned this Apr 15, 2022
cthulhu-rider pushed a commit to cthulhu-rider/neofs-node that referenced this issue Apr 15, 2022
[nspcc-dev#1313] container: Do not invalidate cache on `Container.Del…
c6fb0c3 
…ete`

In previous implementation `Container.Delete` operation caused local
node's cache invalidation (container itself, eACL and listings). Any
subsequent `Container.Get` operation reversed invalidation. Given the
low latency sensitivity of deleting a container, there is no need to
touch the cache. With this approach, all pending deletion operations on
the node via the NeoFS API protocol will be delayed by the cache TTL.

Do not call cache invalidation ops in `morphContainerWriter.Delete`.
Remove no longer needed `InvalidateContainerListByCID` and
`InvalidateContainer` methods.

Signed-off-by: Leonard Lyubich <leonard@nspcc.ru>
cthulhu-rider pushed a commit to cthulhu-rider/neofs-node that referenced this issue Apr 15, 2022
[nspcc-dev#1313] container: Do not invalidate cache on `Container.Del…
7ad8c69 
…ete`

In previous implementation `Container.Delete` operation caused local
node's cache invalidation (container itself, eACL and listings). Any
subsequent `Container.Get` operation reversed invalidation. Given the
low latency sensitivity of deleting a container, there is no need to
touch the cache. With this approach, all pending deletion operations on
the node via the NeoFS API protocol will be delayed by the cache TTL.

Do not call cache invalidation ops in `morphContainerWriter.Delete`.
Remove no longer needed `InvalidateContainerListByCID` and
`InvalidateContainer` methods.

Signed-off-by: Leonard Lyubich <leonard@nspcc.ru>
@cthulhu-rider cthulhu-rider mentioned this issue Apr 25, 2022
Merged
cthulhu-rider pushed a commit that referenced this issue Apr 25, 2022
@cthulhu-rider
[#1313] container: Do not invalidate cache on Container.Delete
d456994 
In previous implementation `Container.Delete` operation caused local
node's cache invalidation (container itself, eACL and listings). Any
subsequent `Container.Get` operation reversed invalidation. Given the
low latency sensitivity of deleting a container, there is no need to
touch the cache. With this approach, all pending deletion operations on
the node via the NeoFS API protocol will be delayed by the cache TTL.

Do not call cache invalidation ops in `morphContainerWriter.Delete`.
Remove no longer needed `InvalidateContainerListByCID` and
`InvalidateContainer` methods.

Signed-off-by: Leonard Lyubich <leonard@nspcc.ru>
aprasolova pushed a commit to aprasolova/neofs-node that referenced this issue Oct 19, 2022
@aprasolova
[nspcc-dev#1313] container: Do not invalidate cache on `Container.Del…
ded24f4 
…ete`

In previous implementation `Container.Delete` operation caused local
node's cache invalidation (container itself, eACL and listings). Any
subsequent `Container.Get` operation reversed invalidation. Given the
low latency sensitivity of deleting a container, there is no need to
touch the cache. With this approach, all pending deletion operations on
the node via the NeoFS API protocol will be delayed by the cache TTL.

Do not call cache invalidation ops in `morphContainerWriter.Delete`.
Remove no longer needed `InvalidateContainerListByCID` and
`InvalidateContainer` methods.

Signed-off-by: Leonard Lyubich <leonard@nspcc.ru>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@cthulhu-rider cthulhu-rider

Labels
question Further information is requested
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

container: Do not invalidate cache on Container.Delete cthulhu-rider/neofs-node
2 participants
@alexvanin @cthulhu-rider