feat: add generic KV interfaces + Pebble adapter#2666
Conversation
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
5e94e07 to
e63359f
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2666 +/- ##
==========================================
+ Coverage 43.73% 43.82% +0.08%
==========================================
Files 1906 1908 +2
Lines 159040 159047 +7
==========================================
+ Hits 69555 69695 +140
+ Misses 83076 82949 -127
+ Partials 6409 6403 -6
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
- Introduce backend-agnostic DB/Batch/Iterator interfaces and ErrNotFound. - Add pebbledb adapter implementing the db_engine interfaces. - Remove legacy pebbledb/kv implementation. - Fix pebbledb mvcc cache lifecycle (no defer Unref; Unref on Close).
e63359f to
36df3bf
Compare
26605b5 to
abdd1f2
Compare
sei-db/db_engine/types.go
Outdated
| // - Callers MUST close the closer when done. | ||
| // - If the value must outlive the closer, callers must copy it (e.g. bytes.Clone). | ||
| type DB interface { | ||
| Get(key []byte) (value []byte, closer io.Closer, err error) |
There was a problem hiding this comment.
I think it's better not to return closer to the user? From design perspective, we should probably handle it for the user
There was a problem hiding this comment.
the current design is for zero-copy reads. If the caller needs to hold the value beyond the closer's lifetime, they clone it themselves. This avoids unnecessary copies when the caller only needs a quick read. however, if we prefer the simplicity over perf, we can change to always clone internally.
also we will wrap it on the flatkv level
There was a problem hiding this comment.
updated to copy inside without return closer
sei-db/db_engine/pebbledb/db.go
Outdated
| cache := pebble.NewCache(1024 * 1024 * 512) // 512MB cache | ||
|
|
||
| var db *pebble.DB | ||
| defer cleanupOnError(&err, |
There was a problem hiding this comment.
I think the original cache is correct.
- pebble.NewCache() - Creates cache with refcount = 1
- pebble.Open(dbPath, opts) - PebbleDB internally calls cache.Ref(), incrementing refcount to 2
- defer cache.Unref() - Runs when OpenDB returns, decrementing refcount back to 1
- Later: db.Close() - PebbleDB internally calls cache.Unref(), decrementing to 0 → cache is freed
There was a problem hiding this comment.
oh pebble.Open() also increase 1, will check thx
There was a problem hiding this comment.
will revert the change
- Introduce backend-agnostic DB/Batch/Iterator interfaces and ErrNotFound. - Add pebbledb adapter implementing the db_engine interfaces. - Remove legacy pebbledb/kv implementation. ## Describe your changes and provide context ## Testing performed to validate your change
Describe your changes and provide context
Testing performed to validate your change