Refactor object building logic away from the GET service #124

Merged
fyrchik merged 1 commit from dstepanov-yadro/frostfs-node:feat/refactor_assemble into master 2023-07-26 21:07:56 +00:00

The logic for collecting LOB is moved to a separate file.

The logic for collecting LOB is moved to a separate file.
dstepanov-yadro changed title from Refactor object building logic away from the GET service to WIP: Refactor object building logic away from the GET service 2023-03-09 08:05:29 +00:00
dstepanov-yadro force-pushed feat/refactor_assemble from 729f27df68 to 34c7cfbd21 2023-03-09 08:27:54 +00:00 Compare
dstepanov-yadro changed title from WIP: Refactor object building logic away from the GET service to Refactor object building logic away from the GET service 2023-03-09 08:29:08 +00:00
dstepanov-yadro requested review from storage-core-committers 2023-03-09 08:29:23 +00:00
dstepanov-yadro requested review from storage-core-developers 2023-03-09 08:29:24 +00:00
carpawell requested changes 2023-03-09 17:56:24 +00:00
@ -0,0 +47,4 @@
a.log = a.log.With(
zap.Stringer("address", a.addr),
zap.Uint64("range offset", a.rng.GetOffset()),
zap.Uint64("range length", a.rng.GetLength()),
Contributor

use underscore for spaces? also, one commit adds that line with a typo while another fixes it, i think it can be added correctly without further fixes

in other words, the whole 1862169b75 and 5876473cf6 commits look redundant to me

use underscore for spaces? also, one commit adds that line with a typo while another fixes it, i think it can be added correctly without further fixes in other words, the whole https://git.frostfs.info/TrueCloudLab/frostfs-node/commit/1862169b75b6179497309872a4caba78f34c9e53 and https://git.frostfs.info/TrueCloudLab/frostfs-node/commit/5876473cf6505e4cb4b07ac27e2ebee2cdb6441f commits look redundant to me
Author
Member

done. also dropped one more.

done. also dropped one more.
carpawell marked this conversation as resolved
@ -0,0 +98,4 @@
}
func (a *assembler) initializeFromSourceObjectID(ctx context.Context, id oid.ID) (*oid.ID, []oid.ID, error) {
log := a.log.With(zap.Stringer("source_object_id", id))
Contributor

@fyrchik usually says me that With is too expensive in a hot code

@fyrchik usually says me that `With` is too expensive in a hot code
Author
Member

dropped. i think it depends.

dropped. i think it depends.
carpawell marked this conversation as resolved
@ -0,0 +108,4 @@
return nil, nil, err
}
//parent object is large object, not previuos!
Contributor

s#//parent#// parent

s#//parent#// parent
Contributor

oh, i see, you used that more than one time, so you write such comments on purpose?

oh, i see, you used that more than one time, so you write such comments on purpose?
Author
Member

this comment was just for me, dropped.

this comment was just for me, dropped.
Contributor

u added some more comments without spaces so i just wanted to clarify if it was done on purpose. in fact, i have never thought about restrictions about spaces and even thought that go's formater adds it by it self but seems like no but also seems like it is kinda common practice

u added some more comments without spaces so i just wanted to clarify if it was done on purpose. in fact, i have never thought about restrictions about spaces and even thought that go's formater adds it by it self but seems like [no](https://github.com/golang/go/issues/30540) but also seems like it is kinda common [practice](https://stackoverflow.com/questions/1467058/space-between-line-comment-characters-and-start-of-actual-comment)
Author
Member

I'm not strict with comments as I think code should be self-documenting. But еhanks, I'll keep that in mind in the future.

I'm not strict with comments as I think code should be self-documenting. But еhanks, I'll keep that in mind in the future.
carpawell marked this conversation as resolved
@ -0,0 +172,4 @@
if verifyIsChild && !a.isChild(obj) {
log.Warn("parent address in child object differs")
return nil, errors.New("parent address in child object differs")
Contributor

add a var for the err and reuse it in the tests?

add a var for the err and reuse it in the tests?
Author
Member

fixed

fixed
carpawell marked this conversation as resolved
dstepanov-yadro force-pushed feat/refactor_assemble from 34c7cfbd21 to 151b748bcc 2023-03-10 08:07:53 +00:00 Compare
dstepanov-yadro requested review from carpawell 2023-03-10 10:39:46 +00:00
fyrchik reviewed 2023-03-10 11:04:06 +00:00
@ -0,0 +24,4 @@
addr oid.Address
splitInfo *objectSDK.SplitInfo
rng *objectSDK.Range
log *zap.Logger
Owner

Later this should be moved to the SDK. Can we get rid of this logger?
SDK should be logger-agnostic.

I am not sure how we do this without losing observability, though.

Also, in many places we log an error which is also returned. I believe we should "consume" errors in one place: either log or return, here -- return.

Later this should be moved to the SDK. Can we get rid of this logger? SDK should be logger-agnostic. I am not sure how we do this without losing observability, though. Also, in many places we log an error _which is also returned_. I believe we should "consume" errors in one place: either log or return, here -- return.
Author
Member

fixed

fixed
fyrchik marked this conversation as resolved
acid-ant approved these changes 2023-03-13 06:26:06 +00:00
dstepanov-yadro force-pushed feat/refactor_assemble from 151b748bcc to 6a5349d381 2023-03-13 07:01:17 +00:00 Compare
dstepanov-yadro force-pushed feat/refactor_assemble from 6a5349d381 to 552ec79566 2023-03-13 07:19:16 +00:00 Compare
dstepanov-yadro requested review from fyrchik 2023-03-13 07:21:27 +00:00
dstepanov-yadro force-pushed feat/refactor_assemble from 552ec79566 to d11d2ac6d2 2023-03-13 07:25:26 +00:00 Compare
fyrchik reviewed 2023-03-14 06:47:39 +00:00
@ -143,0 +69,4 @@
exec.collectedObject = obj
case errors.As(err, new(*objectSDK.SplitInfoError)):
exec.status = statusVIRTUAL
exec.err = err
Owner

Previously we did more complex logic here and exec.err was created with objectSDK.NewSplitInfoError.
What is the reason for this change?

Previously we did more complex logic here and `exec.err` was created with `objectSDK.NewSplitInfoError`. What is the reason for this change?
Author
Member
For assemble() method we had no merge logic: https://git.frostfs.info/TrueCloudLab/frostfs-node/src/branch/master/pkg/services/object/get/exec.go#L213
fyrchik marked this conversation as resolved
@ -143,0 +70,4 @@
case errors.As(err, new(*objectSDK.SplitInfoError)):
exec.status = statusVIRTUAL
exec.err = err
case errors.As(err, new(*apistatus.ObjectOutOfRange)):
Owner

If you look closely, we had apistatus for local operations and *apistatus for remote.
Could you verify it continues to work now?

If you look closely, we had `apistatus` for local operations and `*apistatus` for remote. Could you verify it continues to work now?
Author
Member

fixed

fixed
fyrchik marked this conversation as resolved
@ -0,0 +75,4 @@
return sourceObjectID, true
}
sourceObjectID, ok = a.splitInfo.LastPart()
if !ok {
Owner

Is there any reason why this condition is inverted, compared to the previous condition in this function?

Is there any reason why this condition is inverted, compared to the previous condition in this function?
Author
Member

fixed

fixed
fyrchik marked this conversation as resolved
fyrchik reviewed 2023-03-14 07:58:29 +00:00
@ -0,0 +228,4 @@
func (a *assembler) buildChain(ctx context.Context, prevID oid.ID) ([]oid.ID, []objectSDK.Range, error) {
var (
chain = make([]oid.ID, 0)
Owner

While we are here, why not just nil?

While we are here, why not just `nil`?
Author
Member

just because nil is evil. fixed.

just because nil is evil. fixed.
fyrchik marked this conversation as resolved
dstepanov-yadro force-pushed feat/refactor_assemble from d11d2ac6d2 to 5cd86df1ef 2023-03-14 07:59:06 +00:00 Compare
dstepanov-yadro force-pushed feat/refactor_assemble from 5cd86df1ef to 5b25c800ac 2023-03-14 08:05:05 +00:00 Compare
fyrchik approved these changes 2023-03-15 05:47:30 +00:00
dstepanov-yadro force-pushed feat/refactor_assemble from 5b25c800ac to ac0a278a05 2023-03-15 06:20:11 +00:00 Compare
dstepanov-yadro requested review from storage-core-committers 2023-03-15 07:25:19 +00:00
fyrchik merged commit ac0a278a05 into master 2023-03-15 12:12:24 +00:00
Sign in to join this conversation.
No reviewers
TrueCloudLab/storage-core-developers
TrueCloudLab/storage-core-committers
No milestone
No project
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-node#124
No description provided.