Split head/get/range into separate functions #1616

fyrchik · 2025-01-28T12:08:41Z

fyrchik commented

2025-01-28 12:08:41 +00:00

This PR contains preliminary work for moving object assembling to a separate package and, eventually, to the SDK.

Decouple head+get+range logic into separate functions.
This helps us to simplify flow for everything.

I have intentionally left out this function, will do in a separate PR (sourceObjectID is wrong abstraction, we should split control flow immediately after processing split info).

func (a *assembler) initializeFromSourceObjectID(ctx context.Context, id oid.ID) (*oid.ID, []oid.ID, error) {

This PR contains preliminary work for moving object assembling to a separate package and, eventually, to the SDK. 1. Decouple head+get+range logic into separate functions. 2. This helps us to simplify flow for everything. I have intentionally left out this function, will do in a separate PR (`sourceObjectID` is wrong abstraction, we should split control flow immediately after processing split info). https://git.frostfs.info/TrueCloudLab/frostfs-node/src/commit/a788d4477342b92f7fa9c3bc9c34be8a9c9b45df/pkg/services/object/get/assembler.go#L123

requested reviews from storage-core-committers, storage-core-developers

2025-01-28 12:08:42 +00:00

fyrchik force-pushed refactor-get from 816f1489fa to 27eea1d086

2025-01-28 12:15:23 +00:00

Compare

fyrchik commented

2025-01-28 12:28:30 +00:00

The word clamp is taken from https://docs.rs/num/latest/num/fn.clamp.html
In range we try to "clamp" the range of the part to the range user has requested.

The word `clamp` is taken from https://docs.rs/num/latest/num/fn.clamp.html In range we try to "clamp" the range of the part to the range user has requested.

👀 1

elebedeva approved these changes 2025-01-28 12:55:55 +00:00

Dismissed

pkg/services/object/get/assembler_range.go Outdated

					
				@ -0,0 +59,4 @@

					// fill the chain end-to-start

					for hasPrev {

						// check that only for "range" requests,

						// for `GET` it stops via the false `withPrev`

elebedeva commented

2025-01-28 12:55:39 +00:00

What is "false withPrev"?

What is "false `withPrev`"?

fyrchik commented

2025-01-29 07:51:38 +00:00

Something we do not need any more, removed.

fyrchik force-pushed refactor-get from 27eea1d086 to f4b3178fc6

2025-01-29 08:25:08 +00:00

Compare

fyrchik dismissed elebedeva's review 2025-01-29 08:25:10 +00:00

Reason:

New commits pushed, approval review dismissed automatically according to repository settings

fyrchik force-pushed refactor-get from 599fb412b2 to 8d9a578db7

2025-01-29 09:29:02 +00:00

Compare

dstepanov-yadro approved these changes 2025-01-29 13:23:00 +00:00

a-savchuk reviewed 2025-01-29 13:48:58 +00:00

pkg/services/object/get/assembler.go

					
				@ -60,3 +61,4 @@

						return nil, objectSDK.NewSplitInfoError(a.splitInfo)

					}

					if len(childrenIDs) > 0 {

a-savchuk commented

2025-01-29 13:48:24 +00:00

How about something like this?

switch {
case len(childrenIDs) > 0 && rng != nil:
    err = a.assembleObjectByChildrenListRange(ctx, childrenIDs, writer)
case len(childrenIDs) > 0:
    err = a.assembleObjectByChildrenList(ctx, childrenIDs, writer)
case rng != nil:
    err = a.assemleObjectByPreviousIDInReverseRange(ctx, *previousID, writer)
default:
    err = a.assemleObjectByPreviousIDInReverse(ctx, *previousID, writer)
}

How about something like this? ```go switch { case len(childrenIDs) > 0 && rng != nil: err = a.assembleObjectByChildrenListRange(ctx, childrenIDs, writer) case len(childrenIDs) > 0: err = a.assembleObjectByChildrenList(ctx, childrenIDs, writer) case rng != nil: err = a.assemleObjectByPreviousIDInReverseRange(ctx, *previousID, writer) default: err = a.assemleObjectByPreviousIDInReverse(ctx, *previousID, writer) } ```

fyrchik commented

2025-01-29 15:34:20 +00:00

It definitely looks better, to my taste, but I will remove this in another PR anyway.
In this one, I tried to minimize diff (do not touch outer if).

It definitely looks better, to my taste, but I will remove this in another PR anyway. In this one, I tried to minimize diff (do not touch outer `if`).

👍 1

a-savchuk marked this conversation as resolved

a-savchuk approved these changes 2025-01-29 20:37:11 +00:00

fyrchik merged commit 57dc0a8e9e into master

2025-01-30 06:50:38 +00:00

fyrchik referenced this pull request from a commit

2025-01-30 06:50:40 +00:00

[#1616] getsvc: Move head assembling to a separate file

fyrchik referenced this pull request from a commit

2025-01-30 06:50:40 +00:00

[#1616] getsvc: Move range assembling to a separate file

fyrchik referenced this pull request from a commit