Support non-single DB migration #954
Conversation
KimKyungup
requested review from
ehnuje,
jeongkyun-oh,
joowon-byun and
yoomee1313
as code owners
storage/database/db_migration.go
Outdated
| // make a report | ||
| if fetched%reportCycle == 0 { | ||
| if fetched%(IdealBatchSize*20) == 0 { |
There was a problem hiding this comment.
Why did you change reportCycle to IdealBatchSize*20?
There was a problem hiding this comment.
The log are too much. So I changed it.
There was a problem hiding this comment.
If so, how about increasing reportCycle as it is not used now?
storage/database/db_migration.go
Outdated
|
|
||
| srcIter.Release() | ||
| if err := srcIter.Error(); err != nil { // any accumulated error from iterator | ||
| return errors.Wrap(err, "failed to iterate") |
There was a problem hiding this comment.
errors.Wrap returns an error annotating err with a stack trace.
Do we need stack trace here?
// Wrap returns an error annotating err with a stack trace
// at the point Wrap is called, and the supplied message.
// If err is nil, Wrap returns nil.
func Wrap(err error, message string) error {
if err == nil {
return nil
}
err = &withMessage{
cause: err,
msg: message,
}
return &withStack{
err,
callers(),
}
}
There was a problem hiding this comment.
The stack trace is returned when +v is used in format. It is not returned in Klaytn log.
errors.Wrap can be replaced with errors.WithMessage which is errors.Wrap without the stack trace.
There was a problem hiding this comment.
@winnie-byun If we don't print out stack trace in Klaytn, I think there's no need to use errors.Wrap instead of erros.WithMessage. How do you think?
There was a problem hiding this comment.
I think both ways are fine :)
There was a problem hiding this comment.
| return errors.Wrap(err, "failed to iterate") | |
| return errors.withMessage(err, "failed to iterate") |
Co-authored-by: yumiel yoomee1313 <yumiel.ko@groundx.xyz>
Co-authored-by: jk-jeongkyun <45347815+jeongkyun-oh@users.noreply.github.com>
storage/database/db_migration.go
Outdated
| // make a report | ||
| if fetched%reportCycle == 0 { | ||
| if fetched%(IdealBatchSize*20) == 0 { |
There was a problem hiding this comment.
If so, how about increasing reportCycle as it is not used now?
storage/database/sharded_database.go
Outdated
| @@ -52,7 +52,7 @@ type sdbBatchResult struct { | |||
| // newShardedDB creates database with numShards shards, or partitions. | |||
| // The type of database is specified DBConfig.DBType. | |||
| func newShardedDB(dbc *DBConfig, et DBEntryType, numShards uint) (*shardedDB, error) { | |||
| const numShardsLimit = 16 | |||
| const numShardsLimit = 256 | |||
There was a problem hiding this comment.
How about moving this variable outside the function to package level constant?
Co-authored-by: Winnie (Joowon) <joowon7508@gmail.com>
|
@ehnuje @jeongkyun-oh PTAL |
Proposed changes
This PR implements the non-single DB migration like LocalDB to LocalDB/RemoteDB.
This can migrate a DB to a DB which has different state trie shards like 4 shards to 16 shards.
Minor change
Types of changes
Please put an x in the boxes related to your change.
Checklist
Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.
$ make test)Related issues
Further comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...