Refactor object building logic away from the GET service #124
No reviewers
Labels
No labels
P0
P1
P2
P3
badger
frostfs-adm
frostfs-cli
frostfs-ir
frostfs-lens
frostfs-node
good first issue
triage
Infrastructure
blocked
bug
config
discussion
documentation
duplicate
enhancement
go
help wanted
internal
invalid
kludge
observability
perfomance
question
refactoring
wontfix
No milestone
No project
No assignees
4 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: TrueCloudLab/frostfs-node#124
Loading…
Reference in a new issue
No description provided.
Delete branch "dstepanov-yadro/frostfs-node:feat/refactor_assemble"
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?
The logic for collecting LOB is moved to a separate file.
Refactor object building logic away from the GET serviceto WIP: Refactor object building logic away from the GET service729f27df68
to34c7cfbd21
WIP: Refactor object building logic away from the GET serviceto Refactor object building logic away from the GET service@ -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()),
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
and5876473cf6
commits look redundant to medone. also dropped one more.
@ -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))
@fyrchik usually says me that
With
is too expensive in a hot codedropped. i think it depends.
@ -0,0 +108,4 @@
return nil, nil, err
}
//parent object is large object, not previuos!
s#//parent#// parent
oh, i see, you used that more than one time, so you write such comments on purpose?
this comment was just for me, dropped.
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
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.
@ -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")
add a var for the err and reuse it in the tests?
fixed
34c7cfbd21
to151b748bcc
@ -0,0 +24,4 @@
addr oid.Address
splitInfo *objectSDK.SplitInfo
rng *objectSDK.Range
log *zap.Logger
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.
fixed
151b748bcc
to6a5349d381
6a5349d381
to552ec79566
552ec79566
tod11d2ac6d2
@ -143,0 +69,4 @@
exec.collectedObject = obj
case errors.As(err, new(*objectSDK.SplitInfoError)):
exec.status = statusVIRTUAL
exec.err = err
Previously we did more complex logic here and
exec.err
was created withobjectSDK.NewSplitInfoError
.What is the reason for this change?
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
@ -143,0 +70,4 @@
case errors.As(err, new(*objectSDK.SplitInfoError)):
exec.status = statusVIRTUAL
exec.err = err
case errors.As(err, new(*apistatus.ObjectOutOfRange)):
If you look closely, we had
apistatus
for local operations and*apistatus
for remote.Could you verify it continues to work now?
fixed
@ -0,0 +75,4 @@
return sourceObjectID, true
}
sourceObjectID, ok = a.splitInfo.LastPart()
if !ok {
Is there any reason why this condition is inverted, compared to the previous condition in this function?
fixed
@ -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)
While we are here, why not just
nil
?just because nil is evil. fixed.
d11d2ac6d2
to5cd86df1ef
5cd86df1ef
to5b25c800ac
5b25c800ac
toac0a278a05