[db] implement CommitToDB() for BoltDBVersioned #4517
Conversation
dustinxie
requested review from
CoderZhi,
Liuhaai and
envestcc
as code owners
| return b.commitToDB(version, vnsize, ve, nve) | ||
| } | ||
|
|
||
| func (b *BoltDBVersioned) commitToDB(version uint64, vnsize map[string]int, ve, nve []*batch.WriteInfo) error { |
There was a problem hiding this comment.
can simplify this function, it's a bit too long and difficult to understand.
There was a problem hiding this comment.
all these logic need to be in db.Update(), i can go over to explain if you have any questions
There was a problem hiding this comment.
I guess what @envestcc means is:
- create at least three functions: createNamespace, processVE, and processNVE
- pass
txas parameter into these three functions
moreover, we may discard dedup, then the function will look like:
loop number of retries:
for each entry in batch:
if the entry is in vns:
if the ns hasn't been created yet:
call createNamespace
call processVE
else:
call processNVE
| // 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
| return b.commitToDB(version, vnsize, ve, nve) | ||
| } | ||
|
|
||
| func (b *BoltDBVersioned) commitToDB(version uint64, vnsize map[string]int, ve, nve []*batch.WriteInfo) error { |
There was a problem hiding this comment.
I guess what @envestcc means is:
- create at least three functions: createNamespace, processVE, and processNVE
- pass
txas parameter into these three functions
moreover, we may discard dedup, then the function will look like:
loop number of retries:
for each entry in batch:
if the entry is in vns:
if the ns hasn't been created yet:
call createNamespace
call processVE
else:
call processNVE
| if entryKeySet[k] { | ||
| continue | ||
| } | ||
| if writeType == batch.Put { |
There was a problem hiding this comment.
still cann't understand why only set Put in entryKeySet?
|
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: