Allow to split objects in the client #94

Merged
fyrchik merged 2 commits from acid-ant/frostfs-sdk-go:feature/83-cut-obj-client into master 2023-06-28 12:12:39 +00:00
Member

Close #83

Signed-off-by: Anton Nikiforov an.nikiforov@yadro.com

Close #83 Signed-off-by: Anton Nikiforov <an.nikiforov@yadro.com>
acid-ant requested review from storage-core-committers 2023-06-19 07:36:44 +00:00
acid-ant requested review from storage-core-developers 2023-06-19 07:36:44 +00:00
fyrchik requested review from storage-services-committers 2023-06-19 08:10:25 +00:00
fyrchik requested review from storage-services-developers 2023-06-19 08:10:25 +00:00
aarifullin reviewed 2023-06-19 09:40:44 +00:00
@ -0,0 +21,4 @@
w.ctx, cancel = context.WithCancel(ctx)
stream, err := rpcapi.PutObject(&c.c, &w.respV2, client.WithContext(w.ctx))
if err != nil {
cancel()
Member

I know that is from previos ObjectPutInit implementation. But I was thinking: is it really necessary to invoke cancel() from inherited context when PutObject has already returned the error and w.ctx is not passed somewhere within objectPutInitRaw func?

I know that is from previos `ObjectPutInit` implementation. But I was thinking: is it really necessary to invoke `cancel()` from inherited context when `PutObject` has already returned the error and `w.ctx` is not passed somewhere within `objectPutInitRaw` func?
Author
Member

Updated, please review.

Updated, please review.
aarifullin approved these changes 2023-06-19 09:51:45 +00:00
dstepanov-yadro reviewed 2023-06-19 11:58:09 +00:00
@ -0,0 +57,4 @@
req v2object.PutRequest
partInit v2object.PutObjectPartInit
partChunk v2object.PutObjectPartChunk
ctx context.Context

Looks unused

Looks unused
Author
Member

Removed.

Removed.
dstepanov-yadro marked this conversation as resolved
@ -0,0 +37,4 @@
type objectWriterTransformer struct {
ot transformer.ObjectTarget
it internalTarget
ctx context.Context

I think ctx should be passed as an argument

I think ctx should be passed as an argument
Author
Member

Done.

Done.
dstepanov-yadro marked this conversation as resolved
alexvanin reviewed 2023-06-19 12:13:54 +00:00
@ -0,0 +28,4 @@
Key: key,
NextTargetInit: func() transformer.ObjectTarget { return &w.it },
MaxSize: prm.maxSize,
WithoutHomomorphicHash: true,
Owner

Shouldn't it be parameterized? We didn't abandon homomorphic hashes as far as I know.

Shouldn't it be parameterized? We didn't abandon homomorphic hashes as far as I know.
Author
Member

Definitely should, I've updated pr.

Definitely should, I've updated pr.
alexvanin marked this conversation as resolved
acid-ant force-pushed feature/83-cut-obj-client from 6f8f73317d to 9003259522 2023-06-19 19:47:35 +00:00 Compare
fyrchik reviewed 2023-06-20 06:50:38 +00:00
@ -23,0 +18,4 @@
// we simply assume that 3MB is large enough to reduce the
// number of messages, and not to exceed the limit
// (4MB by default for gRPC servers).
const MaxChunkLen = 3 << 20
Owner

Why is it exported?

Why is it exported?
Author
Member
The idea is to use it here, for example https://git.frostfs.info/TrueCloudLab/frostfs-node/src/branch/master/cmd/frostfs-cli/internal/client/client.go#L416
@ -77,0 +62,4 @@
type ObjectWriter interface {
// WriteHeader writes header of the object. Result means success.
// Failure reason can be received via Close.
WriteHeader(object.Object) bool
Owner

If we change it, may be use context.Context in signatures?

If we change it, may be use `context.Context` in signatures?
Author
Member

Updated, please review.

Updated, please review.
fyrchik marked this conversation as resolved
@ -237,1 +130,3 @@
return &x.res, nil
// WithEpoch specifies epoch for object when cut it on client side.
func (x *PrmObjectPutInit) WithEpoch(epoch uint64) {
x.epoch = epoch
Owner

I think the parameter should not be obligatory, we could try netmap.Snapshot is epoch == 0.

I think the parameter should not be obligatory, we could try `netmap.Snapshot` is `epoch == 0`.
Author
Member

Updated, please review.

Updated, please review.
acid-ant force-pushed feature/83-cut-obj-client from 9003259522 to 73f9f4d4f4 2023-06-22 12:41:58 +00:00 Compare
Author
Member

Passing context.Context as argument for Write.../Close functions.

Passing `context.Context` as argument for `Write.../Close` functions.
dstepanov-yadro approved these changes 2023-06-23 11:59:28 +00:00
acid-ant changed title from Allow to cut objects in the client to Allow to split objects in the client 2023-06-26 07:13:12 +00:00
acid-ant force-pushed feature/83-cut-obj-client from 73f9f4d4f4 to 9e3f0c0017 2023-06-27 06:11:20 +00:00 Compare
fyrchik reviewed 2023-06-27 07:02:51 +00:00
@ -23,0 +19,4 @@
// we simply assume that 3MB is large enough to reduce the
// number of messages, and not to exceed the limit
// (4MB by default for gRPC servers).
const MaxChunkLen = 3 << 20
Owner

Is is needed after #96?

Is is needed after #96?
Author
Member

Think yes, just to avoid using some internal values from sdk across the code base in node.

Think yes, just to avoid using some internal values from sdk across the code base in node.
Owner

Ok, I guess we forgot to rename private field accordingly.
Can we name it similar to SetGRPCPayloadChunkLen, like DefaultGRPCPayloadChunkLen?

Ok, I guess we forgot to rename private field accordingly. Can we name it similar to `SetGRPCPayloadChunkLen`, like `DefaultGRPCPayloadChunkLen`?
Author
Member

Agree, I've renamed and reverted changes.

Agree, I've renamed and reverted changes.
fyrchik reviewed 2023-06-27 07:04:31 +00:00
@ -0,0 +46,4 @@
func (x *objectWriterTransformer) Close(ctx context.Context) (*ResObjectPut, error) {
if ai, err := x.ot.Close(ctx); err != nil {
return nil, err
} else {
Owner

This else is redundant (I wonder why linter does not complain)

This `else` is redundant (I wonder why linter does not complain)
Author
Member

The idea is to reduce the number of lines here. To remove else we need to define ai outside if.

The idea is to reduce the number of lines here. To remove `else` we need to define `ai` outside `if`.
fyrchik marked this conversation as resolved
acid-ant force-pushed feature/83-cut-obj-client from 9e3f0c0017 to 2a40c47398 2023-06-27 08:54:08 +00:00 Compare
Author
Member

Resolved conflicts.

Resolved conflicts.
acid-ant force-pushed feature/83-cut-obj-client from 2a40c47398 to 5df468f6e3 2023-06-27 10:53:59 +00:00 Compare
alexvanin reviewed 2023-06-27 11:31:32 +00:00
@ -89,0 +72,4 @@
WriteHeader(context.Context, object.Object) bool
// WritePayloadChunk writes chunk of the object payload. Result means success.
// Failure reason can be received via Close.
WritePayloadChunk(context.Context, []byte) bool
Owner

As far as I understand, this interface hides object transformation details and should be used as a replacement for current payload streaming approach, with the difference that object is created on the client side.

So objectWriter implementation is not suitable for more complex cases when client application wants to resend the object in case of transmission failures. Should client application use transformers directly in the code to do that and then use this interface to upload and reupload created object?

As far as I understand, this interface hides object transformation details and should be used as a replacement for current payload streaming approach, with the difference that object is created on the client side. So `objectWriter` implementation is not suitable for more complex cases when client application wants to resend the object in case of transmission failures. Should client application use transformers directly in the code to do that and _then_ use this interface to upload and reupload created object?
Author
Member

Thought about this, too. There is no mechanism to resend part of the already formed object. Currently, implemented happy path only. So client should transformer directly.
Do you think we need to add some retries when sending parts of the big object?

Thought about this, too. There is no mechanism to resend part of the already formed object. Currently, implemented happy path only. So client should `transformer` directly. Do you think we need to add some retries when sending parts of the big object?
Owner

Do you think we need to add some retries when sending parts of the big object?

Probably no. But client library will benefit from having an interface to put already prepared object, in my opinion.

> Do you think we need to add some retries when sending parts of the big object? Probably no. But client library will benefit from having an interface to put already prepared object, in my opinion.
Author
Member

You talk about a new method WriteObject which should wrap WriteHeader and WritePayloadChunk?
And it looks like raw writer only should implement it.

You talk about a new method `WriteObject` which should wrap `WriteHeader` and `WritePayloadChunk`? And it looks like raw writer only should implement it.
Owner

Oh yes. I see there is an separate issue for that #64, so we can postpone it. Also we are going to have one more RPC to upload whole object in one request: TrueCloudLab/frostfs-api#9.

Oh yes. I see there is an separate issue for that https://git.frostfs.info/TrueCloudLab/frostfs-sdk-go/issues/64, so we can postpone it. Also we are going to have one more RPC to upload whole object in one request: https://git.frostfs.info/TrueCloudLab/frostfs-api/issues/9.
alexvanin marked this conversation as resolved
fyrchik approved these changes 2023-06-28 07:41:13 +00:00
alexvanin approved these changes 2023-06-28 11:14:17 +00:00
fyrchik merged commit 2f88460172 into master 2023-06-28 12:12:39 +00:00
Sign in to join this conversation.
No reviewers
TrueCloudLab/storage-services-developers
No milestone
No project
No assignees
5 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-sdk-go#94
No description provided.