-
Notifications
You must be signed in to change notification settings - Fork 180
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Storage Refactor] Init pebble DB in scaffold. #6949
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,6 +31,7 @@ import ( | |
"github.com/onflow/flow-go/admin/commands/common" | ||
storageCommands "github.com/onflow/flow-go/admin/commands/storage" | ||
"github.com/onflow/flow-go/cmd/build" | ||
"github.com/onflow/flow-go/cmd/scaffold" | ||
"github.com/onflow/flow-go/config" | ||
"github.com/onflow/flow-go/consensus/hotstuff/persister" | ||
"github.com/onflow/flow-go/fvm/initialize" | ||
|
@@ -164,6 +165,7 @@ func (fnb *FlowNodeBuilder) BaseFlags() { | |
fnb.flags.StringVar(&fnb.BaseConfig.BindAddr, "bind", defaultConfig.BindAddr, "address to bind on") | ||
fnb.flags.StringVarP(&fnb.BaseConfig.BootstrapDir, "bootstrapdir", "b", defaultConfig.BootstrapDir, "path to the bootstrap directory") | ||
fnb.flags.StringVarP(&fnb.BaseConfig.datadir, "datadir", "d", defaultConfig.datadir, "directory to store the public database (protocol state)") | ||
fnb.flags.StringVar(&fnb.BaseConfig.pebbleDir, "pebble-dir", defaultConfig.pebbleDir, "directory to store the public pebble database (protocol state)") | ||
fnb.flags.StringVar(&fnb.BaseConfig.secretsdir, "secretsdir", defaultConfig.secretsdir, "directory to store private database (secrets)") | ||
fnb.flags.StringVarP(&fnb.BaseConfig.level, "loglevel", "l", defaultConfig.level, "level for logging output") | ||
fnb.flags.Uint32Var(&fnb.BaseConfig.debugLogLimit, "debug-log-limit", defaultConfig.debugLogLimit, "max number of debug/trace log events per second") | ||
|
@@ -1056,13 +1058,7 @@ func (fnb *FlowNodeBuilder) initProfiler() error { | |
return nil | ||
} | ||
|
||
func (fnb *FlowNodeBuilder) initDB() error { | ||
|
||
// if a db has been passed in, use that instead of creating one | ||
if fnb.BaseConfig.db != nil { | ||
fnb.DB = fnb.BaseConfig.db | ||
return nil | ||
} | ||
func (fnb *FlowNodeBuilder) initBadgerDB() error { | ||
|
||
// Pre-create DB path (Badger creates only one-level dirs) | ||
err := os.MkdirAll(fnb.BaseConfig.datadir, 0700) | ||
|
@@ -1110,6 +1106,17 @@ func (fnb *FlowNodeBuilder) initDB() error { | |
return nil | ||
} | ||
|
||
func (fnb *FlowNodeBuilder) initPebbleDB() error { | ||
db, closer, err := scaffold.InitPebbleDB(fnb.BaseConfig.pebbleDir) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
fnb.PebbleDB = db | ||
fnb.ShutdownFunc(closer.Close) | ||
return nil | ||
} | ||
|
||
func (fnb *FlowNodeBuilder) initSecretsDB() error { | ||
|
||
// if the secrets DB is disabled (only applicable for Consensus Follower, | ||
|
@@ -1889,9 +1896,7 @@ func WithBindAddress(bindAddress string) Option { | |
|
||
func WithDataDir(dataDir string) Option { | ||
return func(config *BaseConfig) { | ||
if config.db == nil { | ||
config.datadir = dataDir | ||
} | ||
config.datadir = dataDir | ||
} | ||
} | ||
|
||
|
@@ -1925,14 +1930,6 @@ func WithLogLevel(level string) Option { | |
} | ||
} | ||
|
||
// WithDB takes precedence over WithDataDir and datadir will be set to empty if DB is set using this option | ||
func WithDB(db *badger.DB) Option { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not being used anywhere There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this was added to support the consensus follower There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you explain it a bit more? I can't find anywhere using it. I deleted it, and tested localnet, it ran fine. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if you use a consensus follower as a library, you may want to use the same database for the blocks/etc and you app. so there's an option to provide your own db instead of the scaffold always creating one There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see. We need to discuss this, because it creates extra complexity, as we are transitioning to pebble DB. |
||
return func(config *BaseConfig) { | ||
config.db = db | ||
config.datadir = "" | ||
} | ||
} | ||
|
||
// FlowNode creates a new Flow node builder with the given name. | ||
func FlowNode(role string, opts ...Option) *FlowNodeBuilder { | ||
config := DefaultBaseConfig() | ||
|
@@ -2038,7 +2035,11 @@ func (fnb *FlowNodeBuilder) onStart() error { | |
return err | ||
} | ||
|
||
if err := fnb.initDB(); err != nil { | ||
if err := fnb.initBadgerDB(); err != nil { | ||
return err | ||
} | ||
|
||
if err := fnb.initPebbleDB(); err != nil { | ||
return err | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
package scaffold | ||
|
||
import ( | ||
"fmt" | ||
"io" | ||
"os" | ||
|
||
"github.com/cockroachdb/pebble" | ||
|
||
pebblestorage "github.com/onflow/flow-go/storage/pebble" | ||
) | ||
|
||
func InitPebbleDB(dir string) (*pebble.DB, io.Closer, error) { | ||
// Pre-create DB path | ||
err := os.MkdirAll(dir, 0700) | ||
if err != nil { | ||
return nil, nil, fmt.Errorf("could not create pebble db (path: %s): %w", dir, err) | ||
} | ||
|
||
db, err := pebblestorage.OpenDefaultPebbleDB(dir) | ||
if err != nil { | ||
return nil, nil, err | ||
} | ||
|
||
return db, db, nil | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
package scaffold | ||
|
||
import ( | ||
"testing" | ||
|
||
"github.com/stretchr/testify/require" | ||
|
||
"github.com/onflow/flow-go/utils/unittest" | ||
) | ||
|
||
func TestInitPebbleDB(t *testing.T) { | ||
unittest.RunWithTempDir(t, func(dir string) { | ||
_, closer, err := InitPebbleDB(dir) | ||
require.NoError(t, err) | ||
require.NoError(t, closer.Close()) | ||
}) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this was used for the consensus follower
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why we need both
WithDB
andWithDataDir
. I think we should keep only one, no?