-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
expose memtable flush #72
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The c rocksdb type is thread safe so we shouldn't need to lock here as we don't update any fields in the |
||
if not db.isClosed(): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
var cfs = cfs.mapIt(it.cPtr) | ||
var errors: cstring | ||
var opts = rocksdb_flushoptions_create() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
defer: | ||
rocksdb_flushoptions_destroy(opts) | ||
rocksdb_flushoptions_set_wait(opts, 1) | ||
rocksdb_flush_cfs( | ||
db.cPtr, opts, addr cfs[0], cint(cfs.len), cast[cstringArray](errors.addr) | ||
) | ||
bailOnErrors(errors) | ||
|
||
ok() | ||
|
||
proc close*(db: RocksDbRef) = | ||
## Close the `RocksDbRef` which will release the connection to the database | ||
## and free the memory associated with it. `close` is idempotent and can | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes actually, you are correct. That one is using the default CF, my mistake. I'll follow the pattern and implement as you suggested