[db] implement Get/Put/Delete/Version() for BoltDBVersioned #4256
Conversation
dustinxie
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4256 +/- ##
==========================================
- Coverage 77.02% 76.76% -0.27%
==========================================
Files 342 347 +5
Lines 29406 29415 +9
==========================================
- Hits 22651 22581 -70
- Misses 5650 5737 +87
+ Partials 1105 1097 -8 ☔ View full report in Codecov by Sentry. |
db/db_versioned_util.go
Outdated
| return 0 | ||
| } | ||
|
|
||
| func (km *keyMeta) deleteInBetween(write, read uint64) (bool, error) { |
There was a problem hiding this comment.
naming func to notDeleteInBetween()?
db/db_versioned_util.go
Outdated
| return false, ErrDeleted | ||
| } | ||
| // delete and write fall on the same version, need to check further | ||
| // if it's write-after-delete or delete-after-write |
There was a problem hiding this comment.
For each singel version, only the last record (delete/put) can be retained, which can help avoid this.
db/db_versioned.go
Outdated
| return err | ||
| } | ||
| if version == km.lastVersion { | ||
| // write <key, nil> to indicate this is a delete-after-write |
There was a problem hiding this comment.
what if the two updates on same version:
del k
put k nil
There was a problem hiding this comment.
yes, but this will be a very rare corner case
- usually a key's value is some valid data
- if the user intends to use
nilas a special case, then he can use a special []byte value to avoid the corner case
dustinxie
force-pushed
the
version
branch
5 times, most recently
from
cc03631 to
987c36a
Compare
|
| // Put writes a <key, value> record | ||
| func (b *BoltDBVersioned) Put(ns string, version uint64, key, value []byte) error { | ||
| func (b *BoltDBVersioned) Put(version uint64, ns string, key, value []byte) error { |
There was a problem hiding this comment.
Do we really need to implement these single functionPut/Delete? I prefer only keep WriteBatch, b/c version update will be atomic, and also can simplify the impl
There was a problem hiding this comment.
discussed offline, it does not hurt to keep it
| // not allowed to perform write on an earlier version | ||
| return ErrInvalid | ||
| } | ||
| buf.Delete(ns, keyForDelete(key, version), fmt.Sprintf("failed to delete key %x", key)) |
| } | ||
| buf := batch.NewBatch() | ||
| buf.Put(ns, keyForDelete(key, version), nil, fmt.Sprintf("failed to delete key %x", key)) | ||
| buf.Delete(ns, keyForWrite(key, version), fmt.Sprintf("failed to delete key %x", key)) |
There was a problem hiding this comment.
discussed offline
- it is to keep only the last operation on the key
- i recall in the original POC, we have cases where the same key is deleted, then written in the batch at around height 400k. So it is a valid request (to delete then write on the same height). We'll allow this behavior (and also write then delete on the same height)
| lifecycle.StartStopper | ||
|
|
||
| // Put insert or update a record identified by (namespace, key) |
There was a problem hiding this comment.
Why to modify the interface this way? I would recommend that the version should be for all keys rather than individual keys. Do we have a use case that requires this? I think the original interface design is easier for integration, or will there be a wrapper to encapsulate it?
There was a problem hiding this comment.
yes, KvVersioned is the new wrapper to encapsulate it
|
Description
as title, implement 4 methos for versioned DB.
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: