[#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
Member

Signed-off-by: Airat Arifullin a.arifullin@yadro.com

Signed-off-by: Airat Arifullin a.arifullin@yadro.com
aarifullin added the
enhancement
frostfs-node
labels 2023-06-07 10:10:12 +00:00
aarifullin requested review from storage-core-committers 2023-06-07 10:10:35 +00:00
aarifullin requested review from storage-core-developers 2023-06-07 10:10:36 +00:00
aarifullin force-pushed fix/197-correct_delete_status from 827789148b to bca9e93a08 2023-06-07 10:52:29 +00:00 Compare
dstepanov-yadro reviewed 2023-06-07 14:23:15 +00:00
@ -88,3 +90,3 @@
zap.String("error", err.Error()),
)
case err == nil:
case err == nil, apiclient.IsErrObjectAlreadyRemoved(err):

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.
Author
Member

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

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`
Owner

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
Author
Member

@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
Author
Member
cc @alexvanin
Author
Member

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
aarifullin changed title from [#197] object: Make Delete method return correct status to WIP: [#197] object: Make Delete method return correct status 2023-06-20 09:10:43 +00:00
aarifullin changed title from WIP: [#197] object: Make Delete method return correct status to [#197] object: Make Delete method return correct status 2023-06-27 09:05:42 +00:00
dstepanov-yadro approved these changes 2023-06-30 07:19:45 +00:00
fyrchik merged commit f437ab8f15 into master 2023-06-30 12:58:58 +00:00
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
3 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#433
No description provided.