[db] implement CommitToDB() for BoltDBVersioned #4517
Conversation
dustinxie
requested review from
CoderZhi,
Liuhaai and
envestcc
as code owners
db/db_versioned.go
Outdated
| // CommitToDB write a batch to DB, where the batch can contain keys for | ||
| // both versioned and non-versioned namespace | ||
| func (b *BoltDBVersioned) CommitToDB(version uint64, vns map[string]bool, kvsb batch.KVStoreBatch) error { | ||
| vnsize, ve, nve, err := dedup(vns, kvsb) |
There was a problem hiding this comment.
that's doable, then the interface would become
(version uint64, vnsize map[string]int, ve, nve []*batch.WriteInfo)
feels a bit clumsy and too low-level, the current interface is cleaner IMO
| ) | ||
| // get bucket | ||
| bucket, ok := buckets[ns] | ||
| if !ok { |
There was a problem hiding this comment.
if ok == false, panic, because of https://github.com/iotexproject/iotex-core/pull/4517/files#diff-dd423fbb3d496fc9cbbda508288b69c003928d4937de168e9cf69be774f8f19fR224
There was a problem hiding this comment.
thanks, good check, fixed
db/db_versioned.go
Outdated
| switch write.WriteType() { | ||
| case batch.Put: | ||
| if bytes.Equal(key, _minKey) { | ||
| // create namespace |
There was a problem hiding this comment.
upon the first write to the namespace, it will create the metadata, updated the comment
db/db_versioned.go
Outdated
| continue | ||
| } | ||
| // wrong-size key should be caught in dedup(), but check anyway | ||
| if vnsize[ns] != len(key) { |
There was a problem hiding this comment.
There was a problem hiding this comment.
there are if conditions (like L289, L309) that this check won't apply, so do it here
db/db_versioned.go
Outdated
| if val := bucket.Get(_minKey); val == nil { | ||
| // namespace not created yet | ||
| vn = &versionedNamespace{ | ||
| keyLen: uint32(size), |
There was a problem hiding this comment.
could you give an example of using different keyLen for current use case?
There was a problem hiding this comment.
yes, for Account namespace, keyLen = 20, for Contract, keyLen = 32
db/db_versioned.go
Outdated
| nonDBErr = true | ||
| return ErrInvalid | ||
| } | ||
| if err = bucket.Put(keyForWrite(key, version), val); err != nil { |
db/db_versioned.go
Outdated
| keyLen: uint32(size), | ||
| } | ||
| ve = append(ve, batch.NewWriteInfo( | ||
| batch.Put, ns, _minKey, vn.serialize(), |
There was a problem hiding this comment.
what's the advantage of using _minKey, instead of a system namespace to store ns config as meta data?
There was a problem hiding this comment.
when check every ns, the config is stored within itself, so each namespace is self-contained, and avoid need for an extra system namespace
dustinxie
force-pushed
the
commitodb
branch
3 times, most recently
from
7e35527 to
cac2874
Compare
db/db_versioned.go
Outdated
| } | ||
|
|
||
| // Put writes a <key, value> record | ||
| func (b *BoltDBVersioned) Put(version uint64, ns string, key, value []byte) error { | ||
| if !b.db.IsReady() { | ||
| return ErrDBNotStarted | ||
| } | ||
| if _, ok := b.vns[ns]; !ok { |
There was a problem hiding this comment.
could we use vn returned from vn, ok := b.vns[ns] for line 112, so we don't need to read from disk every time?
db/db_versioned.go
Outdated
| @@ -110,11 +135,17 @@ func (b *BoltDBVersioned) Get(version uint64, ns string, key []byte) ([]byte, er | |||
| if !b.db.IsReady() { | |||
| return nil, ErrDBNotStarted | |||
| } | |||
| if _, ok := b.vns[ns]; !ok { | |||
db/db_versioned.go
Outdated
| @@ -161,6 +192,9 @@ func (b *BoltDBVersioned) Delete(version uint64, ns string, key []byte) error { | |||
| if !b.db.IsReady() { | |||
| return ErrDBNotStarted | |||
| } | |||
| if _, ok := b.vns[ns]; !ok { | |||
db/db_versioned.go
Outdated
| @@ -184,6 +221,9 @@ func (b *BoltDBVersioned) Version(ns string, key []byte) (uint64, error) { | |||
| if !b.db.IsReady() { | |||
| return 0, ErrDBNotStarted | |||
| } | |||
| if _, ok := b.vns[ns]; !ok { | |||
db/db_versioned.go
Outdated
| val := bucket.Get(_minKey) | ||
| if val == nil { | ||
| // namespace not created yet | ||
| nonDBErr = true | ||
| return errors.Wrapf(ErrInvalid, "namespace %s has not been added", ns) | ||
| } | ||
| vn, err := deserializeVersionedNamespace(val) |
There was a problem hiding this comment.
could we use b.vns as well?
|
Description
as title.
Fixes #(issue)
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Test Configuration:
Checklist: