-
Notifications
You must be signed in to change notification settings - Fork 409
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
feat: Add flush method to boxes #852
Conversation
@themisir could you look in to this we are very interested in trying this out in our next app release which is scheduled soon. |
This comment has been minimized.
This comment has been minimized.
while compact does call |
For reference, this is our thin layer at work, how we utilize this PR |
This comment has been minimized.
This comment has been minimized.
That is unneeded, the kernel takes care of if a file should be flushed to disk or not, and thus, as long as the application is running normally, you don't have to call |
This comment has been minimized.
This comment has been minimized.
No, is saying that it does not matter for |
This comment has been minimized.
This comment has been minimized.
Interesting findings! Do you have any idea if we could setup an environment to test if flushing does solve corruption issue? In theory it should but real life is complicated so I'm thinking if we could test it out at least.. |
We did already test it and adding flushing after writing fixed it for us. To test it reliably you basically have to unexpectedly kill the system, so you might want to use a VM for that. Something like What we did was verify a bunch of such resets with flushing, and it did not corrupt the database at all, while without flushing it did corrupt quite frequently (not 100% of the time, though, as is with the nature of such things) |
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.
LGTM
Thank you for the quick merge! 👍 Please do keep in mind that people will have to utilize the added flush method correctly, as outlined here: #263 (comment) It might make sense to add auto-flush after every put or something as an easy method, but that ought to impact performance quite a bit |
Yes exactly I thought that too, but then thought it might cause performance issues, so a transaction like extension would be much better for people to run transaction in a callback and when execution completes it automatically flushes updates. But that could be added later on but until then users could write a extension like this: extension HiveX on BoxBase {
Future<void> transaction(FutureOr<void> txn()) async {
try {
await txn();
} finally {
this.flush();
}
}
} I'm not planning to add this method to hive itself currently because I want to think a bit about design before implementation. Maybe transaction should be handled on a hive instance instead of box to track and commit changes from all of the boxes at once. Or it might be modified to work like how other dbs handle transactions: temporarily fork database for the transaction and when committed replace original with the modified data or something like that, or I'm just not sure yet, I have research a bit to learn how other dbs handle those stuff, there might be things needs to be considered. Additionally to that hive also works on web and afaik IndexedDB has its own transaction concept, so if we were to add transactions support we have to implement it across the storage level. As you can see there's lots of things needs to be considered before implementation. So until that we could ship |
Yeah. At work we usually operate on multiple boxes at once, which is why our solution currently is this where it additionally automatically collects which boxes it does write operations on, using dart execution zones. Maybe something like that could also be added here, with like await Hive.transaction(() async {
// write operations on multiple boxes
}); |
Ensure HiveLists are treated as non-nullable in hive_generator (isar#728) * Replaced ?. operator with . operator Resolves error "The argument type 'HiveList<T>?' can't be assigned to the parameter type 'HiveList<T>'. Closes issue isar#664. * Ensuring nullable HiveLists are properly generated Co-authored-by: Misir Jafarov <misir.ceferov@gmail.com> Updated hive_generator dependencies (isar#765) * Updated dependencies * Changed version dependencies Bump up hive_generator to v1.1.1 Get indexDB selectively based on window property (isar#802) Co-authored-by: Wenhao Wu <wenhao.wu@signanthealth.com> Add missing path parameter to HiveInterface methods (isar#776) Fixes broken CI (isar#806) * Update pubspec.yaml * Migrated from mockito to mocktail * Test on dart 2.14 Don't loose track of box objects if init crashes (isar#846) feat: Add flush method to boxes (isar#852) bump: Update to v2.0.5 Updated hive_generator analyzer dependency (isar#858) bump up hive_generator to v1.1.2 Change pedantic to linter and fix warnings (isar#876)
This PR adds a flush method to boxes to be able to manually flush them to the disk. If utilized correctly, this would solve #263, also see this comment: #263 (comment)