[#118] node: add unit concurrent tests for blobstor #233
|
@ -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
|
||||
carpawell
commented
kinda magic number to me: the const from the @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?
aarifullin
commented
Agree. That constant in Tree affects the concurrent write. If concurrent writers are > 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
carpawell
commented
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
carpawell
commented
why same in why `defer` here? looks useless without multiple `return`
same in `testPutFileExistsError`
aarifullin
commented
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
ale64bit
commented
the zero
the zero `WaitGroup` is valid:
```
var wg sync.WaitGroup
```
aarifullin
commented
Fixed 👍 Fixed 👍
|
||||
for i := 0; i < concurrentPutCount; i++ {
|
||||
ale64bit marked this conversation as resolved
Outdated
ale64bit
commented
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?
aarifullin
commented
It shouldn't be here. I just wanted to try if this expression would be valid and it worked out :) 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...`
ale64bit
commented
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
carpawell
commented
have never seen such a [0; n) range -- live and learn :) but may the 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
|
||||
}()
|
||||
aarifullin
commented
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
dstepanov-yadro
commented
Maybe add check that object is not exists? Maybe add check that object is not exists?
aarifullin
commented
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
dstepanov-yadro
commented
Yes, it's true. I'm talking about the fact that after deletion there should be no object exist. Maybe > 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.
aarifullin
commented
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
carpawell
commented
how about:
and do not pass how about:
```
go func() {
wg.Add(1)
testDelete(...)
wg.Done()
}()
```
and do not pass `wg` to the testing funcs?
aarifullin
commented
Sounds reasonble!
Otherwise 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`)
carpawell
commented
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)
|
||||
})
|
||||
}
|
||||
|
|
@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?