Add flush method to Database trait #409
Conversation
rajarshimaitra
left a comment
rajarshimaitra
left a comment
There was a problem hiding this comment.
ACK.
One API comment.
src/database/keyvalue.rs
Outdated
| } | ||
|
|
||
| fn flush(&mut self) -> Result<usize, Error> { | ||
| Ok(Tree::flush(self)?) |
There was a problem hiding this comment.
Would we use the "bytes written" value in some way? This will also force library users implementing their own DB's flush to return some number.
If it's not super useful maybe we shouldn't expose this as general API just because sled does it?
|
We had a short discussion in meeting today. Have we considered just flushing after every write always on sled? I'm not really a fan of forcing the user to care about this. |
|
I think one of the things discussed was that it was a performance hit on all applications when it might have been an issue mostly only for Android/mobile. Would it be possible to have a flag turned on by default where it flushes to disk for all operations, but if you do care about efficiency and don't want the performance hit you can turn it off? Or maybe the opposite? Doesn't happen by default but you can have it flush to disk for all if you are working on mobile stuff (but then we're back to your point about having to make the user care about it). |
|
I really don't think that there would be a performance issue here. It's a wallet not a full node! In any case it's better to not optimize early. Despite it only being an issue on android I would prefer my wallet to flush to disk wherever possible. |
|
Flushing after every sync (maybe improving later to do it only if the state changed) without adding a new method was my first proposal, the safest path with only a small performance hit I think (sync is a network method, adding a write to disk shouldn't change much its execution time in percentual terms). In the 14th July meeting, the general feedback was instead to have an explicit flush method (see also #391 (comment)). I am in favor to close this and flush at sync but maybe @afilini has something to add |
tcharding
left a comment
tcharding
left a comment
There was a problem hiding this comment.
Seems reasonable to me.
|
I think the main reason why I like the explicit We could fix this just for the sled db, but then when the external users implement the trait on other external types there's a chance they could make the same mistake. With an explicit method they are forced to provide an implementation for I agree with @rajarshimaitra that maybe returning the number of changed bytes isn't ideal, because if you implement this for things like Postgres it's kinda meaningless, and it's not that the library needs it in any way. |
|
Rebased and removed the returned |
|
ReACK fe30716 |
src/database/mod.rs
Outdated
| /// It should insert and return `0` if not present in the database | ||
| fn increment_last_index(&mut self, keychain: KeychainKind) -> Result<u32, Error>; | ||
|
|
||
| /// Force changes to be written to disk, returning the number of bytes written |
There was a problem hiding this comment.
Needs to be updated to match last change:
| /// Force changes to be written to disk, returning the number of bytes written | |
| /// Force changes to be written to disk |
7 participants
Description
Expose
flushmethod in theDatabasetrait to explicitly flush db changes to diskA step toward fixing #391.
Following steps are
Checklists
All Submissions:
cargo fmtandcargo clippybefore committingNew Features:
CHANGELOG.md