TrueCloudLab/frostfs-node
21
1
Fork 27
Code Issues 108 Pull requests 3 Projects Releases 1 Activity Actions

Assemble complex object headers if linking object is deleted on EC containers #1295

Merged
fyrchik merged 3 commits from dstepanov-yadro/frostfs-node:fix/virtual_object_head into master 2024-09-04 19:51:10 +00:00
Conversation 0 Commits 3 Files changed 13 +95 -42
dstepanov-yadro commented 2024-08-06 06:37:41 +00:00
Member
Copy link

Scenario:

  1. Put complex EC object
  2. Delete linking object
  3. Head without --raw this object

As is: object not found will be thrown, log will contain can't unmarshal child with parent: object ID is not set error

To be: object headers will be returned

Frostfs-testcases for Object API passed:
image

Scenario: 1. Put complex EC object 2. Delete linking object 3. `Head` without `--raw` this object As is: `object not found` will be thrown, log will contain `can't unmarshal child with parent: object ID is not set` error To be: object headers will be returned Frostfs-testcases for Object API passed: ![image](/attachments/81b6a4a0-a9b0-4d76-9f9e-171f4768e69d)
image.png
53 KiB
dstepanov-yadro reviewed 2024-08-06 06:40:10 +00:00
pkg/local_object_storage/engine/head.go Outdated
@ -126,17 +127,16 @@ func (e *StorageEngine) head(ctx context.Context, prm HeadPrm) (HeadRes, error)
return true
})
if head != nil {
dstepanov-yadro commented 2024-08-06 06:40:10 +00:00
Author
Member
Copy link

Reason to change priority: if some shard returns split info error, but other shard has linking object, then return head result directly.

Reason to change priority: if some shard returns split info error, but other shard has linking object, then return head result directly.
dstepanov-yadro reviewed 2024-08-06 06:40:13 +00:00
dstepanov-yadro reviewed 2024-08-06 06:43:37 +00:00
pkg/services/object/get/get_test.go Outdated
@ -730,7 +730,7 @@ func TestGetRemoteSmall(t *testing.T) {
t.Run("VIRTUAL", func(t *testing.T) {
testHeadVirtual := func(svc *Service, addr oid.Address, i *objectSDK.SplitInfo) {
headPrm := newHeadPrm(false, nil)
dstepanov-yadro commented 2024-08-06 06:43:36 +00:00
Author
Member
Copy link

For raw == false cases it is expected to return object headers, but not split info error.

For `raw == false` cases it is expected to return object headers, but not split info error.
dstepanov-yadro reviewed 2024-08-06 06:44:07 +00:00
pkg/services/object/get/request.go Outdated
@ -90,3 +90,2 @@
func (r *request) canAssemble() bool {
return !r.isRaw() && !r.headOnly()
func (r *request) canAssembleComplexObject() bool {
dstepanov-yadro commented 2024-08-06 06:44:07 +00:00
Author
Member
Copy link

To make method name more clear.

To make method name more clear.
dstepanov-yadro force-pushed fix/virtual_object_head from b099058d46 to 3cbea98e28 2024-08-06 06:46:21 +00:00 Compare
dstepanov-yadro requested review from storage-core-committers 2024-08-06 06:51:37 +00:00
dstepanov-yadro requested review from storage-core-developers 2024-08-06 06:51:37 +00:00
fyrchik reviewed 2024-08-06 07:01:57 +00:00
pkg/services/object/get/assembler.go Outdated
@ -65,15 +77,33 @@ func (a *assembler) Assemble(ctx context.Context, writer ObjectWriter) (*objectS
return a.parentObject, nil
}
func (a *assembler) getHead(ctx context.Context, sourceObjectID oid.ID, writer ObjectWriter) (*objectSDK.Object, error) {
fyrchik commented 2024-08-06 07:01:55 +00:00
Owner
Copy link

It seems to be completely independent of other assembler methods, do we have a reason to provide headOnly as a parameter to the assembler instead of writing a separate function?

It seems to be completely independent of other `assembler` methods, do we have a reason to provide `headOnly` as a parameter to the `assembler` instead of writing a separate function?
dstepanov-yadro commented 2024-08-06 07:28:01 +00:00
Author
Member
Copy link

Good point. Now this method looks more related to assembling: it tries to find parent headers from linking and last part objects.

Good point. Now this method looks more related to assembling: it tries to find parent headers from linking and last part objects.
pkg/services/object/get/assembler.go Outdated
@ -75,2 +103,4 @@
a.log.Debug(logs.LastPartIDFound, zap.Stringer("last_part_object_id", sourceObjectID))
return sourceObjectID, true
}
a.log.Warn(logs.EmptySplitInfo)
fyrchik commented 2024-08-06 06:59:25 +00:00
Owner
Copy link

We return false, then create an error and will log again. Why is this Warn?

We return `false`, then create an error and will log again. Why is this `Warn`?
dstepanov-yadro commented 2024-08-06 07:16:24 +00:00
Author
Member
Copy link

This Warn explains why Assemble returns error.

This `Warn` explains why `Assemble` returns error.
fyrchik commented 2024-08-06 08:41:38 +00:00
Owner
Copy link

I don't like the assembler being able to log anything, as it will be eventually moved to the SDK.

I don't like the assembler being able to log anything, as it will be eventually moved to the SDK.
dstepanov-yadro commented 2024-08-06 09:02:19 +00:00
Author
Member
Copy link
  1. But now assembler is here. Logger could be replaced with an interface.
  2. I think it is useful for troubleshooting.
1. But now assembler is here. Logger could be replaced with an interface. 2. I think it is useful for troubleshooting.
fyrchik commented 2024-08-06 11:48:16 +00:00
Owner
Copy link

It only adds complexity (by producing side-effects), even though we can return everything in the result.
The result in turn is what we log and can use for troubleshooting.

It only adds complexity (by producing side-effects), even though we can return everything in the result. The result in turn is what we log and can use for troubleshooting.
dstepanov-yadro commented 2024-08-06 13:48:48 +00:00
Author
Member
Copy link

I disagree, but fixed.

I disagree, but fixed.
pkg/services/object/get/get_test.go Outdated
@ -731,3 +731,3 @@
t.Run("VIRTUAL", func(t *testing.T) {
testHeadVirtual := func(svc *Service, addr oid.Address, i *objectSDK.SplitInfo) {
headPrm := newHeadPrm(false, nil)
headPrm := newHeadPrm(true, nil)
fyrchik commented 2024-08-06 06:59:50 +00:00
Owner
Copy link

I would expect the tests to stay the same, what is the reason?

I would expect the tests to stay the same, what is the reason?
dstepanov-yadro commented 2024-08-06 07:12:35 +00:00
Author
Member
Copy link

For raw == false cases it is expected to return object headers, but not split info error, as test does. So this test looks loke something unrelated to real code and works just because of mocks.

For `raw == false` cases it is expected to return object headers, but not split info error, as test does. So this test looks loke something unrelated to real code and works just because of mocks.
fyrchik marked this conversation as resolved
dstepanov-yadro force-pushed fix/virtual_object_head from 3cbea98e28 to 82d8190691 2024-08-06 07:26:26 +00:00 Compare
acid-ant reviewed 2024-08-06 08:02:38 +00:00
pkg/services/object/get/assembler.go Outdated
@ -68,0 +112,4 @@
if parent == nil {
return nil, objectSDK.NewSplitInfoError(a.splitInfo)
}
if err := writer.WriteHeader(ctx, obj.Parent()); err != nil {
acid-ant commented 2024-08-06 08:02:15 +00:00
Member
Copy link

obj.Parent() -> parent

obj.Parent() -> parent
dstepanov-yadro commented 2024-08-06 08:24:07 +00:00
Author
Member
Copy link

Fixed

Fixed
acid-ant marked this conversation as resolved
dstepanov-yadro force-pushed fix/virtual_object_head from 82d8190691 to cac6d59bfa 2024-08-06 08:23:38 +00:00 Compare
aarifullin approved these changes 2024-08-06 08:33:51 +00:00
acid-ant approved these changes 2024-08-06 13:43:53 +00:00
dstepanov-yadro force-pushed fix/virtual_object_head from cac6d59bfa to 8e51d7849a 2024-08-06 13:48:37 +00:00 Compare
fyrchik approved these changes 2024-08-06 14:49:19 +00:00
fyrchik merged commit 8e51d7849a into master 2024-08-06 14:49:32 +00:00
fyrchik referenced this pull request from a commit 2024-08-06 14:49:34 +00:00
[#1295] engine: Resolve funlen linter
Sign in to join this conversation.
Reviewers
No reviewers
aarifullin
acid-ant
fyrchik
Labels
Clear labels   
P0

Highest

  
P1

High

  
P2

Medium

  
P3

Low

  
badger

Badger DB related issues

  
frostfs-adm

   
frostfs-cli

   
frostfs-ir

   
frostfs-lens

   
frostfs-node

   
good first issue

Good for newcomers

  
triage
  
Infrastructure

Infrastructure, Automation, CI/CD or DevOps related stuff

  
blocked

The issue or PR is blocked by something

  
bug

Something is not working

  
config

Configuration format update or breaking change

  
discussion

Talking about something in order to reach a decision or to exchange ideas

  
documentation

Documentation related

  
duplicate

This issue or pull request already exists

  
enhancement

New feature

  
go

Language features adoption

  
help wanted

Extra attention is needed

  
internal

   
invalid

Something is wrong

  
kludge

This is a kludge and we know it

  
observability

Related to metrics, tracing and logs

  
perfomance

Perfomance related

  
question

More information is needed

  
refactoring

Refactoring

  
wontfix

This won't be fixed

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
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
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#1295
No description provided.
Delete branch "dstepanov-yadro/frostfs-node:fix/virtual_object_head"

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?