Add PutSingle RPC #31

Merged
fyrchik merged 1 commit from dstepanov-yadro/frostfs-api:feat/put-single into master 2023-07-04 08:28:45 +00:00

In #9 decided to create new Put method to save prepared object.

In https://git.frostfs.info/TrueCloudLab/frostfs-api/issues/9 decided to create new Put method to save prepared object.
fyrchik reviewed 2023-06-29 10:16:24 +00:00
@ -709,0 +746,4 @@
// PUT Single request body
message Body {
// Object ID
neo.fs.v2.refs.ObjectID object_id = 1;
Owner

So, basically, Object + copies_number?
Why do we have 4 separate fields instead of an Object?

So, basically, `Object` + `copies_number`? Why do we have 4 separate fields instead of an `Object`?
Author
Member

fixed

fixed
dstepanov-yadro force-pushed feat/put-single from 40f630e0b6 to 656253d8c1 2023-06-29 14:38:52 +00:00 Compare
dstepanov-yadro force-pushed feat/put-single from 656253d8c1 to d6bb998e3a 2023-06-29 14:45:13 +00:00 Compare
dstepanov-yadro changed title from WIP: Add PutSingle RPC to Add PutSingle RPC 2023-06-30 06:44:47 +00:00
dstepanov-yadro requested review from storage-core-committers 2023-06-30 06:44:54 +00:00
dstepanov-yadro requested review from storage-core-developers 2023-06-30 06:44:55 +00:00
ale64bit approved these changes 2023-06-30 12:08:57 +00:00
@ -229,2 +229,4 @@
// provided session token has expired.
rpc GetRangeHash(GetRangeHashRequest) returns (GetRangeHashResponse);
// Put the prepared object into container. Request uses gRPC stream.
Member

Request uses gRPC stream.

This is a bit misleading; the RPC is unary.

> Request uses gRPC stream. This is a bit misleading; the RPC is unary.
Author
Member

fixed

fixed
ale64bit marked this conversation as resolved
@ -231,0 +231,4 @@
// Put the prepared object into container. Request uses gRPC stream.
// `ContainerID`, `ObjectID` and `OwnerID` of an object
// SHOULD be set. Session token SHOULD be obtained before `PUT SINGLE` operation (see
Member

nit: not sure what kind of rules we have, but I would rather use MUST instead of SHOULD for these things. SHOULD sounds like it's optional.

nit: not sure what kind of rules we have, but I would rather use `MUST` instead of `SHOULD` for these things. `SHOULD` sounds like it's optional.
Author
Member

fixed

fixed
ale64bit marked this conversation as resolved
@ -231,0 +237,4 @@
// Extended headers can change `Put` behaviour:
// * [ __SYSTEM__NETMAP_EPOCH \
// (`__NEOFS__NETMAP_EPOCH` is deprecated) \
// Will use the requsted version of Network Map for object placement
Member

s/requsted/requested

s/requsted/requested
Author
Member

fixed

fixed
ale64bit marked this conversation as resolved
@ -707,2 +741,4 @@
neo.fs.v2.session.ResponseVerificationHeader verify_header = 3;
}
// Object PUT Single request
Member

many of these comments seem superfluous. They don't add any valuable info to the declaration itself.

many of these comments seem superfluous. They don't add any valuable info to the declaration itself.
Author
Member

Well, i think, this is a tradition. And traditions should be respected.

Well, i think, this is a tradition. And traditions should be respected.
Owner

The comments could be useful for doc generation.
Agree with @dstepanov-yadro , let's preserve the style.
Could make alterations altogether in a separate task.

The comments could be useful for doc generation. Agree with @dstepanov-yadro , let's preserve the style. Could make alterations altogether in a separate task.
dstepanov-yadro force-pushed feat/put-single from d6bb998e3a to ebbf395228 2023-06-30 12:28:56 +00:00 Compare
dstepanov-yadro reviewed 2023-06-30 12:35:14 +00:00
@ -709,0 +771,4 @@
// Object PUT Single response
message PutSingleResponse {
// PUT Single Object response body
message Body {
Author
Member

PutSingle requires object id passed in the request. So there is nothing to return, but Body is defined like in other models.

PutSingle requires object id passed in the request. So there is nothing to return, but `Body` is defined like in other models.
fyrchik approved these changes 2023-06-30 12:49:02 +00:00
fyrchik requested review from realloc 2023-06-30 12:49:09 +00:00
fyrchik requested review from alexvanin 2023-06-30 12:49:09 +00:00
acid-ant approved these changes 2023-07-01 13:14:41 +00:00
dstepanov-yadro force-pushed feat/put-single from ebbf395228 to c3be3c160a 2023-07-03 08:41:46 +00:00 Compare
alexvanin reviewed 2023-07-03 08:51:21 +00:00
@ -231,0 +259,4 @@
// (for trusted object preparation) session private key does not exist or has
// been deleted;
// - **TOKEN_EXPIRED** (4097, SECTION_SESSION): \
// provided session token has expired.
Owner

Token failure codes are not relevant now.

Token failure codes are not relevant now.
Author
Member
frostfs-node checks session token for every request: https://git.frostfs.info/TrueCloudLab/frostfs-node/src/branch/master/pkg/services/object/acl/v2/service.go#L243
alexvanin marked this conversation as resolved
dstepanov-yadro force-pushed feat/put-single from c3be3c160a to 5aaa89dbf5 2023-07-03 13:59:36 +00:00 Compare
dstepanov-yadro reviewed 2023-07-03 14:00:54 +00:00
@ -230,1 +230,4 @@
rpc GetRangeHash(GetRangeHashRequest) returns (GetRangeHashResponse);
// Put the prepared object into container.
// `ContainerID`, `ObjectID`, `OwnerID`, `PayloadHash` of an object MUST be set.
Author
Member

PayloadHash is also required.

PayloadSize is required for streaming Put, because we need to check planned and actual size, so PutSinle doesn't require this field i think.

PayloadHash is also required. PayloadSize is required for streaming Put, because we need to check planned and actual size, so PutSinle doesn't require this field i think.
Owner

I believe it requires -- this is a part of correctly formed object.

I believe it requires -- this is a part of correctly formed object.
Author
Member

Ok, added Payload len to required

Ok, added Payload len to required
alexvanin approved these changes 2023-07-03 14:42:32 +00:00
fyrchik approved these changes 2023-07-03 14:57:21 +00:00
dstepanov-yadro force-pushed feat/put-single from 5aaa89dbf5 to 46fcaf9e21 2023-07-03 15:08:48 +00:00 Compare
realloc approved these changes 2023-07-04 08:04:43 +00:00
realloc requested review from fyrchik 2023-07-04 08:05:00 +00:00
realloc requested review from alexvanin 2023-07-04 08:05:08 +00:00
dstepanov-yadro force-pushed feat/put-single from 46fcaf9e21 to f2a60016ab 2023-07-04 08:08:53 +00:00 Compare
fyrchik merged commit f2a60016ab into master 2023-07-04 08:28:45 +00:00
Sign in to join this conversation.
No milestone
No project
No assignees
6 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#31
No description provided.