Wrapper storage package #4453

AdoAdoAdo · 2022-09-12T08:36:32Z

Description of the reasoning behind the pull request

Refactoring dependencies to elrond-go-storage repo

Proposed Changes

Move all changes to a single storage adapter package
Move back to elrond-go the path manager and directory handler

codecov-commenter · 2022-09-12T08:44:24Z

Codecov Report

❗ No coverage uploaded for pull request base (feat/elrond-go-storage@3dd2132). Click here to learn what that means.
The diff coverage is n/a.

@@                    Coverage Diff                    @@
##             feat/elrond-go-storage    #4453   +/-   ##
=========================================================
  Coverage                          ?   73.04%           
=========================================================
  Files                             ?      638           
  Lines                             ?    83548           
  Branches                          ?        0           
=========================================================
  Hits                              ?    61031           
  Misses                            ?    17763           
  Partials                          ?     4754

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

refactor several dependencies

use latest storage

iulianpascalau · 2022-09-14T06:57:32Z

consensus/broadcast/delayedBroadcast.go

@@ -98,7 +98,7 @@ func NewDelayedBlockBroadcaster(args *ArgsDelayedBlockBroadcaster) (*delayedBloc
 		return nil, spos.ErrNilAlarmScheduler
 	}

-	cacheHeaders, err := lrucache.NewCache(sizeHeadersCache)
+	cacheHeaders, err := cache.NewLRUCache(sizeHeadersCache)


iulianpascalau · 2022-09-14T07:01:41Z

dataRetriever/txpool/memorytests/memory_test.go

+		if errClose != nil {
+			errMsg = errClose.Error()
+		}
+		panic(fmt.Sprintf("cannot close file: %s", errMsg))


I see this is old code, I think this line needs to be moved on L235, otherwise it will always panic

iulianpascalau · 2022-09-14T07:24:01Z

integrationTests/consensus/testInitializer.go

@@ -137,9 +136,9 @@ func createTestBlockChain() data.ChainHandler {
 }

 func createMemUnit() storage.Storer {
-	cache, _ := storageUnit.NewCache(storageUnit.CacheConfig{Type: storageUnit.LRUCache, Capacity: 10, Shards: 1, SizeInBytes: 0})
+	suCache, _ := storageunit.NewCache(storageunit.CacheConfig{Type: storageunit.LRUCache, Capacity: 10, Shards: 1, SizeInBytes: 0})


usually I name these variables something like cacheInstance in order to have a different name with the package

iulianpascalau · 2022-09-14T07:37:24Z

storage/database/db.go

+
+// NewLevelDB is a constructor for the leveldb persister
+// It creates the files in the location given as parameter
+func NewLevelDB(path string, batchDelaySeconds int, maxBatchSize int, maxOpenFiles int) (s *LevelDB, err error) {


in this case only, where we have method factories we might have had the returning parameter of type leveldb.DB rather than the alias. Can be left as it is, is just a suggestion to keep the type aliases number as low as possible.

right, removed

iulianpascalau · 2022-09-14T08:00:46Z

storage/storageunit/constants.go

+import "github.com/ElrondNetwork/elrond-go-storage/storageUnit"
+
+const (
+	LRUCache     = storageUnit.LRUCache


comments on exported items?

sstanculeanu · 2022-09-14T08:20:14Z

storage/errors.go

@@ -77,6 +70,30 @@ var ErrNilNodeTypeProvider = errors.New("nil node type provider")
 // ErrNilOldDataCleanerProvider signals that a nil old data cleaner provider has been provided
 var ErrNilOldDataCleanerProvider = errors.New("nil old data cleaner provider")

+// ErrNilCacher is raised when a nil cacher is provided


not used anywhere

sstanculeanu · 2022-09-14T08:22:55Z

storage/pathmanager/errors.go

+// IsNotFoundInStorageErr returns whether an error is a "not found in storage" error.
+// Currently, "item not found" storage errors are untyped (thus not distinguishable from others). E.g. see "pruningStorer.go".
+// As a workaround, we test the error message for a match.
+func IsNotFoundInStorageErr(err error) bool {


sstanculeanu · 2022-09-14T08:47:53Z

storage/constants.go

+)
+
+// MaxRetriesToCreateDB represents the maximum number of times to try to create DB if it failed
+const MaxRetriesToCreateDB = 10


as this is used only in the storage package and it also exists in elrond-go-storage, why not use the one from elrond-go-storage repo? this way we avoid forgetting to update one of them in one of the repos

same for SleepTimeBetweenCreateDBRetries

the constants from elrond-go-storage were removed:
https://github.com/ElrondNetwork/elrond-go-storage/pull/4/files

these 2 were moved to storageUnit.go
https://github.com/ElrondNetwork/elrond-go-storage/pull/4/files#diff-63b94d5e561760c6df458775f6a94633b032820bf3cb2360f5cf859cd63d73f3R65

Ah, right, forgot I moved these there.
Referenced for now the ones in storageUnit, but this will need to be refactored in a different PR so that we allow the consumer of the storage-repo to give these constants as arguments on creation instead.

ssd04

it looks like some of the interfaces in storage/interface.go are not being used like SerializedStoredData, LRUCacheHandler, ForEachItem, maybe we can remove them
also others like Cacher seems to be duplicated with storage repo, maybe use an alias?

gabi-vuls

System test passed.

AdoAdoAdo added 2 commits September 9, 2022 12:53

move external repo cache dependencies to storage adapter package

b287445

timecache dependencies update

6d038ca

AdoAdoAdo added 6 commits September 12, 2022 12:28

storage errors dependencies update

927071c

storage txcache dependencies update

9b5a7cb

storage unit dependencies update

b180e4c

storage memorydb dependencies update

b0c0349

move pathmanager and directory handler back to elrond-go

b12139a

refactor several dependencies

refactor immunitycache dependencies

a12cffa

use latest storage

AdoAdoAdo marked this pull request as ready for review September 13, 2022 15:00

iulianpascalau self-requested a review September 13, 2022 15:05

ssd04 self-requested a review September 13, 2022 19:21

AdoAdoAdo self-assigned this Sep 14, 2022

sstanculeanu self-requested a review September 14, 2022 07:48

iulianpascalau reviewed Sep 14, 2022

View reviewed changes

sstanculeanu reviewed Sep 14, 2022

View reviewed changes

ssd04 reviewed Sep 14, 2022

View reviewed changes

AdoAdoAdo added 2 commits September 14, 2022 16:11

cleanup unused constants, comments, types

5a31928

remove unused interfaces

0a022bc

iulianpascalau approved these changes Sep 14, 2022

View reviewed changes

ssd04 approved these changes Sep 14, 2022

View reviewed changes

sstanculeanu approved these changes Sep 14, 2022

View reviewed changes

gabi-vuls approved these changes Sep 15, 2022

View reviewed changes

gabi-vuls merged commit 013b2cf into feat/elrond-go-storage Sep 15, 2022

gabi-vuls deleted the wrapper-storage-package branch September 15, 2022 07:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wrapper storage package #4453

Wrapper storage package #4453

AdoAdoAdo commented Sep 12, 2022 •

edited

Loading

codecov-commenter commented Sep 12, 2022 •

edited

Loading

iulianpascalau Sep 14, 2022

iulianpascalau Sep 14, 2022

AdoAdoAdo Sep 14, 2022

iulianpascalau Sep 14, 2022

iulianpascalau Sep 14, 2022

AdoAdoAdo Sep 14, 2022

iulianpascalau Sep 14, 2022

AdoAdoAdo Sep 14, 2022

sstanculeanu Sep 14, 2022

sstanculeanu Sep 14, 2022

sstanculeanu Sep 14, 2022 •

edited

Loading

AdoAdoAdo Sep 14, 2022

sstanculeanu Sep 14, 2022 •

edited

Loading

AdoAdoAdo Sep 14, 2022

ssd04 left a comment

gabi-vuls left a comment

Wrapper storage package #4453

Wrapper storage package #4453

Conversation

AdoAdoAdo commented Sep 12, 2022 • edited Loading

Description of the reasoning behind the pull request

Proposed Changes

codecov-commenter commented Sep 12, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sstanculeanu Sep 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sstanculeanu Sep 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssd04 left a comment

Choose a reason for hiding this comment

gabi-vuls left a comment

Choose a reason for hiding this comment

AdoAdoAdo commented Sep 12, 2022 •

edited

Loading

codecov-commenter commented Sep 12, 2022 •

edited

Loading

sstanculeanu Sep 14, 2022 •

edited

Loading

sstanculeanu Sep 14, 2022 •

edited

Loading