TrueCloudLab/frostfs-sdk-go

Fork 21

Fix object payload size limiter #293

Merged

fyrchik merged 1 commit from aarifullin/frostfs-sdk-go:fix/object_transformer into master

2024-11-07 12:11:20 +00:00

aarifullin commented

2024-11-05 13:56:31 +00:00

Member

Sending empty chunks by writeChunk should not release new objects as this doesn't change payloadSizeLimiter internal
state.
This fixes the bug with patcher when an offset of a patch equals to MaxSize: payloadSizeLimiter releases object again although state is the same. This led to error because EC-encoder receives empty payload and couldn't not process it.

* Sending empty chunks by `writeChunk` should not release new objects as this doesn't change `payloadSizeLimiter` internal state. * This fixes the bug with patcher when an offset of a patch equals to `MaxSize`: `payloadSizeLimiter` releases object again although state is the same. This led to error because EC-encoder receives empty payload and couldn't not process it.

aarifullin added the

bug

label 2024-11-05 13:56:31 +00:00

aarifullin added 1 commit 2024-11-05 13:56:31 +00:00

[#XX] object: Fix payload size limiter

DCO / DCO (pull_request) Failing after 55s

Details

Tests and linters / Tests (pull_request) Successful in 1m21s

Details

Tests and linters / Lint (pull_request) Successful in 1m53s

Details

25523066e7

* Sending empty chunks by `writeChunk` should not release new
  objects as this doesn't change `payloadSizeLimiter` internal
  state.
* This also fixes the bug with patcher when an offset of a patch
  equals to `MaxSize` - `payloadSizeLimiter` releases object again
  although state is the same. This led to error because EC-encoder
  receieved empty payload and couldn't not process it.

Signed-off-by: Airat Arifullin <a.arifullin@yadro.com>

aarifullin requested review from storage-core-committers 2024-11-05 13:56:41 +00:00

aarifullin requested review from storage-core-developers 2024-11-05 13:56:43 +00:00

aarifullin force-pushed fix/object_transformer from 25523066e7 to 26e059e628

2024-11-05 13:59:58 +00:00

Compare

fyrchik approved these changes 2024-11-05 14:27:32 +00:00

Dismissed

dstepanov-yadro approved these changes 2024-11-05 14:30:59 +00:00

Dismissed

fyrchik commented

2024-11-05 14:34:26 +00:00

Owner

@aarifullin could you check the following case?
The last write is maxSize + some bytes. I.e. we must release 2 objects in writeChunk().

I have concerns because chunk variable is altered at the end of the loop, and we might hit the condition even if writeChunk was called with non-empty chunk.

@aarifullin could you check the following case? The last write is `maxSize + some bytes`. I.e. we must release 2 objects in `writeChunk()`. I have concerns because `chunk` variable is altered at the end of the loop, and we might hit the condition even if `writeChunk` was called with non-empty chunk.

aarifullin commented

2024-11-06 08:19:18 +00:00

Author

Member

@aarifullin could you check the following case?
The last write is maxSize + some bytes. I.e. we must release 2 objects in writeChunk().

I have concerns because chunk variable is altered at the end of the loop, and we might hit the condition even if writeChunk was called with non-empty chunk.

You're talking about the case N * max_size + some_bytes. This releases N + 1 objects - that has been before and that is for now. TestTransform already checks for the number of produced objects.

and we might hit the condition even if writeChunk was called with non-empty chunk.

You're right. It hits. After first loop we write max_size bytes into payload and increase written. Chunk is cut and some_bytes are left. Next iteration. We hit the condition, release the previous max_size write and payload is truncated for next iterations. We write remaining bytes into payload that is emptied before. Chunk is done -> writeChunk is done. So, we Close() target and this invocation releases last object and finishes the whole job

> @aarifullin could you check the following case? > The last write is `maxSize + some bytes`. I.e. we must release 2 objects in `writeChunk()`. > > I have concerns because `chunk` variable is altered at the end of the loop, and we might hit the condition even if `writeChunk` was called with non-empty chunk. You're talking about the case `N * max_size + some_bytes`. This releases `N + 1` objects - that has been before and that is for now. `TestTransform` already checks for the number of produced objects. > and we might hit the condition even if `writeChunk` was called with non-empty chunk. You're right. It hits. After first loop we write `max_size` bytes into `payload` and increase `written`. Chunk is cut and `some_bytes` are left. Next iteration. We hit the condition, release the previous `max_size` write and `payload` is truncated for next iterations. We write remaining bytes into `payload` that is emptied before. Chunk is done -> `writeChunk` is done. So, we `Close()` target and this invocation releases last object and finishes the whole job

fyrchik commented

2024-11-06 08:29:01 +00:00

Owner

Let me be more clear:

maxSize = 16
We first write 8 bytes, then we write 24 bytes.

Does this work as expected?

Let me be more clear: 1. maxSize = 16 2. We first write 8 bytes, then we write 24 bytes. Does this work as expected?

aarifullin force-pushed fix/object_transformer from 26e059e628 to 5f962e0465

2024-11-06 08:48:42 +00:00

Compare

aarifullin dismissed fyrchik's review 2024-11-06 08:48:43 +00:00

Reason:

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

aarifullin dismissed dstepanov-yadro's review 2024-11-06 08:48:43 +00:00

Reason:

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

aarifullin commented

2024-11-06 08:49:54 +00:00

Author

Member

Let me be more clear:

maxSize = 16

We first write 8 bytes, then we write 24 bytes.

Does this work as expected?

Please, check the new unit-test TestTransformerNonMonotonicWrites. I hope I correctly got your idea: firstly, it writes 8 bytes then 24 bytes and we get 2 released objects as expected

> Let me be more clear: > 1. maxSize = 16 > 2. We first write 8 bytes, then we write 24 bytes. > > Does this work as expected? > Please, check the new unit-test `TestTransformerNonMonotonicWrites`. I hope I correctly got your idea: firstly, it writes `8 bytes` then `24` bytes and we get 2 released objects as expected