expose memtable flush #72
Conversation
This can be used to avoid log replays on startup, for example
rocksdb/rocksdb.nim
Outdated
| @@ -465,6 +465,22 @@ proc releaseSnapshot*(db: RocksDbRef, snapshot: SnapshotRef) = | |||
| rocksdb_release_snapshot(db.cPtr, snapshot.cPtr) | |||
| snapshot.setClosed() | |||
|
|
|||
| proc flush*(db: RocksDbRef, cfs: openArray[ColFamilyHandleRef]): RocksDBResult[void] = | |||
| withLock(db.lock): | |||
There was a problem hiding this comment.
The c rocksdb type is thread safe so we shouldn't need to lock here as we don't update any fields in the RocksDbRef.
rocksdb/rocksdb.nim
Outdated
| @@ -465,6 +465,22 @@ proc releaseSnapshot*(db: RocksDbRef, snapshot: SnapshotRef) = | |||
| rocksdb_release_snapshot(db.cPtr, snapshot.cPtr) | |||
| snapshot.setClosed() | |||
|
|
|||
| proc flush*(db: RocksDbRef, cfs: openArray[ColFamilyHandleRef]): RocksDBResult[void] = | |||
| withLock(db.lock): | |||
| if not db.isClosed(): | |||
There was a problem hiding this comment.
I would make this isClosed check an assertion because we should never try flushing a closed database. It would be a bug/developer error if so.
rocksdb/rocksdb.nim
Outdated
| if not db.isClosed(): | ||
| var cfs = cfs.mapIt(it.cPtr) | ||
| var errors: cstring | ||
| var opts = rocksdb_flushoptions_create() |
There was a problem hiding this comment.
How often do we plan to call flush? If it gets called frequently then perhaps we should create the flush options when starting the database and hold a pointer to it and we can free when closing the database to reduce the number of allocations for performance reasons.
rocksdb/rocksdb.nim
Outdated
| @@ -465,6 +465,22 @@ proc releaseSnapshot*(db: RocksDbRef, snapshot: SnapshotRef) = | |||
| rocksdb_release_snapshot(db.cPtr, snapshot.cPtr) | |||
| snapshot.setClosed() | |||
|
|
|||
| proc flush*(db: RocksDbRef, cfs: openArray[ColFamilyHandleRef]): RocksDBResult[void] = | |||
There was a problem hiding this comment.
@arnetheduck Is the goal simply to flush all the opened column families? If so, I'll remove the cfs parameter and just flush all the column familes in ColFamilyTableRef
There was a problem hiding this comment.
for some reason, rocksdb exposes it like this - I think we should strive for "rough api equivalence", ie since rocksdb offers the option for specific columns, we should just follow along. this applies to everything in the library.
There was a problem hiding this comment.
The Java library has 3 alternatives: https://javadoc.io/doc/org.rocksdb/rocksdbjni/9.6.1/org/rocksdb/RocksDB.html#flush-org.rocksdb.FlushOptions-
I can do something similar if we want to have more options. One of the functions has no cf parameter which implies flushing all opened column families.
There was a problem hiding this comment.
One of the functions has no cf parameter which implies flushing all opened column families.
I'd verify this, ie I think it flushes just the default CF and none of the custom ones.
There was a problem hiding this comment.
Yes actually, you are correct. That one is using the default CF, my mistake. I'll follow the pattern and implement as you suggested
|
@arnetheduck I've pushed an update to the PR. Ready for your review. |
rocksdb/options/dbopts.nim
Outdated
| @@ -111,6 +111,9 @@ proc defaultDbOptions*(autoClose = false): DbOptionsRef = | |||
| # Enable creating column families if they do not exist | |||
| dbOpts.createMissingColumnFamilies = true | |||
|
|
|||
| # Make sure flush is atomic accross column families | |||
| dbOpts.atomicFlush = true | |||
There was a problem hiding this comment.
this is not something we want to enable by default, ie it carries greater (performance) implications than is obvious
This can be used to avoid log replays on startup, for example