TrueCloudLab/frostfs-node
18
1
Fork 27
Code Issues 94 Pull requests 3 Projects Releases 1 Activity Actions

[#197] object: Make Delete method return correct status #433

Merged
fyrchik merged 1 commit from aarifullin/frostfs-node:fix/197-correct_delete_status into master 2023-06-30 12:58:58 +00:00
Conversation 0 Commits 1 Files changed 1 +10 -3
1 changed files with 10 additions and 3 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

13
pkg/services/object/delete/exec.go
View file

@ -8,6 +8,7 @@ import (
"git.frostfs.info/TrueCloudLab/frostfs-node/internal/logs"
"git.frostfs.info/TrueCloudLab/frostfs-node/pkg/services/object/util"
"git.frostfs.info/TrueCloudLab/frostfs-node/pkg/util/logger"
apiclient "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/client"
cid "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/container/id"
"git.frostfs.info/TrueCloudLab/frostfs-sdk-go/object"
oid "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/object/id"
@ -75,8 +76,9 @@ func (exec *execCtx) newAddress(id oid.ID) oid.Address {
}
func (exec *execCtx) formSplitInfo(ctx context.Context) bool {
var err error
success := false
var err error
exec.splitInfo, err = exec.svc.header.splitInfo(ctx, exec)
switch {
@ -87,12 +89,17 @@ func (exec *execCtx) formSplitInfo(ctx context.Context) bool {
exec.log.Debug(logs.DeleteCouldNotComposeSplitInfo,
zap.String("error", err.Error()),
)
case err == nil:
case err == nil, apiclient.IsErrObjectAlreadyRemoved(err):
dstepanov-yadro commented 2023-06-07 14:23:15 +00:00
Review
Copy link

Why deleting already marked for deletion object is success?
Marked for deletion object are the same as non existing objects i think.

Why deleting already marked for deletion object is success? Marked for deletion object are the same as non existing objects i think.
aarifullin commented 2023-06-08 14:59:02 +00:00
Review
Copy link

Please, check the issue :)

Briefly: because Delete should return only specified statutes

Please, check the issue :) Briefly: because `Delete` should return only [specified](https://git.frostfs.info/TrueCloudLab/frostfs-api/src/commit/fc393d4605e7e4c5545e882ef20d5dc171e25ce8/object/service.proto#L99-L109) statutes
dstepanov-yadro commented 2023-06-09 14:54:27 +00:00
Review
Copy link

Ok, but then we can fix case delete of non-existing object, because now

frostfs-cli object delete --cid <cid> --oid <some non existing object id>

returns

failed to get raw object header: read object header via client: status: code = 2049 message = object not found

Ok, but then we can fix case `delete of non-existing object`, because now `frostfs-cli object delete --cid <cid> --oid <some non existing object id>` returns `failed to get raw object header: read object header via client: status: code = 2049 message = object not found`
fyrchik commented 2023-06-13 07:22:51 +00:00
Review
Copy link

It is tricky I believe we should change API too: if we had not found an object, but it exists, we should not return OK. cc @alexvanin @realloc

It is tricky I believe we should change API too: if we had not found an object, but it exists, we should not return OK. cc @alexvanin @realloc
aarifullin commented 2023-06-13 08:29:15 +00:00
Review
Copy link

@dstepanov-yadro

Oh, really. I didn't know that but you're right - I checked on my own using CLI and api methods within unittest.

It seems it's easier to extend Delete specification (as @fyrchik suggested above) with two statuses and avoid tricks like in my PR

@dstepanov-yadro Oh, really. I didn't know that but you're right - I checked on my own using CLI and api methods within [unittest](https://git.frostfs.info/TrueCloudLab/frostfs-node/issues/197#issuecomment-11619). It seems it's easier to extend `Delete` specification (as @fyrchik suggested above) with two statuses and avoid tricks like in my PR
aarifullin commented 2023-06-14 08:30:57 +00:00
Review
Copy link

cc @alexvanin

cc @alexvanin
👍 1
aarifullin commented 2023-06-27 09:06:41 +00:00
Review
Copy link

So, we decided OBJECT_ALREADY_REMOVED cannot be returned by this function but have not decided yet should it return OBJECT_NOT_FOUND instead. I'd keep this PR like that - return success in the case of OBJECT_ALREADY_REMOVED error

So, we decided `OBJECT_ALREADY_REMOVED` cannot be returned by this function but have not decided yet should it return `OBJECT_NOT_FOUND` instead. I'd keep this PR like that - return success in the case of `OBJECT_ALREADY_REMOVED` error
// IsErrObjectAlreadyRemoved check is required because splitInfo
// implicitly performs Head request that may return ObjectAlreadyRemoved
// status that is not specified for Delete
exec.status = statusOK
exec.err = nil
success = true
}
return err == nil
return success
}
func (exec *execCtx) collectMembers(ctx context.Context) (ok bool) {