[#118] node: add unit concurrent tests for blobstor #233

Merged
fyrchik merged 1 commit from aarifullin/frostfs-node:feature/blobstor_concurrent_tests into master 2023-04-19 10:22:52 +00:00

View file

@ -2,6 +2,7 @@ package blobstor
import (
"path/filepath"
"sync"
"testing"
"git.frostfs.info/TrueCloudLab/frostfs-node/pkg/core/object"
@ -173,3 +174,156 @@ func TestBlobstor_needsCompression(t *testing.T) {
require.False(t, b.NeedsCompression(obj))
})
}
func TestConcurrentPut(t *testing.T) {
dir := t.TempDir()
const (
smallSizeLimit = 512
// concurrentPutCount is fstree implementation specific
concurrentPutCount = 5

@fyrchik
I've got this from this constant.
It looks like the constant sets the restriction for the concurent putting. Should we get this documented somehow?

@fyrchik I've got this from this [constant](https://git.frostfs.info/TrueCloudLab/frostfs-node/src/branch/master/pkg/local_object_storage/blobstor/fstree/fstree.go#L262). It looks like the constant sets the restriction for the concurent putting. Should we get this documented somehow?

kinda magic number to me: the const from the FSTree may vary independently

@fyrchik, what problem cases are expected to be tested by that PR?

kinda magic number to me: the const from the `FSTree` may vary independently @fyrchik, what problem cases are expected to be tested by that PR?

kinda magic number to me: the const from the FSTree may vary independently

Agree. That constant in Tree affects the concurrent write. If concurrent writers are > concurrentPutCount, then errors occur

> kinda magic number to me: the const from the FSTree may vary independently Agree. That constant in Tree affects the concurrent write. If concurrent writers are `> concurrentPutCount`, then errors occur
)
blobStor := New(
carpawell marked this conversation as resolved Outdated

not used var here and below?

not used var here and below?
WithStorages(defaultStorages(dir, smallSizeLimit)))
require.NoError(t, blobStor.Open(false))
require.NoError(t, blobStor.Init())
testGet := func(t *testing.T, b *BlobStor, obj *objectSDK.Object) {
res, err := b.Get(common.GetPrm{Address: object.AddressOf(obj)})
require.NoError(t, err)
require.Equal(t, obj, res.Object)
}
testPut := func(t *testing.T, b *BlobStor, obj *objectSDK.Object) {
var prm common.PutPrm
prm.Object = obj
_, err := b.Put(prm)
require.NoError(t, err)
}
carpawell marked this conversation as resolved Outdated

why defer here? looks useless without multiple return

same in testPutFileExistsError

why `defer` here? looks useless without multiple `return` same in `testPutFileExistsError`

wg is no longer used in the such methods

wg is no longer used in the such methods
testPutFileExistsError := func(t *testing.T, b *BlobStor, obj *objectSDK.Object) {
var prm common.PutPrm
prm.Object = obj
if _, err := b.Put(prm); err != nil {
require.ErrorContains(t, err, "file exists")
}
}
t.Run("put the same big object", func(t *testing.T) {
bigObj := testObject(smallSizeLimit * 2)
var wg sync.WaitGroup
ale64bit marked this conversation as resolved Outdated

the zero WaitGroup is valid:

var wg sync.WaitGroup
the zero `WaitGroup` is valid: ``` var wg sync.WaitGroup ```

Fixed 👍

Fixed 👍
for i := 0; i < concurrentPutCount; i++ {
ale64bit marked this conversation as resolved Outdated

cute loop. But why? Just because it's slightly shorter to type or is there another reason?

cute loop. But why? Just because it's slightly shorter to type or is there another reason?

It shouldn't be here. I just wanted to try if this expression would be valid and it worked out :)
I'll replace it by for i:=0...

It shouldn't be here. I just wanted to try if this expression would be valid and it worked out :) I'll replace it by `for i:=0...`

It certainly works. It's probably useful for code-golfing :) first time I see it, so TIL.

It certainly works. It's probably useful for code-golfing :) first time I see it, so TIL.
wg.Add(1)
go func() {
testPut(t, blobStor, bigObj)
wg.Done()
}()
}
wg.Wait()
testGet(t, blobStor, bigObj)
})
t.Run("put the same big object with error", func(t *testing.T) {
bigObj := testObject(smallSizeLimit * 2)
var wg sync.WaitGroup
for i := 0; i < concurrentPutCount+1; i++ {
wg.Add(1)
go func() {
testPutFileExistsError(t, blobStor, bigObj)
wg.Done()
carpawell marked this conversation as resolved Outdated

have never seen such a [0; n) range -- live and learn :)

but may the for i := 0; ...; i++ be more expected by a regular GO reader? also, your way should allocate more memory for nothing

have never seen such a [0; n) range -- live and learn :) but may the `for i := 0; ...; i++` be more expected by a regular GO reader? also, your way should allocate more memory for nothing
}()

No more good useful scenario came to my mind apart from the two ones. Would be glad to listen to your ideas

No more good useful scenario came to my mind apart from the two ones. Would be glad to listen to your ideas
}
wg.Wait()
testGet(t, blobStor, bigObj)
})
t.Run("put the same small object", func(t *testing.T) {
smallObj := testObject(smallSizeLimit / 2)
var wg sync.WaitGroup
for i := 0; i < concurrentPutCount; i++ {
wg.Add(1)
go func() {
testPut(t, blobStor, smallObj)
wg.Done()
}()
}
wg.Wait()
testGet(t, blobStor, smallObj)
})
}
func TestConcurrentDelete(t *testing.T) {
dir := t.TempDir()
const smallSizeLimit = 512
dstepanov-yadro marked this conversation as resolved Outdated

Maybe add check that object is not exists?

Maybe add check that object is not exists?

There are many ways to check if an object exists. But for me it's more logically correct to check the error from deletion because I check concurrent deletes

There are many ways to check if an object exists. But for me it's more logically correct to check the error from deletion because I check concurrent deletes

to check the error from deletion because I check concurrent deletes

Yes, it's true. I'm talking about the fact that after deletion there should be no object exist. Maybe delete does not return an error, but the object is not deleted.

> to check the error from deletion because I check concurrent deletes Yes, it's true. I'm talking about the fact that after deletion there should be no object exist. Maybe `delete` does not return an error, but the object is not deleted.

Maybe delete does not return an error, but the object is not deleted

You're right. That must be checked. I've added check about object's existence

> Maybe delete does not return an error, but the object is not deleted You're right. That must be checked. I've added check about object's existence
blobStor := New(
WithStorages(defaultStorages(dir, smallSizeLimit)))
require.NoError(t, blobStor.Open(false))
require.NoError(t, blobStor.Init())
testPut := func(t *testing.T, b *BlobStor, obj *objectSDK.Object) {
var prm common.PutPrm
prm.Object = obj
_, err := b.Put(prm)
require.NoError(t, err)
}
testDelete := func(t *testing.T, b *BlobStor, obj *objectSDK.Object) {
var prm common.DeletePrm
prm.Address = object.AddressOf(obj)
if _, err := b.Delete(prm); err != nil {
require.ErrorContains(t, err, "object not found")
}
}
testDeletedExists := func(t *testing.T, b *BlobStor, obj *objectSDK.Object) {
var prm common.ExistsPrm
prm.Address = object.AddressOf(obj)
res, err := b.Exists(prm)
require.NoError(t, err)
require.False(t, res.Exists)
}
t.Run("delete the same big object", func(t *testing.T) {
bigObj := testObject(smallSizeLimit * 2)
testPut(t, blobStor, bigObj)
var wg sync.WaitGroup
for i := 0; i < 2; i++ {
wg.Add(1)
go func() {
testDelete(t, blobStor, bigObj)
wg.Done()
}()
}
carpawell marked this conversation as resolved Outdated

how about:

go func() {
    wg.Add(1)
    testDelete(...)
    wg.Done()
}()

and do not pass wg to the testing funcs?

how about: ``` go func() { wg.Add(1) testDelete(...) wg.Done() }() ``` and do not pass `wg` to the testing funcs?

Sounds reasonble!
But with one fix:

wg.Add(1)
go func() {
    testDelete(...)
    wg.Done()
}()

Otherwise wg.Wait() can be passed meanwhile goroutine has not been run yet (no wg.Add)

Sounds reasonble! But with one fix: ``` wg.Add(1) go func() { testDelete(...) wg.Done() }() ``` Otherwise `wg.Wait()` can be passed meanwhile goroutine has not been run yet (no `wg.Add`)

But with one fix

oh, yeah, for sure. fast browser programming may lead to stupid bugs

> But with one fix oh, yeah, for sure. fast browser programming may lead to stupid bugs
wg.Wait()
testDeletedExists(t, blobStor, bigObj)
})
t.Run("delete the same small object", func(t *testing.T) {
smallObj := testObject(smallSizeLimit / 2)
testPut(t, blobStor, smallObj)
var wg sync.WaitGroup
for i := 0; i < 2; i++ {
wg.Add(1)
go func() {
testDelete(t, blobStor, smallObj)
wg.Done()
}()
}
wg.Wait()
testDeletedExists(t, blobStor, smallObj)
})
}