TrueCloudLab/frostfs-api
26
0
Fork 17
Code Issues 11 Pull requests 1 Releases Activity Actions

Introduce Patch rpc for object service #56

Merged
fyrchik merged 1 commit from aarifullin/frostfs-api:feat/patch/1 into master 2024-07-29 13:37:39 +00:00
Conversation 1 Commits 1 Files changed 2 +232 -1
aarifullin commented 2024-07-23 11:15:33 +00:00
Member
Copy link
  • Introduce rpc Patch and corresponding types;
  • Generate docs.
* Introduce rpc `Patch` and corresponding types; * Generate docs.
aarifullin added the
discussion
 label 2024-07-23 11:15:33 +00:00
aarifullin added 2 commits 2024-07-23 11:15:35 +00:00 
* Non-functional change.

Signed-off-by: Airat Arifullin <aarifullin@yadro.com>
* Introduce rpc `Patch` and corresponding types;
* Generate docs.

Signed-off-by: Airat Arifullin <aarifullin@yadro.com>

Signed-off-by: Airat Arifullin <aarifullin@yadro.com>
aarifullin force-pushed feat/patch/1 from 416212629b to c363a7e82d 2024-07-23 11:15:59 +00:00 Compare
aarifullin reviewed 2024-07-23 11:18:02 +00:00
object/service.proto Outdated
@ -817,3 +817,3 @@
neo.fs.v2.session.ResponseVerificationHeader verify_header = 3;
}
}
aarifullin commented 2024-07-23 11:18:01 +00:00
Author
Member
Copy link

This is unrelated change occured because there was no linefeed at the end of this file

This is unrelated change occured because there was no linefeed at the end of this file
aarifullin marked this conversation as resolved
aarifullin requested review from storage-core-committers 2024-07-23 11:18:37 +00:00
aarifullin requested review from storage-core-developers 2024-07-23 11:18:37 +00:00
aarifullin requested review from storage-services-developers 2024-07-23 11:18:37 +00:00
aarifullin requested review from storage-services-committers 2024-07-23 11:18:37 +00:00
fyrchik requested changes 2024-07-23 11:49:06 +00:00
fyrchik left a comment
Owner
Copy link
  1. I do not see the Patch RPC itself in the service definition, just request and response.
  2. The newline does not worth a separate commit (we do not have a linter in this repo, and all changes are still viewed as a single hunk.
  3. This is not the request scheme that was described in the RFC and that we have agreed upon.
1. I do not see the Patch RPC itself in the service definition, just request and response. 2. The newline does not worth a separate commit (we do not have a linter in this repo, and all changes are still viewed as a single hunk. 3. This is not the request scheme that was described in the RFC and that we have agreed upon.
aarifullin changed title from Introduce Patch rpc for object service to WIP: Introduce Patch rpc for object service 2024-07-23 12:27:06 +00:00
aarifullin force-pushed feat/patch/1 from c363a7e82d to f10d3162d6 2024-07-23 12:35:58 +00:00 Compare
aarifullin force-pushed feat/patch/1 from f10d3162d6 to 7739ed5e44 2024-07-23 12:40:10 +00:00 Compare
aarifullin force-pushed feat/patch/1 from 7739ed5e44 to 0d3acb33d0 2024-07-23 12:41:37 +00:00 Compare
aarifullin force-pushed feat/patch/1 from 0d3acb33d0 to c0f14a62ed 2024-07-23 12:45:02 +00:00 Compare
aarifullin force-pushed feat/patch/1 from c0f14a62ed to 6c1fbf753d 2024-07-23 13:03:33 +00:00 Compare
aarifullin changed title from WIP: Introduce Patch rpc for object service to Introduce Patch rpc for object service 2024-07-23 13:04:39 +00:00
aarifullin commented 2024-07-23 13:09:36 +00:00
Author
Member
Copy link
  1. I do not see the Patch RPC itself in the service definition, just request and response.

My bad. The result of unsuccessful stash. Fixed

  1. The newline does not worth a separate commit (we do not have a linter in this repo, and all changes are still viewed as a single hunk.

Removed this commit

  1. This is not the request scheme that was described in the RFC and that we have agreed upon.

Fixed. But I slightly changed something

  1. We need to properly address the object, so I've introduced address instead objectID within PatchRequest as we also need container ID
  2. I've introduced replace_attributes flag as overriding/merging attributes problem still remains and we could discuss this
  3. Also, I am wondering if need to add OwnerID field for the request body as it the object's ID can be changed
> 1. I do not see the Patch RPC itself in the service definition, just request and response. My bad. The result of unsuccessful stash. Fixed > 2. The newline does not worth a separate commit (we do not have a linter in this repo, and all changes are still viewed as a single hunk. Removed this commit > 3. This is not the request scheme that was described in the RFC and that we have agreed upon. Fixed. But I slightly changed something 1. We need to properly address the object, so I've introduced `address` instead `objectID` within `PatchRequest` as we also need container ID 2. I've introduced `replace_attributes` flag as overriding/merging attributes problem still remains and we could discuss this 3. Also, I am wondering if need to add `OwnerID` field for the request body as it the object's ID can be changed
fyrchik reviewed 2024-07-23 13:10:44 +00:00
object/service.proto Outdated
@ -286,0 +317,4 @@
// - **LOCKED** (2050, SECTION_OBJECT): \
// placement of an object of type TOMBSTONE that includes at least one
// locked object is prohibited;
// - **LOCK_NON_REGULAR_OBJECT** (2051, SECTION_OBJECT): \
fyrchik commented 2024-07-23 13:07:49 +00:00
Owner
Copy link

These statuses are explicit for PUT because we can lock and delete through it.
For patch it is an explicit non-goal.
I suggest failing with some error if we try to patch Tombstone or Lock objects.

These statuses are explicit for PUT because we can lock and delete through it. For patch it is an explicit non-goal. I suggest failing with some error if we try to patch Tombstone or Lock objects.
aarifullin commented 2024-07-23 13:58:43 +00:00
Author
Member
Copy link

Your point is right. Fixed

Your point is right. Fixed
aarifullin marked this conversation as resolved
object/service.proto Outdated
@ -820,0 +885,4 @@
//
// Default `false` value for this flag means the attributes will be just merged. If the incoming `new_attributes`
// list contains already existing key, then it just replaces it while merging the lists.
bool replace_attributes = 3;
fyrchik commented 2024-07-23 13:09:50 +00:00
Owner
Copy link

I suggest removing this from the initial implementation and replace by default.

I suggest removing this from the initial implementation and replace by default.
fyrchik commented 2024-07-23 13:10:30 +00:00
Owner
Copy link

@realloc @alexvanin do we have any specific reason to prefer attributes append over replacing?

If we provide nil here, we still will retain initial attributes.

@realloc @alexvanin do we have any specific reason to prefer attributes append over replacing? If we provide `nil` here, we still will retain initial attributes.
fyrchik commented 2024-07-23 13:26:11 +00:00
Owner
Copy link

Maybe update case is a sensible default, it has a nicer behaviour for nil (append nil = do nothing, compared to special logic with replace) and it can override a single attirbute (because attributes must be unique, so we have no choice).

Maybe `update` case is a sensible default, it has a nicer behaviour for `nil` (append nil = do nothing, compared to special logic with `replace`) and it can override a single attirbute (because attributes must be unique, so we have no choice).
fyrchik commented 2024-07-23 13:30:35 +00:00
Owner
Copy link

I am ok with this implementation as long as we not forget to update RFC

I am ok with this implementation as long as we not forget to update RFC
aarifullin commented 2024-07-23 13:52:38 +00:00
Author
Member
Copy link

it can override a single attirbute

I think keeping empty value for the attribute is OK until the client may misinterpret its value for empty but not for "unset"/"reset". If this is not a big problem, we can remove this flag away.

as we not forget to update RFC

We need to update anyway as the object can be addressed with neo.fs.v2.refs.Address address = 1 only

> it can override a single attirbute I think keeping empty value for the attribute is OK until the client may misinterpret its value for empty but not for "unset"/"reset". If this is not a big problem, we can remove this flag away. > as we not forget to update RFC We need to update anyway as the object can be addressed with `neo.fs.v2.refs.Address address = 1` only
alexvanin commented 2024-07-24 06:40:23 +00:00
Owner
Copy link

do we have any specific reason to prefer attributes append over replacing?

No, as long as we can provide nil and do not change headers at all.

I think keeping empty value for the attribute is OK until the client may misinterpret its value for empty but not for "unset"/"reset"

Can use optional field label among with repeated. Is it going to help?

> do we have any specific reason to prefer attributes append over replacing? No, as long as we can provide `nil` and do not change headers at all. > I think keeping empty value for the attribute is OK until the client may misinterpret its value for empty but not for "unset"/"reset" Can use `optional` field label among with `repeated`. Is it going to help?
fyrchik commented 2024-07-24 06:57:49 +00:00
Owner
Copy link

In proto3 all fields are optional

In proto3 all fields are optional
aarifullin commented 2024-07-24 07:17:32 +00:00
Author
Member
Copy link

Can use optional field label among with repeated. Is it going to help?

It's not :(

> Can use optional field label among with repeated. Is it going to help? It's not :(
aarifullin force-pushed feat/patch/1 from 6c1fbf753d to e6e3c20402 2024-07-23 13:57:37 +00:00 Compare
fyrchik approved these changes 2024-07-24 15:59:23 +00:00
dstepanov-yadro approved these changes 2024-07-29 13:00:13 +00:00
fyrchik merged commit 2efdc8fedb into master 2024-07-29 13:37:39 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
fyrchik
dstepanov-yadro
TrueCloudLab/storage-core-developers
TrueCloudLab/storage-services-developers
TrueCloudLab/storage-services-committers
Labels
Clear labels   
P0

Highest

  
P1

High

  
P2

Medium

  
P3

Low

  
good first issue

Good for newcomers

  
triage

Needs to be triaged

  
Infrastructure

Infrastructure, Automation, CI/CD or DevOps related stuff

  
blocked

The issue or PR is blocked by something

  
bug

Something is not working

  
config

Configuration format update or breaking change

  
discussion

Talking about something in order to reach a decision or to exchange ideas

  
documentation

Documentation related

  
duplicate

This issue or pull request already exists

  
enhancement

New feature

  
go

Language features adoption

  
help wanted

Extra attention is needed

  
internal

   
invalid

Something is wrong

  
kludge

This is a kludge and we know it

  
observability

Related to metrics, tracing and logs

  
perfomance

Perfomance related

  
question

More information is needed

  
refactoring

Refactoring

  
wontfix

This won't be fixed

No labels
P0
P1
P2
P3
good first issue
triage
Infrastructure
blocked
bug
config
discussion
documentation
duplicate
enhancement
go
help wanted
internal
invalid
kludge
observability
perfomance
question
refactoring
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
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-api#56
No description provided.
Delete branch "aarifullin/frostfs-api:feat/patch/1"

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?