[#574] policer: Check the container has been really removed #640
No reviewers
Labels
No labels
P0
P1
P2
P3
badger
frostfs-adm
frostfs-cli
frostfs-ir
frostfs-lens
frostfs-node
good first issue
triage
Infrastructure
blocked
bug
config
discussion
documentation
duplicate
enhancement
go
help wanted
internal
invalid
kludge
observability
perfomance
question
refactoring
wontfix
No milestone
No project
No assignees
3 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: TrueCloudLab/frostfs-node#640
Loading…
Add table
Reference in a new issue
No description provided.
Delete branch "aarifullin/frostfs-node:fix/574-cnt_ever_existed"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Signed-off-by: Airat Arifullin a.arifullin@yadro.com
Please, rebase on master, tests failures should be fixed there.
@ -46,0 +46,4 @@
// SpecInfoProvider is the interface that provides methods to
// get container specific info or metadata.
type SpecInfoProvider interface {
DeletionInfo(cid.ID) (DelInfo, error)
To me it is a part of a
Source
interface, why have you decided to introduce a separate one?Because this affects many implementations of Source interface (some of them wrapped in wrapped wrappers) and for me
So, I tried to avoid harmful refactoring
Can we have 2 interface, one included in another? This should make it easier to accept just what you need.
Another option is to define the interface where we use it (so, tree and policer).
Alright. I will try then
@ -15,0 +31,4 @@
return c.DeletionInfo(binCnr)
}
func (c *Client) DeletionInfo(cid []byte) (containercore.DelInfo, error) {
Previously it returned a pointer, why this change?
@ -119,2 +121,4 @@
}
// WithContainerSpecInfo returns option to set container specific info provider for Policer.
func WithContainerSpecInfo(v container.SpecInfoProvider) Option {
We refactor and introduce new policer functionality in the same commit, can we split it?
[#574] policer: Check the container has been really removedto WIP: [#574] policer: Check the container has been really removedWIP: [#574] policer: Check the container has been really removedto [#574] policer: Check the container has been really removed[#574] policer: Check the container has been really removedto WIP: [#574] policer: Check the container has been really removed76405209f4
to9dcf70033e
9dcf70033e
tofe9b617e8f
WIP: [#574] policer: Check the container has been really removedto [#574] policer: Check the container has been really removedfe9b617e8f
to7aa67b383b
7aa67b383b
to1c0e9a1f09
@ -159,1 +166,3 @@
s.set(cnr, nil, new(apistatus.ContainerNotFound))
s.containerCache.set(cnr, nil, new(apistatus.ContainerNotFound))
// The removal causes the cache miss and thus deletion info (that contains
The removal invalidates possibly stored error response.
sounds better to me.@ -42,6 +42,7 @@ const (
NotificatorNotificatorStartProcessingObjectNotifications = "notificator: start processing object notifications"
NotificatorNotificatorProcessingObjectNotification = "notificator: processing object notification"
PolicerCouldNotGetContainer = "could not get container"
PolicerCouldNotCheckIfContainerEverExisted = "could not check if container ever existed"
What about
could not confirm container removal
? "ever existed" sounds vague to me -- it definitely existed if there is an object in our storage.Nice!
@ -0,0 +9,4 @@
// EverExisted checks whether the container ever existed or
// it just has not been created yet at the current epoch.
func EverExisted(s Source, cid cid.ID) (bool, error) {
Again, why not "WasRemoved"?
For me
EverExisted
makes he motivation for rechecking very understandable.If I name the method
WasRemoved
then is it really obvious that I need to deduce removed containers and then callWasRemoved
when it deduced they are removed? I prefer to keepEverExisted
.My point can seem confusing but, briefly, I want to make the rechecking more obvious and logical
But this is what you actually check: the method in contract is called
DeletionInfo
and, ironically,EverExisted
will return false if container exists but was not removed. I find this confusing.Alright :) Fixed to
WasRemoved
. I don't mindAlright :) Fixed to
WasRemoved
. I don't mind1c0e9a1f09
to06558507c6
06558507c6
toc166fa007c
@ -133,2 +134,3 @@
cachedContainerLister.update(cnr.Value.Owner(), ev.ID, true)
cachedContainerStorage.set(ev.ID, cnr, nil)
cachedContainerStorage.containerCache.set(ev.ID, cnr, nil)
cachedContainerStorage.delInfoCache.set(ev.ID, nil, new(apistatus.ContainerNotFound))
Containers are long-living entities, do we need to PUT sth here (vs just deleting or not touching at all)?
Alright, agree!
@ -15,0 +20,4 @@
DeletionInfo(cid []byte) (*containercore.DelInfo, error)
}
func AsContainerSpecInfoProvider(w *Client) containercore.Source {
The title should be adjusted (do we already have
AsContainerSource
?Sorry, my fault, bad refactoring
@ -39,0 +39,4 @@
return nil, new(apistatus.ContainerNotFound)
},
deletionInfo: func(id cid.ID) (*container.DelInfo, error) {
return &container.DelInfo{}, nil
Do we check the behaviour we introduce in this task, namely ContainerNotFound in both methods?
We don't but actually no good idea comes to my mind how I would check this case
This scenario when container is really removed can be tested by checking bury channel. In the case when container never existed in chain - nothing happens. Probably, to check the scenario I need to make some refactoring for
processObject
and put messages like (object won't be deleted
) and check them from the unit-test side. WDYT?Just check that bury channel is empty (maybe wait for 2 seconds)? I mean if there is no definite info, we do not expect anything there.
c166fa007c
to1c0540c529
@ -42,6 +42,7 @@ const (
NotificatorNotificatorStartProcessingObjectNotifications = "notificator: start processing object notifications"
NotificatorNotificatorProcessingObjectNotification = "notificator: processing object notification"
PolicerCouldNotGetContainer = "could not get container"
PolicerCouldNotConfirmContainerRemoval = "could not confirm container remova"
remova
- typofixed 👍
1c0540c529
to57dcd5a53c