From 9f0f3a4bef78a488a6096a641eede6f9ab5420de Mon Sep 17 00:00:00 2001 From: ssd04 Date: Mon, 4 Dec 2023 18:28:09 +0200 Subject: [PATCH 1/4] added persister factory interface --- storageUnit/storageunit.go | 32 +++++++++----------------------- 1 file changed, 9 insertions(+), 23 deletions(-) diff --git a/storageUnit/storageunit.go b/storageUnit/storageunit.go index a560f252..edcd39e8 100644 --- a/storageUnit/storageunit.go +++ b/storageUnit/storageunit.go @@ -16,9 +16,7 @@ import ( logger "github.com/multiversx/mx-chain-logger-go" "github.com/multiversx/mx-chain-storage-go/common" "github.com/multiversx/mx-chain-storage-go/fifocache" - "github.com/multiversx/mx-chain-storage-go/leveldb" "github.com/multiversx/mx-chain-storage-go/lrucache" - "github.com/multiversx/mx-chain-storage-go/memorydb" "github.com/multiversx/mx-chain-storage-go/monitoring" "github.com/multiversx/mx-chain-storage-go/types" ) @@ -289,8 +287,12 @@ func NewStorageUnit(c types.Cacher, p types.Persister) (*Unit, error) { return sUnit, nil } +type PersisterFactoryHandler interface { + Create(path string) (types.Persister, error) +} + // NewStorageUnitFromConf creates a new storage unit from a storage unit config -func NewStorageUnitFromConf(cacheConf CacheConfig, dbConf DBConfig) (*Unit, error) { +func NewStorageUnitFromConf(cacheConf CacheConfig, dbConf DBConfig, persisterFactory PersisterFactoryHandler) (*Unit, error) { var cache types.Cacher var db types.Persister var err error @@ -307,14 +309,7 @@ func NewStorageUnitFromConf(cacheConf CacheConfig, dbConf DBConfig) (*Unit, erro return nil, err } - argDB := ArgDB{ - DBType: dbConf.Type, - Path: dbConf.FilePath, - BatchDelaySeconds: dbConf.BatchDelaySeconds, - MaxBatchSize: dbConf.MaxBatchSize, - MaxOpenFiles: dbConf.MaxOpenFiles, - } - db, err = NewDB(argDB) + db, err = NewDB(persisterFactory, dbConf.FilePath) if err != nil { return nil, err } @@ -378,24 +373,15 @@ type ArgDB struct { } // NewDB creates a new database from database config -func NewDB(argDB ArgDB) (types.Persister, error) { +func NewDB(persisterFactory PersisterFactoryHandler, path string) (types.Persister, error) { var db types.Persister var err error for i := 0; i < MaxRetriesToCreateDB; i++ { - switch argDB.DBType { - case LvlDB: - db, err = leveldb.NewDB(argDB.Path, argDB.BatchDelaySeconds, argDB.MaxBatchSize, argDB.MaxOpenFiles) - case LvlDBSerial: - db, err = leveldb.NewSerialDB(argDB.Path, argDB.BatchDelaySeconds, argDB.MaxBatchSize, argDB.MaxOpenFiles) - case MemoryDB: - db = memorydb.New() - default: - return nil, common.ErrNotSupportedDBType - } + persister, err := persisterFactory.Create(path) if err == nil { - return db, nil + return persister, nil } // TODO: extract this in a parameter and inject it From 53909e7c1f0f6f46b46a126c0eae3c766c21b02f Mon Sep 17 00:00:00 2001 From: ssd04 Date: Tue, 5 Dec 2023 12:09:16 +0200 Subject: [PATCH 2/4] update storage unit tests --- storageUnit/storageunit.go | 5 +- storageUnit/storageunit_test.go | 70 +++++++++++++--------- testscommon/persisterFactoryHandlerMock.go | 45 ++++++++++++++ 3 files changed, 89 insertions(+), 31 deletions(-) create mode 100644 testscommon/persisterFactoryHandlerMock.go diff --git a/storageUnit/storageunit.go b/storageUnit/storageunit.go index edcd39e8..543b9deb 100644 --- a/storageUnit/storageunit.go +++ b/storageUnit/storageunit.go @@ -287,6 +287,7 @@ func NewStorageUnit(c types.Cacher, p types.Persister) (*Unit, error) { return sUnit, nil } +// PersisterFactoryHandler defines the behaviour of a component which is able to create persisters type PersisterFactoryHandler interface { Create(path string) (types.Persister, error) } @@ -378,10 +379,10 @@ func NewDB(persisterFactory PersisterFactoryHandler, path string) (types.Persist var err error for i := 0; i < MaxRetriesToCreateDB; i++ { - persister, err := persisterFactory.Create(path) + db, err = persisterFactory.Create(path) if err == nil { - return persister, nil + return db, nil } // TODO: extract this in a parameter and inject it diff --git a/storageUnit/storageunit_test.go b/storageUnit/storageunit_test.go index a0c8cdf0..45c43fb2 100644 --- a/storageUnit/storageunit_test.go +++ b/storageUnit/storageunit_test.go @@ -10,6 +10,7 @@ import ( "github.com/multiversx/mx-chain-storage-go/lrucache" "github.com/multiversx/mx-chain-storage-go/memorydb" "github.com/multiversx/mx-chain-storage-go/storageUnit" + "github.com/multiversx/mx-chain-storage-go/testscommon" "github.com/stretchr/testify/assert" ) @@ -254,14 +255,13 @@ func TestCreateCacheFromConfOK(t *testing.T) { } func TestCreateDBFromConfWrongType(t *testing.T) { - arg := storageUnit.ArgDB{ - DBType: "NotLvlDB", - Path: "test", - BatchDelaySeconds: 10, - MaxBatchSize: 10, - MaxOpenFiles: 10, - } - persister, err := storageUnit.NewDB(arg) + persisterFactory := testscommon.NewPersisterFactoryHandlerMock( + "NotLvlDB", + 10, + 10, + 10, + ) + persister, err := storageUnit.NewDB(persisterFactory, "test") assert.NotNil(t, err, "error expected") assert.Nil(t, persister, "persister expected to be nil, but got %s", persister) @@ -272,27 +272,29 @@ func TestCreateDBFromConfWrongFileNameLvlDB(t *testing.T) { t.Skip("this is not a short test") } - arg := storageUnit.ArgDB{ - DBType: storageUnit.LvlDB, - Path: "", - BatchDelaySeconds: 10, - MaxBatchSize: 10, - MaxOpenFiles: 10, - } - persister, err := storageUnit.NewDB(arg) + path := "" + persisterFactory := testscommon.NewPersisterFactoryHandlerMock( + storageUnit.LvlDB, + 10, + 10, + 10, + ) + + persister, err := storageUnit.NewDB(persisterFactory, path) assert.NotNil(t, err, "error expected") assert.Nil(t, persister, "persister expected to be nil, but got %s", persister) } func TestCreateDBFromConfLvlDBOk(t *testing.T) { - arg := storageUnit.ArgDB{ - DBType: storageUnit.LvlDB, - Path: t.TempDir(), - BatchDelaySeconds: 10, - MaxBatchSize: 10, - MaxOpenFiles: 10, - } - persister, err := storageUnit.NewDB(arg) + path := t.TempDir() + persisterFactory := testscommon.NewPersisterFactoryHandlerMock( + storageUnit.LvlDB, + 10, + 10, + 10, + ) + + persister, err := storageUnit.NewDB(persisterFactory, path) assert.Nil(t, err, "no error expected") assert.NotNil(t, persister, "valid persister expected but got nil") @@ -311,7 +313,9 @@ func TestNewStorageUnit_FromConfWrongCacheSizeVsBatchSize(t *testing.T) { MaxBatchSize: 11, BatchDelaySeconds: 1, MaxOpenFiles: 10, - }) + }, + testscommon.NewPersisterFactoryHandlerMock(storageUnit.LvlDB, 11, 1, 10), + ) assert.NotNil(t, err, "error expected") assert.Nil(t, storer, "storer expected to be nil but got %s", storer) @@ -328,7 +332,9 @@ func TestNewStorageUnit_FromConfWrongCacheConfig(t *testing.T) { BatchDelaySeconds: 1, MaxBatchSize: 1, MaxOpenFiles: 10, - }) + }, + testscommon.NewPersisterFactoryHandlerMock(storageUnit.LvlDB, 1, 1, 10), + ) assert.NotNil(t, err, "error expected") assert.Nil(t, storer, "storer expected to be nil but got %s", storer) @@ -341,7 +347,9 @@ func TestNewStorageUnit_FromConfWrongDBConfig(t *testing.T) { }, storageUnit.DBConfig{ FilePath: "Blocks", Type: "NotLvlDB", - }) + }, + testscommon.NewPersisterFactoryHandlerMock("NotLvlDB", 0, 0, 0), + ) assert.NotNil(t, err, "error expected") assert.Nil(t, storer, "storer expected to be nil but got %s", storer) @@ -357,7 +365,9 @@ func TestNewStorageUnit_FromConfLvlDBOk(t *testing.T) { MaxBatchSize: 1, BatchDelaySeconds: 1, MaxOpenFiles: 10, - }) + }, + testscommon.NewPersisterFactoryHandlerMock(storageUnit.LvlDB, 1, 1, 10), + ) assert.Nil(t, err, "no error expected but got %s", err) assert.NotNil(t, storer, "valid storer expected but got nil") @@ -375,7 +385,9 @@ func TestNewStorageUnit_ShouldWorkLvlDB(t *testing.T) { BatchDelaySeconds: 1, MaxBatchSize: 1, MaxOpenFiles: 10, - }) + }, + testscommon.NewPersisterFactoryHandlerMock(storageUnit.LvlDB, 1, 1, 10), + ) assert.Nil(t, err, "no error expected but got %s", err) assert.NotNil(t, storer, "valid storer expected but got nil") diff --git a/testscommon/persisterFactoryHandlerMock.go b/testscommon/persisterFactoryHandlerMock.go new file mode 100644 index 00000000..e3b60ab6 --- /dev/null +++ b/testscommon/persisterFactoryHandlerMock.go @@ -0,0 +1,45 @@ +package testscommon + +import ( + "github.com/multiversx/mx-chain-storage-go/common" + "github.com/multiversx/mx-chain-storage-go/leveldb" + "github.com/multiversx/mx-chain-storage-go/memorydb" + "github.com/multiversx/mx-chain-storage-go/storageUnit" + "github.com/multiversx/mx-chain-storage-go/types" +) + +type persisterFactoryHandlerMock struct { + dbType storageUnit.DBType + batchDelaySeconds int + maxBatchSize int + maxOpenFiles int +} + +// NewPersisterFactoryHandlerMock - +func NewPersisterFactoryHandlerMock(dbType storageUnit.DBType, batchDelaySeconds int, maxBatchSize int, maxOpenFiles int) *persisterFactoryHandlerMock { + return &persisterFactoryHandlerMock{ + dbType: dbType, + batchDelaySeconds: batchDelaySeconds, + maxBatchSize: maxBatchSize, + maxOpenFiles: maxOpenFiles, + } +} + +// Create - +func (mock *persisterFactoryHandlerMock) Create(path string) (types.Persister, error) { + switch mock.dbType { + case storageUnit.LvlDB: + return leveldb.NewDB(path, mock.batchDelaySeconds, mock.maxBatchSize, mock.maxOpenFiles) + case storageUnit.LvlDBSerial: + return leveldb.NewSerialDB(path, mock.batchDelaySeconds, mock.maxBatchSize, mock.maxOpenFiles) + case storageUnit.MemoryDB: + return memorydb.New(), nil + default: + return nil, common.ErrNotSupportedDBType + } +} + +// IsInterfaceNil - +func (mock *persisterFactoryHandlerMock) IsInterfaceNil() bool { + return mock == nil +} From 0943b1e3e63458710d529fb69acaf8efeedda7e2 Mon Sep 17 00:00:00 2001 From: ssd04 Date: Tue, 5 Dec 2023 12:12:00 +0200 Subject: [PATCH 3/4] added todo comment --- storageUnit/storageunit.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/storageUnit/storageunit.go b/storageUnit/storageunit.go index 543b9deb..c3c734ae 100644 --- a/storageUnit/storageunit.go +++ b/storageUnit/storageunit.go @@ -374,6 +374,8 @@ type ArgDB struct { } // NewDB creates a new database from database config +// TODO: refactor to integrate retries loop into persister factory; maybe implement persister +// factory separatelly in storage repo func NewDB(persisterFactory PersisterFactoryHandler, path string) (types.Persister, error) { var db types.Persister var err error From 873df9f557adae81613dce130ab6a801af6091e9 Mon Sep 17 00:00:00 2001 From: ssd04 Date: Tue, 5 Dec 2023 13:05:43 +0200 Subject: [PATCH 4/4] fixes after review --- storageUnit/storageunit.go | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/storageUnit/storageunit.go b/storageUnit/storageunit.go index c3c734ae..54c95353 100644 --- a/storageUnit/storageunit.go +++ b/storageUnit/storageunit.go @@ -3,6 +3,7 @@ package storageUnit import ( "encoding/base64" "encoding/json" + "errors" "fmt" "sync" "time" @@ -73,6 +74,9 @@ const MaxRetriesToCreateDB = 10 // SleepTimeBetweenCreateDBRetries represents the number of seconds to sleep between DB creates const SleepTimeBetweenCreateDBRetries = 5 * time.Second +// ErrNilPersisterFactory signals that a nil persister factory handler has been provided +var ErrNilPersisterFactory = errors.New("nil persister factory") + // UnitConfig holds the configurable elements of the storage unit type UnitConfig struct { CacheConf CacheConfig @@ -290,6 +294,7 @@ func NewStorageUnit(c types.Cacher, p types.Persister) (*Unit, error) { // PersisterFactoryHandler defines the behaviour of a component which is able to create persisters type PersisterFactoryHandler interface { Create(path string) (types.Persister, error) + IsInterfaceNil() bool } // NewStorageUnitFromConf creates a new storage unit from a storage unit config @@ -375,8 +380,12 @@ type ArgDB struct { // NewDB creates a new database from database config // TODO: refactor to integrate retries loop into persister factory; maybe implement persister -// factory separatelly in storage repo +// factory separatelly in storage repo func NewDB(persisterFactory PersisterFactoryHandler, path string) (types.Persister, error) { + if check.IfNil(persisterFactory) { + return nil, ErrNilPersisterFactory + } + var db types.Persister var err error