Introduce patcher package #247

Merged
fyrchik merged 2 commits from aarifullin/frostfs-sdk-go:feat/patch/1 into master 2024-08-05 12:32:15 +00:00
Member

Introduce Patch type

  • Make ToV2, FromV2 converters for Patch and PayloadPatch;
  • Add unit-tests.

object: Introduce patcher package

  • Introduce patcher package that contains such interfaces to be implemented:
    • PatchApplier - the main patching engine that merges the stream of patches and the stream of original object payload divided by ranges. The merged streams result is output to ChunkedObjectWriter;
    • RangeProvider - provides the original object payload by ranges;
    • HeaderProvider - provides the original object header.
  • Introduce patcher that implements PatchApplier;
  • Cover all possible cases with unit-tests.
#### Introduce `Patch` type * Make ToV2, FromV2 converters for `Patch` and `PayloadPatch`; * Add unit-tests. #### object: Introduce `patcher` package * Introduce `patcher` package that contains such interfaces to be implemented: - `PatchApplier` - the main patching engine that merges the stream of patches and the stream of original object payload divided by ranges. The merged streams result is output to `ChunkedObjectWriter`; - `RangeProvider` - provides the original object payload by ranges; - `HeaderProvider` - provides the original object header. * Introduce `patcher` that implements `PatchApplier`; * Cover all possible cases with unit-tests.
aarifullin added the
discussion
label 2024-07-31 15:04:07 +00:00
aarifullin added 2 commits 2024-07-31 15:04:13 +00:00
* Make ToV2, FromV2 converters for Patch and PayloadPatch;
* Add unit-tests.

Signed-off-by: Airat Arifullin <a.arifullin@yadro.com>
[#XX] object: Introduce patcher package
Some checks failed
DCO / DCO (pull_request) Failing after 1m10s
Tests and linters / Tests (1.21) (pull_request) Successful in 1m15s
Tests and linters / Tests (1.22) (pull_request) Failing after 1m13s
Tests and linters / Lint (pull_request) Failing after 1m32s
e4aa512c1f
* Introduce `patcher` package that contains such interfaces to be
  implemented:
  - `PatchApplier` - the main patching engine that merges the stream
    of patches and the stream of original object payload divided by
    ranges. The merged streams result is output to `ChunkedObjectWriter`;
  - `RangeProvider` - provides the original object payload by ranges;
  - `HeaderProvider` - provides the original object header.
* Introduce `patcher` that implements `PatchApplier`;
* Cover all possible cases with unit-tests.

Signed-off-by: Airat Arifullin <a.arifullin@yadro.com>
aarifullin force-pushed feat/patch/1 from e4aa512c1f to e4d85fa865 2024-07-31 15:04:39 +00:00 Compare
aarifullin requested review from storage-core-committers 2024-07-31 15:04:41 +00:00
aarifullin requested review from storage-core-developers 2024-07-31 15:04:43 +00:00
aarifullin requested review from storage-services-committers 2024-07-31 15:04:46 +00:00
aarifullin requested review from storage-services-developers 2024-07-31 15:04:55 +00:00
aarifullin force-pushed feat/patch/1 from e4d85fa865 to 2fc0854a48 2024-07-31 15:23:49 +00:00 Compare
aarifullin requested review from mbiryukova 2024-07-31 15:24:09 +00:00
aarifullin force-pushed feat/patch/1 from 2fc0854a48 to 7d29c58d7f 2024-07-31 15:49:40 +00:00 Compare
fyrchik requested changes 2024-08-01 05:35:50 +00:00
@ -0,0 +13,4 @@
var (
ErrOffsetExceedsSize = errors.New("patch offset exceeds object size")
ErrEmptyPayloadPatch = errors.New("patch must contain payload")
Owner

Changing only attributes is allowed, when do we throw this message?

Changing only attributes is allowed, when do we throw this message?
Author
Member

According to RFC:

Only the first stream message can contain a patch for object attributes

That means ApplyPatch can be invoked with patch that must set only PayloadPatch if this invocation is not first. I think the error message should be slightly fixed

According to RFC: > Only the first stream message can contain a patch for object attributes That means `ApplyPatch` can be invoked with `patch` that must set only `PayloadPatch` if this invocation is not first. I think the error message should be slightly fixed
Author
Member

Refactored to ErrPayloadPatchIsNil

Refactored to `ErrPayloadPatchIsNil`
fyrchik marked this conversation as resolved
@ -0,0 +39,4 @@
// HeaderProvider is the interface that provides a method to get an original's object header.
type HeaderProvider interface {
// GetObjectHeader gets an original's object header.
GetObjectHeader(ctx context.Context, addr oid.Address) (*objectSDK.Object, error)
Owner

This is an interface with method called only once (exactly once) for a single address.
Why not provide the header as a parameter?

This is an interface with method called only once (exactly once) for a single address. Why not provide the header as a parameter?
Author
Member

Briefly, it's kind of trick for patcher's initialization. If we agree about this interface, HeaderProvider will be not needed

Briefly, it's kind of trick for `patcher`'s initialization. If we agree about [this interface](https://git.frostfs.info/TrueCloudLab/frostfs-sdk-go/pulls/247/files#issuecomment-45868), `HeaderProvider` will be not needed
aarifullin marked this conversation as resolved
@ -0,0 +48,4 @@
// ReadRange reads an original object payload by the given range.
// The method returns io.Reader over the data range only. This means if the data is read out,
// then ReadRange has to be invoked to provide reader over the next range.
ReadRange(ctx context.Context, addr oid.Address, rng *objectSDK.Range) io.Reader
Owner

We have a single address for patcher, I think it should accept addressless interfaces, there is no need in duplication.

We have a single address for patcher, I think it should accept addressless interfaces, there is no need in duplication.
Author
Member

Fixed

Fixed
fyrchik marked this conversation as resolved
@ -0,0 +81,4 @@
}
}
func (p *patcher) ApplyPatch(ctx context.Context, currPatch *objectSDK.Patch) bool {
Owner

This API mimics the gRPC restrictions, by accepting objectSDK.Patch which can contain both attributes and payload.
I believe the right way is to have two methods: one for attributes func(attr []Attribute, replace bool), another for payload patches func(PayloadPatch).
We then need only to validate that the former is called once, and we won't need multiple nested ifs as in this method.

This API mimics the gRPC restrictions, by accepting `objectSDK.Patch` which can contain both attributes and payload. I believe the right way is to have two methods: one for attributes `func(attr []Attribute, replace bool)`, another for payload patches `func(PayloadPatch)`. We then need only to validate that the former is called once, and we won't need multiple nested ifs as in this method.
Author
Member

We agreed that patch stream won't get an initialization message (like for Put), but patcher requires some initialization path.

Let's consider such an interface:

(first of all I'd like to introduce header patching func(attr []Attribute, replace bool) like ApplyHeaderPatch)

type PatchApplier interface {
  Init(patch *objectSDK.Patch) error
  
  ApplyHeaderPatch(patch *objectSDK.Patch) bool // for new attributes
  
  ApplyPayloadPatch(patch *objectSDK.Patch) bool
  
  Close(context.Context) (PatchRes, error)
}

So, a client like frostfs-storage is responsible to follow the invocation order Init -> ApplyHeaderPatch for the same patch -> ApplyPayloadPatch -> Close. Is it ok?

We agreed that `patch` stream won't get an initialization message (like for `Put`), but `patcher` requires some initialization path. Let's consider such an interface: (first of all I'd like to introduce header patching `func(attr []Attribute, replace bool)` like `ApplyHeaderPatch`) ```go type PatchApplier interface { Init(patch *objectSDK.Patch) error ApplyHeaderPatch(patch *objectSDK.Patch) bool // for new attributes ApplyPayloadPatch(patch *objectSDK.Patch) bool Close(context.Context) (PatchRes, error) } ``` So, a client like `frostfs-storage` is responsible to follow the invocation order `Init` -> `ApplyHeaderPatch` for the same patch -> `ApplyPayloadPatch` -> `Close`. Is it ok?
Owner

Why does ApplyHeaderPatch and ApplyPayloadPatch have the same argument?
What do we need Init for?

Why does `ApplyHeaderPatch` and `ApplyPayloadPatch` have the same argument? What do we need `Init` for?
Author
Member

...have the same argument?

I've already reconsidered this approach. So:

// PatchApplier is the interface that provides method to apply header and payload patches.
type PatchApplier interface {
	// ApplyAttributesPatch applies the patch only the object's attributes. This method must be invoked
	// only after Init and before ApplyPayloadPatch.
	ApplyAttributesPatch(ctx context.Context, newAttrs []objectSDK.Attribute, replaceAttrs bool) error

	// ApplyPayloadPatch applies the patch for the object's payload.
	ApplyPayloadPatch(ctx context.Context, payloadPatch *objectSDK.PayloadPatch) error

	// Close closes PatchApplier when the patch stream is over.
	Close(context.Context) (PatchRes, error)
}

What do we need Init for?

Nevermind, we don't. We just need to refactor the consturctor

func New(originalObjectHeader *objectSDK.Header, objectRangeSplitter RangeProvider, objectWriter transformer.ChunkedObjectWriter) PatchApplier {
	return &patcher{
		rangeProvider: objectRangeSplitter,

		objectWriter: objectWriter,

		originalObjectHeader: originalObjectHeader,
	}
}

ApplyAttributesPatch will be applied for original hdr, addr is used by RangeProvider

> ...have the same argument? I've already reconsidered this approach. So: ```go // PatchApplier is the interface that provides method to apply header and payload patches. type PatchApplier interface { // ApplyAttributesPatch applies the patch only the object's attributes. This method must be invoked // only after Init and before ApplyPayloadPatch. ApplyAttributesPatch(ctx context.Context, newAttrs []objectSDK.Attribute, replaceAttrs bool) error // ApplyPayloadPatch applies the patch for the object's payload. ApplyPayloadPatch(ctx context.Context, payloadPatch *objectSDK.PayloadPatch) error // Close closes PatchApplier when the patch stream is over. Close(context.Context) (PatchRes, error) } ``` > What do we need Init for? Nevermind, we don't. We just need to refactor the consturctor ``` func New(originalObjectHeader *objectSDK.Header, objectRangeSplitter RangeProvider, objectWriter transformer.ChunkedObjectWriter) PatchApplier { return &patcher{ rangeProvider: objectRangeSplitter, objectWriter: objectWriter, originalObjectHeader: originalObjectHeader, } } ``` `ApplyAttributesPatch` will be applied for original `hdr`, `addr` is used by `RangeProvider`
Owner

This one looks almost perfect!

This one looks almost perfect!
Author
Member

Have refactored the interface. Please, check this out

Have refactored the interface. Please, check this out
fyrchik marked this conversation as resolved
@ -0,0 +83,4 @@
func (p *patcher) ApplyPatch(ctx context.Context, currPatch *objectSDK.Patch) bool {
if currPatch == nil {
return true
Owner

Why do we return bool instead of error here. We do this in node in object services, frankly this was a costly mistake.

Why do we return `bool` instead of `error` here. We do this in node in `object` services, frankly this was a costly mistake.
Author
Member

TBH, I used this approach because I wanted to test ApplyPatch. If we expect an error from ApplyPatch but check this without Close then we also need to expect on which iteration we expect the error.

TBH, I used this approach because I wanted to test `ApplyPatch`. If we expect an error from `ApplyPatch` but check this without `Close` then we also need to expect on which iteration we expect the error.
Author
Member

The method is no longer relevant. Let me resolve the comment

The method is no longer relevant. Let me resolve the comment
aarifullin marked this conversation as resolved
@ -0,0 +161,4 @@
rdr := p.rangeProvider.ReadRange(ctx, p.addr, rng)
for {
remain := make([]byte, 1024)
Owner

Magic constant. Can we make it a parameter? This rdr.Read could translate to the gRPC call.

Magic constant. Can we make it a parameter? This `rdr.Read` _could_ translate to the gRPC call.
Author
Member

Can we make it a parameter?

Yes, that makes sense

This rdr.Read could translate to the gRPC call.

Could you clarify, please, how is this point related to buffer size?

> Can we make it a parameter? Yes, that makes sense > This rdr.Read could translate to the gRPC call. Could you clarify, please, how is this point related to buffer size?
Owner

Could you clarify, please, how is this point related to buffer size?

The number of times you call read is inversely proportional to the buffer size.

> Could you clarify, please, how is this point related to buffer size? The number of times you call read is inversely proportional to the buffer size.
Author
Member

Fixed. I have introduced a configurable parameter.
Also fixed the consturctor

Fixed. I have introduced a configurable parameter. Also fixed the consturctor
fyrchik marked this conversation as resolved
@ -0,0 +194,4 @@
rdr := p.rangeProvider.ReadRange(ctx, p.addr, rng)
for {
orig := make([]byte, 1024)
Owner

Magic constant.

Magic constant.
aarifullin marked this conversation as resolved
@ -0,0 +238,4 @@
}
var mergedAttrs []objectSDK.Attribute
for key, value := range attrMap {
Owner

I would leave the order of initial attributes as-is.
Iterating over the map is unstable.

I would leave the order of initial attributes as-is. Iterating over the map is unstable.
Author
Member

Fixed, please, check this out

Fixed, please, check this out
fyrchik marked this conversation as resolved
@ -0,0 +316,4 @@
},
},
patchPayloads: []*objectSDK.PayloadPatch{
nil,
Owner

This should be an invalid message, otherwise we could send an infinite stream of nil.

This should be an invalid message, otherwise we could send an infinite stream of `nil`.
Author
Member

Note, that this is not patch := &Patch{} itself. PayloadPatch can be nil if patch is targeted to object attributes

Note, that this is not `patch := &Patch{}` itself. `PayloadPatch` can be nil if patch is targeted to object attributes
Author
Member

As unit-test have been fixed, that's no longer relevant

As unit-test have been fixed, that's no longer relevant
fyrchik marked this conversation as resolved
fyrchik requested changes 2024-08-01 05:44:45 +00:00
@ -0,0 +198,4 @@
var n int
n, err = rdr.Read(orig)
if err != nil {
if err == io.EOF {
Owner

What is err == io.EOF, but n != 0.
It seems you tests this, but I don't see why it works. Reader is explicitly allowed to behave like this:

From https://pkg.go.dev/io#Reader

An instance of this general case is that a Reader returning a non-zero number of bytes at the end of the input stream may return either err == EOF or err == nil. The next Read should return 0, EOF.

What is `err == io.EOF`, but `n != 0`. It seems you tests this, but I don't see why it works. `Reader` is explicitly allowed to behave like this: From https://pkg.go.dev/io#Reader >An instance of this general case is that a Reader returning a non-zero number of bytes at the end of the input stream may return either err == EOF or err == nil. The next Read should return 0, EOF.
Author
Member

Callers should always process the n > 0 bytes returned before considering the error err

Yes, you're right. It's implementation based and I couldn't catch this with io.Pipe and bytes.NewReader. n > 0` should be proccessed anyway. I'll fix that

> Callers should always process the n > 0 bytes returned before considering the error err Yes, you're right. It's implementation based and I couldn't catch this with `io.Pipe` and `bytes.NewReader`. n > 0` should be proccessed anyway. I'll fix that
Author
Member

Fixed, check copyPayload out, please

Fixed, check `copyPayload` out, please
fyrchik marked this conversation as resolved
@ -0,0 +53,4 @@
var _ RangeProvider = (*mockRangeProvider)(nil)
func (m *mockRangeProvider) ReadRange(_ context.Context, addr oid.Address, rng *objectSDK.Range) io.Reader {
Owner

This whole function is rather big.
It seems to me it could be reduced to a bytes.NewReader(m.originalPayload[offset:offset+length]).
Or no?

This whole function is rather big. It seems to me it could be reduced to a `bytes.NewReader(m.originalPayload[offset:offset+length])`. Or no?
Author
Member

Okay, I'll fix that to bytes.NewReader. I just wanted to check how it's gonna work with io.Pipe

Okay, I'll fix that to `bytes.NewReader`. I just wanted to check how it's gonna work with `io.Pipe`
Author
Member

Fixed

Fixed
aarifullin marked this conversation as resolved
aarifullin force-pushed feat/patch/1 from 7d29c58d7f to 13dc1d3a01 2024-08-01 10:41:29 +00:00 Compare
aarifullin force-pushed feat/patch/1 from 13dc1d3a01 to 950b4b7b20 2024-08-01 12:11:52 +00:00 Compare
aarifullin force-pushed feat/patch/1 from 950b4b7b20 to c11741807b 2024-08-01 12:14:29 +00:00 Compare
aarifullin force-pushed feat/patch/1 from c11741807b to 3b63e58a09 2024-08-01 12:17:15 +00:00 Compare
aarifullin force-pushed feat/patch/1 from 3b63e58a09 to fd75436d21 2024-08-01 12:17:59 +00:00 Compare
fyrchik reviewed 2024-08-01 12:27:10 +00:00
@ -0,0 +81,4 @@
}
}
func (p *patcher) ApplyAttributesPatch(ctx context.Context, newAttrs []objectSDK.Attribute, replaceAttrs bool) error {
Owner

We need to validate that this function is called once (and an appropriate test).

We need to validate that this function is called once (and an appropriate test).
Author
Member

Fixed. Test - added

Fixed. Test - added
fyrchik marked this conversation as resolved
@ -0,0 +128,4 @@
rng.SetOffset(p.currOffset)
rng.SetLength(p.originalPayloadSize - p.currOffset)
rdr := p.rangeProvider.ReadRange(ctx, rng)
Owner

applyPatch and Close intersect in this part (non-surprising, given that both can copy data from the original object).
What about reusing this code between them? Should be identical.

`applyPatch` and `Close` intersect in this part (non-surprising, given that both can copy data from the original object). What about reusing this code between them? Should be identical.
Author
Member

Fixed, check copyPayload out, please

Fixed, check `copyPayload` out, please
fyrchik marked this conversation as resolved
aarifullin reviewed 2024-08-01 13:02:28 +00:00
@ -0,0 +453,4 @@
require.NoError(t, err)
require.Equal(t, test.patched, patchedObject.Payload())
// attrOfPatchedObject := patchedObject.Attributes()
Author
Member

Oh, my bad

Oh, my bad
aarifullin marked this conversation as resolved
aarifullin force-pushed feat/patch/1 from fd75436d21 to d92e7e4f84 2024-08-01 14:45:42 +00:00 Compare
dstepanov-yadro approved these changes 2024-08-01 15:07:23 +00:00
fyrchik reviewed 2024-08-01 15:09:51 +00:00
@ -0,0 +116,4 @@
p.firstPayloadPatchCall = false
}()
if !p.attrPatchAlreadyApplied {
Owner

We can skip ApplyAttributesPatch completely if we want.

We can skip `ApplyAttributesPatch` completely if we want.
Author
Member

No longer relevant. The reason I wrote this:

#247 (comment)

No longer relevant. The reason I wrote this: https://git.frostfs.info/TrueCloudLab/frostfs-sdk-go/pulls/247#issuecomment-45990
fyrchik marked this conversation as resolved
@ -0,0 +121,4 @@
}
if payloadPatch == nil {
if p.firstPayloadPatchCall {
Owner

What does this if achieve?

What does this if achieve?
Author
Member

Nevermind, I wanted to impose on the client the order of Apply... invocation but I understood that it only makes things unreasonably compilcated. I've removed this flag

Nevermind, I wanted to impose on the client the order of `Apply...` invocation but I understood that it only makes things unreasonably compilcated. I've removed this flag
fyrchik marked this conversation as resolved
aarifullin force-pushed feat/patch/1 from d92e7e4f84 to 73ff62bbe1 2024-08-01 15:11:46 +00:00 Compare
aarifullin force-pushed feat/patch/1 from 73ff62bbe1 to 1a00fb9a05 2024-08-01 15:24:52 +00:00 Compare
aarifullin force-pushed feat/patch/1 from 1a00fb9a05 to 4899a8925f 2024-08-01 15:26:49 +00:00 Compare
aarifullin force-pushed feat/patch/1 from 4899a8925f to 48f4d6933c 2024-08-01 15:39:39 +00:00 Compare
aarifullin requested review from dstepanov-yadro 2024-08-01 17:53:48 +00:00
aarifullin requested review from fyrchik 2024-08-01 17:53:53 +00:00
aarifullin force-pushed feat/patch/1 from 48f4d6933c to c000b255ea 2024-08-01 17:55:46 +00:00 Compare
dstepanov-yadro approved these changes 2024-08-02 09:39:27 +00:00
fyrchik reviewed 2024-08-02 12:43:28 +00:00
@ -0,0 +25,4 @@
// PatchApplier is the interface that provides method to apply header and payload patches.
type PatchApplier interface {
// ApplyAttributesPatch applies the patch only for the object's attributes.
// This method must be ALWAYS invoked first as attribute patch must income ONLY with the first patch
Owner

Can we avoid this restriction? It seems unnecessary.

Can we avoid this restriction? It seems unnecessary.
Author
Member

Sure. The comment is outdated, just forgot to remove this line

Sure. The comment is outdated, just forgot to remove this line
fyrchik marked this conversation as resolved
@ -0,0 +49,4 @@
// ReadRange reads an original object payload by the given range.
// The method returns io.Reader over the data range only. This means if the data is read out,
// then ReadRange has to be invoked to provide reader over the next range.
ReadRange(ctx context.Context, rng *objectSDK.Range) io.Reader
Owner

We have Range and GetRange in node, why add the 3rd way to spell this?

We have `Range` and `GetRange` in node, why add the 3rd way to spell this?
Author
Member

Renamed to GetRange

Renamed to `GetRange`
fyrchik marked this conversation as resolved
@ -0,0 +119,4 @@
if replaceAttrs {
p.hdr.SetAttributes(newAttrs...)
} else if len(newAttrs) > 0 {
mergedAttrs := mergeAttributes(newAttrs, p.hdr.Attributes())
Owner

Does p.hdr.Attributes() copy the slice?
It can be unexpected for the user if no (patch will change the header we have provided in constructor).

Does `p.hdr.Attributes()` copy the slice? It can be unexpected for the user if no (patch will change the header we have provided in constructor).
Author
Member

Sorry, didn't get your question

func (o *Object) Attributes() []Attribute {
	attrs := (*object.Object)(o).
		GetHeader().
		GetAttributes()

	res := make([]Attribute, len(attrs))

	for i := range attrs {
		res[i] = *NewAttributeFromV2(&attrs[i])
	}

	return res
}

So, mergeAttrs equals to p.hdr.Attributes() if len(newAttrs) == 0

Sorry, didn't get your question ```go func (o *Object) Attributes() []Attribute { attrs := (*object.Object)(o). GetHeader(). GetAttributes() res := make([]Attribute, len(attrs)) for i := range attrs { res[i] = *NewAttributeFromV2(&attrs[i]) } return res } ``` So, `mergeAttrs` equals to `p.hdr.Attributes()` if `len(newAttrs) == 0`
Owner

So Attributes allocates, then we can reuse the slice, that was the question.

So `Attributes` allocates, then we can reuse the slice, that was the question.
fyrchik marked this conversation as resolved
@ -0,0 +157,4 @@
// copy remaining originial payload
if err := p.copyRange(ctx, rng); err != nil {
return PatchRes{}, fmt.Errorf("copy payload error: %w", err)
Owner

if you wrap it multiple times we will have patch error: copy payload error: read error:. The error conveys no meaning and is probably mentioned somewhere else in the logging level.
What about removing it, compare to applyPatch: copy payload: read: ...

if you wrap it multiple times we will have `patch error: copy payload error: read error:`. The error conveys no meaning and is probably mentioned somewhere else in the logging level. What about removing it, compare to `applyPatch: copy payload: read: ...`
Author
Member

Good point, I'm gonna fix this

Good point, I'm gonna fix this
Author
Member

Fixed

Fixed
fyrchik marked this conversation as resolved
@ -0,0 +219,4 @@
}
func mergeAttributes(newAttrs, oldAttrs []objectSDK.Attribute) []objectSDK.Attribute {
attrMap := make(map[string]string)
Owner

We know it has len(newAttrs) elements, could prealloc.

We know it has `len(newAttrs)` elements, could prealloc.
Author
Member

Fixed

Fixed
fyrchik marked this conversation as resolved
@ -0,0 +556,4 @@
require.NoError(t, err)
require.Equal(t, test.patched, patchedObject.Payload())
newAttrs := append([]objectSDK.Attribute{}, test.patches[0].NewAttributes...)
Owner

Does original object has no headers, though?

Does original object has no headers, though?
Author
Member

This is a trick for require.Equal becasue it compares nil with empty slice and throws the error although logically they are both empty

This is a trick for `require.Equal` becasue it compares `nil` with empty slice and throws the error although logically they are both empty
Owner

No, I mean do we have the test which patches and object with already existing headers and replaces some of them?
The test should check correctness and the order.

No, I mean do we have the test which patches and object with already existing headers and replaces _some of them_? The test should check correctness and the order.
Author
Member

Oh, my bad. I have added

{
			name: "header only merge attributes",
			patches: []objectSDK.Patch{
				{
					NewAttributes: []objectSDK.Attribute{
						newTestAttribute("key1", "val2"),
						newTestAttribute("key2", "val2-incoming"),
					},
					PayloadPatch: &objectSDK.PayloadPatch{
						Range: rangeWithOffestWithLength(0, 0),
						Chunk: []byte("inserted at the beginning"),
					},
				},
			},
			originalHeaderAttributes: []objectSDK.Attribute{
				newTestAttribute("key2", "to be popped out"),
				newTestAttribute("key3", "val3"),
			},
			originalObjectPayload: []byte("0123456789qwertyuiopasdfghjklzxcvbnm"),
			patchedPayload:        []byte("inserted at the beginning0123456789qwertyuiopasdfghjklzxcvbnm"),
			patchedHeaderAttributes: []objectSDK.Attribute{
				newTestAttribute("key2", "val2-incoming"),
				newTestAttribute("key3", "val3"),
				newTestAttribute("key1", "val2"),
			},
		},

Works fine. The order key2, key3 is correct. The value is replaced

Oh, my bad. I have added ```go { name: "header only merge attributes", patches: []objectSDK.Patch{ { NewAttributes: []objectSDK.Attribute{ newTestAttribute("key1", "val2"), newTestAttribute("key2", "val2-incoming"), }, PayloadPatch: &objectSDK.PayloadPatch{ Range: rangeWithOffestWithLength(0, 0), Chunk: []byte("inserted at the beginning"), }, }, }, originalHeaderAttributes: []objectSDK.Attribute{ newTestAttribute("key2", "to be popped out"), newTestAttribute("key3", "val3"), }, originalObjectPayload: []byte("0123456789qwertyuiopasdfghjklzxcvbnm"), patchedPayload: []byte("inserted at the beginning0123456789qwertyuiopasdfghjklzxcvbnm"), patchedHeaderAttributes: []objectSDK.Attribute{ newTestAttribute("key2", "val2-incoming"), newTestAttribute("key3", "val3"), newTestAttribute("key1", "val2"), }, }, ``` Works fine. The order `key2`, `key3` is correct. The value is replaced
fyrchik marked this conversation as resolved
aarifullin force-pushed feat/patch/1 from c000b255ea to fe2c45671c 2024-08-02 13:01:18 +00:00 Compare
aarifullin force-pushed feat/patch/1 from fe2c45671c to 233c88a526 2024-08-02 13:29:12 +00:00 Compare
fyrchik approved these changes 2024-08-02 14:40:19 +00:00
mbiryukova approved these changes 2024-08-02 16:22:26 +00:00
@ -0,0 +49,4 @@
var attr1, attr2 Attribute
attr1.SetKey("key1")
attr1.SetValue("val1")
attr1.SetKey("key2")
Member

attr2?

`attr2`?
Author
Member

Fixed!

Fixed!
mbiryukova marked this conversation as resolved
@ -0,0 +362,4 @@
},
originalObjectPayload: []byte("0123456789qwertyuiopasdfghjklzxcvbnm"),
patchedPayload: []byte("inserted at the beginning0123456789qwertyuiopasdfghjklzxcvbnm"),
expectedPayloadPatchErr: ErrPayloadPatchIsNil,
Member

Seems it's not expected in this test

Seems it's not expected in this test
Author
Member

That's true, my bad.

This unit-test case shouldn't expect for the error but it passes as testing loop checks require only if ApplyPayloadPatch returns an error. As we don't use getting an error by Close, the proper way is to check an error on a specific iteration (iterationExpErr int), but this makes tests hard to read

That's true, my bad. This unit-test case shouldn't expect for the error but it passes as testing loop checks `require` only if `ApplyPayloadPatch` returns an error. As we don't use getting an error by `Close`, the proper way is to check an error on a specific iteration (`iterationExpErr int`), but this makes tests hard to read
mbiryukova marked this conversation as resolved
aarifullin force-pushed feat/patch/1 from 233c88a526 to 66e1ef74df 2024-08-05 08:15:04 +00:00 Compare
aarifullin force-pushed feat/patch/1 from 66e1ef74df to 6504e83169 2024-08-05 08:18:59 +00:00 Compare
aarifullin requested review from dstepanov-yadro 2024-08-05 08:24:41 +00:00
aarifullin requested review from fyrchik 2024-08-05 08:24:41 +00:00
aarifullin requested review from mbiryukova 2024-08-05 08:24:41 +00:00
mbiryukova approved these changes 2024-08-05 08:38:38 +00:00
achuprov approved these changes 2024-08-05 12:16:05 +00:00
fyrchik merged commit 361739e860 into master 2024-08-05 12:32:15 +00:00
fyrchik referenced this pull request from a commit 2024-08-05 12:32:16 +00:00
fyrchik added the
internal
label 2024-08-05 12:48:43 +00:00
Sign in to join this conversation.
No reviewers
TrueCloudLab/storage-services-committers
No milestone
No project
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#247
No description provided.