client: Introduce ObjectPatch
method #249
No reviewers
TrueCloudLab/storage-core-developers
TrueCloudLab/storage-services-committers
Labels
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
No milestone
No project
No assignees
5 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: TrueCloudLab/frostfs-sdk-go#249
Loading…
Reference in a new issue
No description provided.
Delete branch "aarifullin/frostfs-sdk-go:feat/patch_cli"
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?
SplitByMaxChunkLength
methodObjectPatch
methodpatch
verbobjectPatch
verbSplitByMaxChunkLength
method 895a2ef0e2objectPatch
methodc0109f796a
to6c28bf0dbd
@ -0,0 +35,4 @@
// into a few patches. After split each `PayloadPatch` has a `Chunk` with the size <= `MaxChunkLength`.
//
// Result means success. Failure reason can be received via Close.
Patch(_ context.Context, patch *object.Patch) bool
@fyrchik, please, let's go on discussing here
(from #248 (comment))
let's introduce complexity when it is really needed
Oh, I see now, it is similar to
ObjectWriter
. Then OK.Can we make it more similar to
ObjectWriter
then? It hasWriteHeader
andWriteChunk
as separate methods, similar to what you have done for patcher implementation.Please, check the refactored interface out
@ -0,0 +147,4 @@
maxChunkLen int
}
func (x *objectPatcher) Patch(_ context.Context, patch *object.Patch) bool {
The interface requires us to perform splitting, even though it also performs it internally.
Can we avoid the duplication here?
We might have
Init
function return a more convenient interface on top of it (withPatchHeader
andPatchAttributes
) and make thispatch
private (or allow the user to cast to this "raw" interface via another method).@ -80,1 +80,4 @@
// SplitByMaxChunkLength splits a payload patch into a few payload patches if its `Chunk` size
// exceeds `maxChunkLen`.
func (p *PayloadPatch) SplitByMaxChunkLength(maxChunkLen int) []*PayloadPatch {
I have concerns about this method:
PayloadPatch
is a patch from a single gRPC message.Removed this method, the logic is moved to
object_patch.go
@ -81,0 +81,4 @@
// SplitByMaxChunkLength splits a payload patch into a few payload patches if its `Chunk` size
// exceeds `maxChunkLen`.
func (p *PayloadPatch) SplitByMaxChunkLength(maxChunkLen int) []*PayloadPatch {
if len(p.Chunk) <= maxChunkLen {
If it is expected that patch chunk will too large for single message, then
Chunk
must be something likeio.Reader
to not to store all data in memory.Please, check the refactored interface out
@ -599,6 +599,7 @@ func TestObject_ForVerb(t *testing.T) {
session.VerbObjectRangeHash: v2session.ObjectVerbRangeHash,
session.VerbObjectRange: v2session.ObjectVerbRange,
session.VerbObjectDelete: v2session.ObjectVerbDelete,
session.VerbObjectPatch: v2session.ObjectPatch,
Why
v2session.ObjectPatch
, but notv2session.ObjectVerbPatch
like other methods?TrueCloudLab/frostfs-api-go#100 (comment)
6c28bf0dbd
tocf2254a147
cf2254a147
toc642c42b07
c642c42b07
to5a121098f6
5a121098f6
to27cff41f36
27cff41f36
to5fc7ebc322
5fc7ebc322
to6835affbb1
@ -0,0 +190,4 @@
rngPart.SetOffset(offset)
if originalLength == 0 || offset-rng.GetOffset() >= originalLength {
rngPart.SetLength(0)
It is the default value, no need to set.
Fixed
@ -0,0 +197,4 @@
if !x.patch(&object.Patch{
Address: x.addr,
Why are there spaces in every struct literal?
Fixed
@ -0,0 +209,4 @@
offset += uint64(n)
if length > 0 {
length -= patchLength
The whole offset tracking thing is rather tedious.
I suggest using
(offset, length)
for the first message and(offset+length,0)
for the rest. Should be less code, because we don't need any calculations.Sorry but I don't get the idea and how it helps to recude code. These calculations are needed because we can't just simply break down to
(offset, length)
and(offset + length, 0)
becausechunk
can be very large. This leads to the point when we need to split(offset, length - max_buf_size)
and(offset + length - max_buf_size, length - max_buf_size)
I do not see any problem here.
You replace
(offset,length)
with the first part (of any possible length, you pick).You replace
(offset+length,0)
with the second part (of any possible length, you pick).Where do we need to take
chunk
into account?Let's consider such a patch:
Let
maxChunkLen = 2
-> the size for the buff to read datathen the patch should be splitted like that:
if the first read message is like
(offset, length)
and the rest are(offset + length, 0)
, thenSo, we need to track the length because it's not always
0
in(offset + length, 0)
Why is this correct
{ .offset = 4, .length = 0, .payload = '45' } -> inserts bytes at the original payload
And this is not
{ .offset = 2, .length = 0, .payload = '23' } -> incorrect
?The misunderstanding can stem from the fact that patch size CAN differ from the part in the original payload, so you can replace
X
bytes with anotherY
bytes, and there is no relation between X and Y.We replacing first
.length = 4
bytes and just insert the restSo, consider the buffer size is
2
, so each subpatch's payload can be only<=2
{ .offset = 2, .length = 0, .payload = '23' }
whould be correctthen we insert
23
, not replaceSo, that how I assumed how it should work
You need to replace
{ .offset = 0, .length = 2, .payload = '01' }
with{ .offset = 0, .length = 4, .payload = '01' }
.It should work according to spec, irregardless of what is implemented now.
After
{ .offset = 0, .length = 4, .payload = '01' }
:[0][1][i][n][a][l][p][a][y][l][o][a][d]
After
{.offset = 4, .length = 0, .payload = '23' }
:[0][1][2][3][i][n][a][l][p][a][y][l][o][a][d]
Finally, I get your point. Alright, let me check
patcher
then, because I didn't take this fact in account when I was designing itI've checked
patcher
. It correctly works. So, I fixedPatchPayload
according to your suggestion - it's really perfectFor those who don't get the idea we have been discussing:
ε - empty character, it erases a character with the same position
I don't see where it is changed, you still have
length -= patchLength
and patchLength depends on the number of items you have read (n, err
)Um... sorry for the confusion - forgot to push after local rebase. Please, check this out
@ -0,0 +38,4 @@
}
func TestObjectPatcher(t *testing.T) {
t.Run("no split payload patch", func(t *testing.T) {
It seems this suit of tests could be less in size by using table tests pattern: we have payload, we have range and that's all.
What do you think?
Fixed
@ -164,6 +166,7 @@ const (
methodAPEManagerAddChain
methodAPEManagerRemoveChain
methodAPEManagerListChains
methodObjectPatch
@dkirillov do we need backwards compatibility in this field?
If no, @aarifullin, could you move it to the
object
group?It seems we have stringer for the type, and it is only used for metrics.
I suppose no
Fixed
6835affbb1
tof0a8e54735
f0a8e54735
to5b77cbdc79
5b77cbdc79
to5d58519253