Skip to content
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

Merged
merged 1 commit into from
Dec 6, 2021
Merged

Conversation

Sorunome
Copy link
Contributor

@Sorunome Sorunome commented Dec 6, 2021

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)

@yasargil
Copy link

yasargil commented Dec 6, 2021

@themisir could you look in to this we are very interested in trying this out in our next app release which is scheduled soon.

@yasargil

This comment has been minimized.

@Sorunome
Copy link
Contributor Author

Sorunome commented Dec 6, 2021

while compact does call .flush() it also does way more, which is why it wouldn't be that ideal for performance. You wouldn't have to call it before making a write, but after making a write.

@Sorunome
Copy link
Contributor Author

Sorunome commented Dec 6, 2021

For reference, this is our thin layer at work, how we utilize this PR

@yasargil

This comment has been minimized.

@Sorunome
Copy link
Contributor Author

Sorunome commented Dec 6, 2021

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 .flush(). The issue comes if the application unexpectedly stops, meaning that there could be pending changes left in-memory which have not been flushed to disk yet.

@yasargil

This comment has been minimized.

@Sorunome
Copy link
Contributor Author

Sorunome commented Dec 6, 2021

No, is saying that it does not matter for .compact() if the changes are flushed to disk or not, as the kernel abstracts that layer mostly away.

@yasargil

This comment has been minimized.

@themisir
Copy link
Contributor

themisir commented Dec 6, 2021

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..

@Sorunome
Copy link
Contributor Author

Sorunome commented Dec 6, 2021

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 echo b > /proc/sysrq-trigger should do the trick.

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)

Copy link
Contributor

@themisir themisir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@themisir themisir merged commit 5e391b1 into isar:master Dec 6, 2021
@Sorunome
Copy link
Contributor Author

Sorunome commented Dec 6, 2021

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

@themisir
Copy link
Contributor

themisir commented Dec 6, 2021

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 flush method and let users write their own transaction extensions.

@Sorunome
Copy link
Contributor Author

Sorunome commented Dec 6, 2021

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
});

mrdavidrees pushed a commit to mrdavidrees/hive that referenced this pull request Jan 16, 2022
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants