TrueCloudLab/frostfs-sdk-go
30
0
Fork 21
Code Issues 33 Pull requests 2 Releases Activity Actions

client: Fix PatchPayload method #268

Merged
fyrchik merged 2 commits from aarifullin/frostfs-sdk-go:fix/patch/4 into master 2024-11-02 14:21:45 +00:00
Conversation 2 Commits 2 Files changed 2 +109 -12
aarifullin commented 2024-09-11 11:10:28 +00:00
Member
Copy link
  • Make the method PatchPayload send a patch with empty payload patch if range's length is non-zero and if it's the first call.
  • Empty payload patches just cut original object payload. So, these patches are also valid.
  • The flag 'firstPayloadPatch' keeps its state after first PatchPayload that make other calls incorrectly set patch ranges.
* Make the method `PatchPayload` send a patch with empty payload patch if range's length is non-zero and if it's the first call. * Empty payload patches just cut original object payload. So, these patches are also valid. * The flag 'firstPayloadPatch' keeps its state after first `PatchPayload` that make other calls incorrectly set patch ranges.
aarifullin added the
bug
 label 2024-09-11 11:10:28 +00:00
aarifullin added 1 commit 2024-09-11 11:10:28 +00:00
[#XX] client: Fix PatchPayload method
Some checks failed
DCO / DCO (pull_request) Failing after 1m23s
Details
Tests and linters / Tests (pull_request) Successful in 2m44s
Details
Tests and linters / Lint (pull_request) Successful in 3m24s
Details
69bb03e9bc 
* Make the method `PatchPayload` send a patch with empty
  payload patch if range's length is non-zero and if it's the
  first call.
* Empty payload patches just cut original object payload. So, these
  patches are also valid.

Signed-off-by: Airat Arifullin <a.arifullin@yadro.com>
aarifullin requested review from storage-core-committers 2024-09-11 11:11:24 +00:00
aarifullin force-pushed fix/patch/4 from 69bb03e9bc to 9ef9e51500 2024-09-11 11:11:25 +00:00 Compare
aarifullin requested review from mbiryukova 2024-09-11 11:11:28 +00:00
aarifullin requested review from storage-core-developers 2024-09-11 11:11:29 +00:00
dstepanov-yadro approved these changes 2024-09-11 11:30:21 +00:00
Dismissed
fyrchik commented 2024-09-11 11:38:42 +00:00
Owner
Copy link

Please, add a test which fails on master and passes with your change.

Please, add a test which fails on master and passes with your change.
fyrchik reviewed 2024-09-11 11:41:45 +00:00
client/object_patch.go Outdated
@ -178,6 +178,20 @@ func (x *objectPatcher) PatchPayload(_ context.Context, rng *object.Range, paylo
return false
}
if n == 0 {
if rng.GetLength() != 0 && x.firstPatchPayload {
fyrchik commented 2024-09-11 11:41:45 +00:00
Owner
Copy link

If rng.GetLength() == 0 and n == 0 it should be an error.
Also, where does x.firstPatchPayload restriction comes from?

If `rng.GetLength() == 0` and `n == 0` it should be an error. Also, where does `x.firstPatchPayload` restriction comes from?
aarifullin commented 2024-09-11 14:45:28 +00:00
Author
Member
Copy link

rng.GetLength() == 0 and n == 0

Fixed

Also, where does x.firstPatchPayload restriction comes from?

Briefly, it comes from partitioning of a patch with the large payload for some cases

> rng.GetLength() == 0 and n == 0 Fixed > Also, where does x.firstPatchPayload restriction comes from? Briefly, it comes from partitioning of a patch with the large payload for some cases
fyrchik commented 2024-09-12 07:48:21 +00:00
Owner
Copy link

Briefly, it comes from partitioning of a patch with the large payload for some cases

I don't understand, could you elaborate?

>Briefly, it comes from partitioning of a patch with the large payload for some cases I don't understand, could you elaborate?
aarifullin commented 2024-09-12 08:56:52 +00:00
Author
Member
Copy link

First of all, I removed firstPatchPayload because it's unnecessary structure's flag that only makes repeating PatchPayload incorrect - the second PatchPayload used distorted range offset because this flag was set to true. So, I replaced it with patchIter within this function that fixes that problem.

Still, why do we need to check for patchIter == 0?
.offet = 0, length =0 and the large payload .chunk = "large large payload...". Read buffer size is limited. So, we expect that the payload is read by parts with fixed parameters .offset = 0, length = 0 till the last iteration (n == 0) but it's still valid. If we don't check whether it's first iteration, then we'll fail although this case is valid

First of all, I removed `firstPatchPayload ` because it's unnecessary structure's flag that only makes repeating `PatchPayload` incorrect - the second `PatchPayload` used _distorted_ range offset because this flag was set to true. So, I replaced it with `patchIter` within this function that fixes that problem. Still, why do we need to check for `patchIter == 0`? `.offet = 0, length =0` and the large payload `.chunk = "large large payload...".` Read buffer size is limited. So, we expect that the payload is read by parts with fixed parameters `.offset = 0, length = 0` till the last iteration (`n == 0`) but it's still valid. If we don't check whether it's first iteration, then we'll fail although this case is valid
fyrchik commented 2024-09-12 12:06:32 +00:00
Owner
Copy link

patchIter was exactly I was having in mind, nice

`patchIter` was exactly I was having in mind, nice
fyrchik marked this conversation as resolved
aarifullin force-pushed fix/patch/4 from 9ef9e51500 to cfa90cf232 2024-09-11 13:43:00 +00:00 Compare
aarifullin dismissed dstepanov-yadro's review 2024-09-11 13:43:02 +00:00
Reason:

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

aarifullin requested review from dstepanov-yadro 2024-09-11 13:45:37 +00:00
aarifullin force-pushed fix/patch/4 from cfa90cf232 to c94f80dc3d 2024-09-11 14:33:13 +00:00 Compare
mbiryukova approved these changes 2024-09-11 15:24:09 +00:00
Dismissed
aarifullin requested review from mbiryukova 2024-09-11 16:11:06 +00:00
acid-ant approved these changes 2024-09-11 16:36:56 +00:00
Dismissed
aarifullin changed title from client: Fix PatchPayload method to WIP: client: Fix PatchPayload method 2024-09-12 02:19:19 +00:00
aarifullin commented 2024-09-12 02:19:53 +00:00
Author
Member
Copy link

Some rework is still required

Some rework is still required
aarifullin force-pushed fix/patch/4 from c94f80dc3d to 22d344044b 2024-09-12 08:27:25 +00:00 Compare
aarifullin dismissed mbiryukova's review 2024-09-12 08:27:25 +00:00
Reason:

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

aarifullin dismissed acid-ant's review 2024-09-12 08:27:25 +00:00
Reason:

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

aarifullin changed title from WIP: client: Fix PatchPayload method to client: Fix PatchPayload method 2024-09-12 08:48:33 +00:00
fyrchik approved these changes 2024-09-12 11:33:36 +00:00
Dismissed
client/object_patch.go Outdated
@ -182,4 +194,3 @@
}
rngPart := object.NewRange()
if x.firstPatchPayload {
fyrchik commented 2024-09-12 11:31:41 +00:00
Owner
Copy link

It seems to be another bug, multiple patches hasn't worked properly at all, right?

It seems to be another bug, multiple patches hasn't worked properly at all, right?
aarifullin commented 2024-09-12 11:48:27 +00:00
Author
Member
Copy link

Right. Because I checked it on big files and didn't see incorrect behavior

Right. Because I checked it on big files and didn't see incorrect behavior
fyrchik marked this conversation as resolved
fyrchik approved these changes 2024-09-12 12:06:40 +00:00
aarifullin force-pushed fix/patch/4 from 22d344044b to d342c0bc16 2024-09-12 12:14:08 +00:00 Compare
aarifullin requested review from fyrchik 2024-09-12 12:14:34 +00:00
dstepanov-yadro approved these changes 2024-09-12 12:53:10 +00:00
fyrchik merged commit d342c0bc16 into master 2024-09-12 12:55:45 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
mbiryukova
fyrchik
dstepanov-yadro
TrueCloudLab/storage-core-developers
Labels
Clear labels   
P0

Highest

  
P1

High

  
P2

Medium

  
P3

Low

  
good first issue

Good for newcomers

  
pool

Tasks related to client pool

  
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
good first issue
pool
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
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#268
No description provided.
Delete branch "aarifullin/frostfs-sdk-go:fix/patch/4"

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?