Remove database flush

[kafaichoi]

Description

This PR is to remove Database::flush. See this issue for detail #567

Notes to the reviewers

The 2nd commit is a small refactoring of adding a new private ivec_to_u32 to avoid too much code duplication. Please let me know if it's ok to include this in this PR or I should make it into a separate PR

Currently existing test cases are shared across for all Databaes implementation so I am not sure if we should add specific test cases for keyvalue(Tree) for this auto-flush behaviour?(and I feel like it's more a implementation detail). Please let me know how should I proceed for test case in this PR

Checklists

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing

New Features:

• I've added tests for the new feature
• I've added docs for the new feature
• I've updated CHANGELOG.md

Bugfixes:

• This pull request breaks the existing API
• I've added tests to reproduce the issue which are now passing
• I'm linking the issue being fixed by this PR

[notmandatory]

I think your code changes to remove flush from the Database trait and to refactor duplicate code into the ivec_to_u32 function look good.

But I'm having second thoughts on doing the auto-flush. I believe it's not meant to be used on every update since it will hurt performance and on most os/filesystems it is not needed since the os will automatically flush the data to disk if the process dies. For Android users it's reasonable to recommend using sqlite and not sled for now. In future bdk api changes that are currently being discussed the database will most likely be extracted from the Wallet and then if a user wants to use Sled on android they will be able directly call the .flush() function on Tree when they want such as when their app goes off screen or something like that. I'm going to update #567 to remove the part about auto flushing so you can remove the related code and tests from this PR.

You also need to do a cargo fmt which CI checks for. Thanks!

[kafaichoi]

I think your code changes to remove flush from the Database trait and to refactor duplicate code into the ivec_to_u32 function look good.

But I'm having second thoughts on doing the auto-flush. I believe it's not meant to be used on every update since it will hurt performance and on most os/filesystems it is not needed since the os will automatically flush the data to disk if the process dies. For Android users it's reasonable to recommend using sqlite and not sled for now. In future bdk api changes that are currently being discussed the database will most likely be extracted from the Wallet and then if a user wants to use Sled on android they will be able directly call the .flush() function on Tree when they want such as when their app goes off screen or something like that. I'm going to update #567 to remove the part about auto flushing so you can remove the related code and tests from this PR.

You also need to do a cargo fmt which CI checks for. Thanks!

That's very reasonable. Thank you so much for your comment. I have removed the auto-flush behaviour(and ran cargo fmt, clippy for each commit)

[rajarshimaitra]

tACK 6dc9282

With few nits..

[rajarshimaitra]

@notmandatory so we don't need the explicit flush any more for any of the DBs?

[notmandatory]

@notmandatory so we don't need the explicit flush any more for any of the DBs?

The only database that actually implemented flush was Sled, and even for Sled it seems to only be needed for Android. That's why I suggested removing it and that mobile apps use sqlite instead. This #409 is the PR that added it, and it was to trying to fix the issues that @thunderbiscuit ran into on Android.

[afilini]

Maybe we should deprecate for one release before removing it? I don't know if anybody is using it, but it seems it wouldn't cost us anything to just delay the removal and it can make the transition a bit less painful for our users.

Concept ACK for the refactor but I'll take another look later to make sure the code is also good.

[notmandatory]

Maybe we should deprecate for one release before removing it? I don't know if anybody is using it, but it seems it wouldn't cost us anything to just delay the removal and it can make the transition a bit less painful for our users.

Good point, I was assuming no one was using this but I think we did promise to deprecate APIs before removing them. If we do deprecate that should go in a new PR, so this one can be ready to go right after we cut the next release branch.

[rajarshimaitra]

ReACK 0e713f8

[notmandatory]

I've assigned this to the release 0.19.0 and added #577 to deprecate Database::flush() in 0.18.0. This PR can then be merged after we create the release/0.19.0 feature freeze branch next week.

[notmandatory]

Hi, please rebase to pickup changes in #596. Thanks!

[notmandatory]

I had to push this PR to the next release so the team can focus on #593, and after that one you'll probably need to rebase again. But then I promise we get this one in. :-)

[danielabrozzoni]

tACK b1ace3c

[rajarshimaitra]

tACK b1ace3c

This looks ready for merge to me..

[danielabrozzoni]

This PR needs rebase to fix the CHANGELOG.md conflicts and fix the CI, but then it's ready to merge 🙏🏻

[danielabrozzoni]

re-ACK 5ff8320

[notmandatory]

Finally merged, thanks for seeing this one through!

[kafaichoi]

Description

This PR is to remove Database::flush. See this issue for detail #567

Notes to the reviewers

The 2nd commit is a small refactoring of adding a new private ivec_to_u32 to avoid too much code duplication. Please let me know if it's ok to include this in this PR or I should make it into a separate PR

Currently existing test cases are shared across for all Databaes implementation so I am not sure if we should add specific test cases for keyvalue(Tree) for this auto-flush behaviour?(and I feel like it's more a implementation detail). Please let me know how should I proceed for test case in this PR

Checklists

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing

New Features:

• I've added tests for the new feature
• I've added docs for the new feature
• I've updated CHANGELOG.md

Bugfixes:

• This pull request breaks the existing API
• I've added tests to reproduce the issue which are now passing
• I'm linking the issue being fixed by this PR

[notmandatory]

I think your code changes to remove flush from the Database trait and to refactor duplicate code into the ivec_to_u32 function look good.

But I'm having second thoughts on doing the auto-flush. I believe it's not meant to be used on every update since it will hurt performance and on most os/filesystems it is not needed since the os will automatically flush the data to disk if the process dies. For Android users it's reasonable to recommend using sqlite and not sled for now. In future bdk api changes that are currently being discussed the database will most likely be extracted from the Wallet and then if a user wants to use Sled on android they will be able directly call the .flush() function on Tree when they want such as when their app goes off screen or something like that. I'm going to update #567 to remove the part about auto flushing so you can remove the related code and tests from this PR.

You also need to do a cargo fmt which CI checks for. Thanks!

[kafaichoi]

I think your code changes to remove flush from the Database trait and to refactor duplicate code into the ivec_to_u32 function look good.

But I'm having second thoughts on doing the auto-flush. I believe it's not meant to be used on every update since it will hurt performance and on most os/filesystems it is not needed since the os will automatically flush the data to disk if the process dies. For Android users it's reasonable to recommend using sqlite and not sled for now. In future bdk api changes that are currently being discussed the database will most likely be extracted from the Wallet and then if a user wants to use Sled on android they will be able directly call the .flush() function on Tree when they want such as when their app goes off screen or something like that. I'm going to update #567 to remove the part about auto flushing so you can remove the related code and tests from this PR.

You also need to do a cargo fmt which CI checks for. Thanks!

That's very reasonable. Thank you so much for your comment. I have removed the auto-flush behaviour(and ran cargo fmt, clippy for each commit)

[rajarshimaitra]

tACK 6dc9282

With few nits..

[rajarshimaitra]

@notmandatory so we don't need the explicit flush any more for any of the DBs?

[notmandatory]

@notmandatory so we don't need the explicit flush any more for any of the DBs?

The only database that actually implemented flush was Sled, and even for Sled it seems to only be needed for Android. That's why I suggested removing it and that mobile apps use sqlite instead. This #409 is the PR that added it, and it was to trying to fix the issues that @thunderbiscuit ran into on Android.

[afilini]

Maybe we should deprecate for one release before removing it? I don't know if anybody is using it, but it seems it wouldn't cost us anything to just delay the removal and it can make the transition a bit less painful for our users.

Concept ACK for the refactor but I'll take another look later to make sure the code is also good.

[notmandatory]

Maybe we should deprecate for one release before removing it? I don't know if anybody is using it, but it seems it wouldn't cost us anything to just delay the removal and it can make the transition a bit less painful for our users.

Good point, I was assuming no one was using this but I think we did promise to deprecate APIs before removing them. If we do deprecate that should go in a new PR, so this one can be ready to go right after we cut the next release branch.

[rajarshimaitra]

ReACK 0e713f8

[notmandatory]

I've assigned this to the release 0.19.0 and added #577 to deprecate Database::flush() in 0.18.0. This PR can then be merged after we create the release/0.19.0 feature freeze branch next week.

[notmandatory]

Hi, please rebase to pickup changes in #596. Thanks!

[notmandatory]

I had to push this PR to the next release so the team can focus on #593, and after that one you'll probably need to rebase again. But then I promise we get this one in. :-)

[danielabrozzoni]

tACK b1ace3c

[rajarshimaitra]

tACK b1ace3c

This looks ready for merge to me..

[danielabrozzoni]

This PR needs rebase to fix the CHANGELOG.md conflicts and fix the CI, but then it's ready to merge 🙏🏻

[danielabrozzoni]

re-ACK 5ff8320

[notmandatory]

Finally merged, thanks for seeing this one through!

[kafaichoi]

Description

This PR is to remove Database::flush. See this issue for detail #567

Notes to the reviewers

The 2nd commit is a small refactoring of adding a new private ivec_to_u32 to avoid too much code duplication. Please let me know if it's ok to include this in this PR or I should make it into a separate PR

Currently existing test cases are shared across for all Databaes implementation so I am not sure if we should add specific test cases for keyvalue(Tree) for this auto-flush behaviour?(and I feel like it's more a implementation detail). Please let me know how should I proceed for test case in this PR

Checklists

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing

New Features:

• I've added tests for the new feature
• I've added docs for the new feature
• I've updated CHANGELOG.md

Bugfixes:

• This pull request breaks the existing API
• I've added tests to reproduce the issue which are now passing
• I'm linking the issue being fixed by this PR

[notmandatory]

I think your code changes to remove flush from the Database trait and to refactor duplicate code into the ivec_to_u32 function look good.

But I'm having second thoughts on doing the auto-flush. I believe it's not meant to be used on every update since it will hurt performance and on most os/filesystems it is not needed since the os will automatically flush the data to disk if the process dies. For Android users it's reasonable to recommend using sqlite and not sled for now. In future bdk api changes that are currently being discussed the database will most likely be extracted from the Wallet and then if a user wants to use Sled on android they will be able directly call the .flush() function on Tree when they want such as when their app goes off screen or something like that. I'm going to update #567 to remove the part about auto flushing so you can remove the related code and tests from this PR.

You also need to do a cargo fmt which CI checks for. Thanks!

[kafaichoi]

I think your code changes to remove flush from the Database trait and to refactor duplicate code into the ivec_to_u32 function look good.

But I'm having second thoughts on doing the auto-flush. I believe it's not meant to be used on every update since it will hurt performance and on most os/filesystems it is not needed since the os will automatically flush the data to disk if the process dies. For Android users it's reasonable to recommend using sqlite and not sled for now. In future bdk api changes that are currently being discussed the database will most likely be extracted from the Wallet and then if a user wants to use Sled on android they will be able directly call the .flush() function on Tree when they want such as when their app goes off screen or something like that. I'm going to update #567 to remove the part about auto flushing so you can remove the related code and tests from this PR.

You also need to do a cargo fmt which CI checks for. Thanks!

That's very reasonable. Thank you so much for your comment. I have removed the auto-flush behaviour(and ran cargo fmt, clippy for each commit)

[rajarshimaitra]

tACK 6dc9282

With few nits..

[rajarshimaitra]

@notmandatory so we don't need the explicit flush any more for any of the DBs?

[notmandatory]

@notmandatory so we don't need the explicit flush any more for any of the DBs?

The only database that actually implemented flush was Sled, and even for Sled it seems to only be needed for Android. That's why I suggested removing it and that mobile apps use sqlite instead. This #409 is the PR that added it, and it was to trying to fix the issues that @thunderbiscuit ran into on Android.

[afilini]

Maybe we should deprecate for one release before removing it? I don't know if anybody is using it, but it seems it wouldn't cost us anything to just delay the removal and it can make the transition a bit less painful for our users.

Concept ACK for the refactor but I'll take another look later to make sure the code is also good.

[notmandatory]

Maybe we should deprecate for one release before removing it? I don't know if anybody is using it, but it seems it wouldn't cost us anything to just delay the removal and it can make the transition a bit less painful for our users.

Good point, I was assuming no one was using this but I think we did promise to deprecate APIs before removing them. If we do deprecate that should go in a new PR, so this one can be ready to go right after we cut the next release branch.

[rajarshimaitra]

ReACK 0e713f8

[notmandatory]

I've assigned this to the release 0.19.0 and added #577 to deprecate Database::flush() in 0.18.0. This PR can then be merged after we create the release/0.19.0 feature freeze branch next week.

[notmandatory]

Hi, please rebase to pickup changes in #596. Thanks!

[notmandatory]

I had to push this PR to the next release so the team can focus on #593, and after that one you'll probably need to rebase again. But then I promise we get this one in. :-)

[danielabrozzoni]

tACK b1ace3c

[rajarshimaitra]

tACK b1ace3c

This looks ready for merge to me..

[danielabrozzoni]

This PR needs rebase to fix the CHANGELOG.md conflicts and fix the CI, but then it's ready to merge 🙏🏻

[danielabrozzoni]

re-ACK 5ff8320

[notmandatory]

Finally merged, thanks for seeing this one through!

[kafaichoi]

Description

This PR is to remove Database::flush. See this issue for detail #567

Notes to the reviewers

The 2nd commit is a small refactoring of adding a new private ivec_to_u32 to avoid too much code duplication. Please let me know if it's ok to include this in this PR or I should make it into a separate PR

Currently existing test cases are shared across for all Databaes implementation so I am not sure if we should add specific test cases for keyvalue(Tree) for this auto-flush behaviour?(and I feel like it's more a implementation detail). Please let me know how should I proceed for test case in this PR

Checklists

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing

New Features:

• I've added tests for the new feature
• I've added docs for the new feature
• I've updated CHANGELOG.md

Bugfixes:

• This pull request breaks the existing API
• I've added tests to reproduce the issue which are now passing
• I'm linking the issue being fixed by this PR

[notmandatory]

I think your code changes to remove flush from the Database trait and to refactor duplicate code into the ivec_to_u32 function look good.

But I'm having second thoughts on doing the auto-flush. I believe it's not meant to be used on every update since it will hurt performance and on most os/filesystems it is not needed since the os will automatically flush the data to disk if the process dies. For Android users it's reasonable to recommend using sqlite and not sled for now. In future bdk api changes that are currently being discussed the database will most likely be extracted from the Wallet and then if a user wants to use Sled on android they will be able directly call the .flush() function on Tree when they want such as when their app goes off screen or something like that. I'm going to update #567 to remove the part about auto flushing so you can remove the related code and tests from this PR.

You also need to do a cargo fmt which CI checks for. Thanks!

[kafaichoi]

I think your code changes to remove flush from the Database trait and to refactor duplicate code into the ivec_to_u32 function look good.

But I'm having second thoughts on doing the auto-flush. I believe it's not meant to be used on every update since it will hurt performance and on most os/filesystems it is not needed since the os will automatically flush the data to disk if the process dies. For Android users it's reasonable to recommend using sqlite and not sled for now. In future bdk api changes that are currently being discussed the database will most likely be extracted from the Wallet and then if a user wants to use Sled on android they will be able directly call the .flush() function on Tree when they want such as when their app goes off screen or something like that. I'm going to update #567 to remove the part about auto flushing so you can remove the related code and tests from this PR.

You also need to do a cargo fmt which CI checks for. Thanks!

That's very reasonable. Thank you so much for your comment. I have removed the auto-flush behaviour(and ran cargo fmt, clippy for each commit)

[rajarshimaitra]

tACK 6dc9282

With few nits..

[rajarshimaitra]

@notmandatory so we don't need the explicit flush any more for any of the DBs?

[notmandatory]

@notmandatory so we don't need the explicit flush any more for any of the DBs?

The only database that actually implemented flush was Sled, and even for Sled it seems to only be needed for Android. That's why I suggested removing it and that mobile apps use sqlite instead. This #409 is the PR that added it, and it was to trying to fix the issues that @thunderbiscuit ran into on Android.

[afilini]

Maybe we should deprecate for one release before removing it? I don't know if anybody is using it, but it seems it wouldn't cost us anything to just delay the removal and it can make the transition a bit less painful for our users.

Concept ACK for the refactor but I'll take another look later to make sure the code is also good.

[notmandatory]

Maybe we should deprecate for one release before removing it? I don't know if anybody is using it, but it seems it wouldn't cost us anything to just delay the removal and it can make the transition a bit less painful for our users.

Good point, I was assuming no one was using this but I think we did promise to deprecate APIs before removing them. If we do deprecate that should go in a new PR, so this one can be ready to go right after we cut the next release branch.

[rajarshimaitra]

ReACK 0e713f8

[notmandatory]

I've assigned this to the release 0.19.0 and added #577 to deprecate Database::flush() in 0.18.0. This PR can then be merged after we create the release/0.19.0 feature freeze branch next week.

[notmandatory]

Hi, please rebase to pickup changes in #596. Thanks!

[notmandatory]

I had to push this PR to the next release so the team can focus on #593, and after that one you'll probably need to rebase again. But then I promise we get this one in. :-)

[danielabrozzoni]

tACK b1ace3c

[rajarshimaitra]

tACK b1ace3c

This looks ready for merge to me..

[danielabrozzoni]

This PR needs rebase to fix the CHANGELOG.md conflicts and fix the CI, but then it's ready to merge 🙏🏻

[danielabrozzoni]

re-ACK 5ff8320

[notmandatory]

Finally merged, thanks for seeing this one through!

[kafaichoi]

Description

This PR is to remove Database::flush. See this issue for detail #567

Notes to the reviewers

The 2nd commit is a small refactoring of adding a new private ivec_to_u32 to avoid too much code duplication. Please let me know if it's ok to include this in this PR or I should make it into a separate PR

Currently existing test cases are shared across for all Databaes implementation so I am not sure if we should add specific test cases for keyvalue(Tree) for this auto-flush behaviour?(and I feel like it's more a implementation detail). Please let me know how should I proceed for test case in this PR

Checklists

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing

New Features:

• I've added tests for the new feature
• I've added docs for the new feature
• I've updated CHANGELOG.md

Bugfixes:

• This pull request breaks the existing API
• I've added tests to reproduce the issue which are now passing
• I'm linking the issue being fixed by this PR

[notmandatory]

I think your code changes to remove flush from the Database trait and to refactor duplicate code into the ivec_to_u32 function look good.

But I'm having second thoughts on doing the auto-flush. I believe it's not meant to be used on every update since it will hurt performance and on most os/filesystems it is not needed since the os will automatically flush the data to disk if the process dies. For Android users it's reasonable to recommend using sqlite and not sled for now. In future bdk api changes that are currently being discussed the database will most likely be extracted from the Wallet and then if a user wants to use Sled on android they will be able directly call the .flush() function on Tree when they want such as when their app goes off screen or something like that. I'm going to update #567 to remove the part about auto flushing so you can remove the related code and tests from this PR.

You also need to do a cargo fmt which CI checks for. Thanks!

[kafaichoi]

I think your code changes to remove flush from the Database trait and to refactor duplicate code into the ivec_to_u32 function look good.

But I'm having second thoughts on doing the auto-flush. I believe it's not meant to be used on every update since it will hurt performance and on most os/filesystems it is not needed since the os will automatically flush the data to disk if the process dies. For Android users it's reasonable to recommend using sqlite and not sled for now. In future bdk api changes that are currently being discussed the database will most likely be extracted from the Wallet and then if a user wants to use Sled on android they will be able directly call the .flush() function on Tree when they want such as when their app goes off screen or something like that. I'm going to update #567 to remove the part about auto flushing so you can remove the related code and tests from this PR.

You also need to do a cargo fmt which CI checks for. Thanks!

That's very reasonable. Thank you so much for your comment. I have removed the auto-flush behaviour(and ran cargo fmt, clippy for each commit)

[rajarshimaitra]

tACK 6dc9282

With few nits..

[rajarshimaitra]

@notmandatory so we don't need the explicit flush any more for any of the DBs?

[notmandatory]

@notmandatory so we don't need the explicit flush any more for any of the DBs?

The only database that actually implemented flush was Sled, and even for Sled it seems to only be needed for Android. That's why I suggested removing it and that mobile apps use sqlite instead. This #409 is the PR that added it, and it was to trying to fix the issues that @thunderbiscuit ran into on Android.

[afilini]

Maybe we should deprecate for one release before removing it? I don't know if anybody is using it, but it seems it wouldn't cost us anything to just delay the removal and it can make the transition a bit less painful for our users.

Concept ACK for the refactor but I'll take another look later to make sure the code is also good.

[notmandatory]

Maybe we should deprecate for one release before removing it? I don't know if anybody is using it, but it seems it wouldn't cost us anything to just delay the removal and it can make the transition a bit less painful for our users.

Good point, I was assuming no one was using this but I think we did promise to deprecate APIs before removing them. If we do deprecate that should go in a new PR, so this one can be ready to go right after we cut the next release branch.

[rajarshimaitra]

ReACK 0e713f8

[notmandatory]

I've assigned this to the release 0.19.0 and added #577 to deprecate Database::flush() in 0.18.0. This PR can then be merged after we create the release/0.19.0 feature freeze branch next week.

[notmandatory]

Hi, please rebase to pickup changes in #596. Thanks!

[notmandatory]

I had to push this PR to the next release so the team can focus on #593, and after that one you'll probably need to rebase again. But then I promise we get this one in. :-)

[danielabrozzoni]

tACK b1ace3c

[rajarshimaitra]

tACK b1ace3c

This looks ready for merge to me..

[danielabrozzoni]

This PR needs rebase to fix the CHANGELOG.md conflicts and fix the CI, but then it's ready to merge 🙏🏻

[danielabrozzoni]

re-ACK 5ff8320

[notmandatory]

Finally merged, thanks for seeing this one through!

[kafaichoi]

Description

This PR is to remove Database::flush. See this issue for detail #567

Notes to the reviewers

The 2nd commit is a small refactoring of adding a new private ivec_to_u32 to avoid too much code duplication. Please let me know if it's ok to include this in this PR or I should make it into a separate PR

Currently existing test cases are shared across for all Databaes implementation so I am not sure if we should add specific test cases for keyvalue(Tree) for this auto-flush behaviour?(and I feel like it's more a implementation detail). Please let me know how should I proceed for test case in this PR

Checklists

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing

New Features:

• I've added tests for the new feature
• I've added docs for the new feature
• I've updated CHANGELOG.md

Bugfixes:

• This pull request breaks the existing API
• I've added tests to reproduce the issue which are now passing
• I'm linking the issue being fixed by this PR

[notmandatory]

I think your code changes to remove flush from the Database trait and to refactor duplicate code into the ivec_to_u32 function look good.

But I'm having second thoughts on doing the auto-flush. I believe it's not meant to be used on every update since it will hurt performance and on most os/filesystems it is not needed since the os will automatically flush the data to disk if the process dies. For Android users it's reasonable to recommend using sqlite and not sled for now. In future bdk api changes that are currently being discussed the database will most likely be extracted from the Wallet and then if a user wants to use Sled on android they will be able directly call the .flush() function on Tree when they want such as when their app goes off screen or something like that. I'm going to update #567 to remove the part about auto flushing so you can remove the related code and tests from this PR.

You also need to do a cargo fmt which CI checks for. Thanks!

[kafaichoi]

I think your code changes to remove flush from the Database trait and to refactor duplicate code into the ivec_to_u32 function look good.

But I'm having second thoughts on doing the auto-flush. I believe it's not meant to be used on every update since it will hurt performance and on most os/filesystems it is not needed since the os will automatically flush the data to disk if the process dies. For Android users it's reasonable to recommend using sqlite and not sled for now. In future bdk api changes that are currently being discussed the database will most likely be extracted from the Wallet and then if a user wants to use Sled on android they will be able directly call the .flush() function on Tree when they want such as when their app goes off screen or something like that. I'm going to update #567 to remove the part about auto flushing so you can remove the related code and tests from this PR.

You also need to do a cargo fmt which CI checks for. Thanks!

That's very reasonable. Thank you so much for your comment. I have removed the auto-flush behaviour(and ran cargo fmt, clippy for each commit)

[rajarshimaitra]

tACK 6dc9282

With few nits..

[rajarshimaitra]

@notmandatory so we don't need the explicit flush any more for any of the DBs?

[notmandatory]

@notmandatory so we don't need the explicit flush any more for any of the DBs?

The only database that actually implemented flush was Sled, and even for Sled it seems to only be needed for Android. That's why I suggested removing it and that mobile apps use sqlite instead. This #409 is the PR that added it, and it was to trying to fix the issues that @thunderbiscuit ran into on Android.

[afilini]

Maybe we should deprecate for one release before removing it? I don't know if anybody is using it, but it seems it wouldn't cost us anything to just delay the removal and it can make the transition a bit less painful for our users.

Concept ACK for the refactor but I'll take another look later to make sure the code is also good.

[notmandatory]

Maybe we should deprecate for one release before removing it? I don't know if anybody is using it, but it seems it wouldn't cost us anything to just delay the removal and it can make the transition a bit less painful for our users.

Good point, I was assuming no one was using this but I think we did promise to deprecate APIs before removing them. If we do deprecate that should go in a new PR, so this one can be ready to go right after we cut the next release branch.

[rajarshimaitra]

ReACK 0e713f8

[notmandatory]

I've assigned this to the release 0.19.0 and added #577 to deprecate Database::flush() in 0.18.0. This PR can then be merged after we create the release/0.19.0 feature freeze branch next week.

[notmandatory]

Hi, please rebase to pickup changes in #596. Thanks!

[notmandatory]

I had to push this PR to the next release so the team can focus on #593, and after that one you'll probably need to rebase again. But then I promise we get this one in. :-)

[danielabrozzoni]

tACK b1ace3c

[rajarshimaitra]

tACK b1ace3c

This looks ready for merge to me..

[danielabrozzoni]

This PR needs rebase to fix the CHANGELOG.md conflicts and fix the CI, but then it's ready to merge 🙏🏻

[danielabrozzoni]

re-ACK 5ff8320

[notmandatory]

Finally merged, thanks for seeing this one through!

[kafaichoi]

Description

This PR is to remove Database::flush. See this issue for detail #567

Notes to the reviewers

The 2nd commit is a small refactoring of adding a new private ivec_to_u32 to avoid too much code duplication. Please let me know if it's ok to include this in this PR or I should make it into a separate PR

Currently existing test cases are shared across for all Databaes implementation so I am not sure if we should add specific test cases for keyvalue(Tree) for this auto-flush behaviour?(and I feel like it's more a implementation detail). Please let me know how should I proceed for test case in this PR

Checklists

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing

New Features:

• I've added tests for the new feature
• I've added docs for the new feature
• I've updated CHANGELOG.md

Bugfixes:

• This pull request breaks the existing API
• I've added tests to reproduce the issue which are now passing
• I'm linking the issue being fixed by this PR

[notmandatory]

I think your code changes to remove flush from the Database trait and to refactor duplicate code into the ivec_to_u32 function look good.

But I'm having second thoughts on doing the auto-flush. I believe it's not meant to be used on every update since it will hurt performance and on most os/filesystems it is not needed since the os will automatically flush the data to disk if the process dies. For Android users it's reasonable to recommend using sqlite and not sled for now. In future bdk api changes that are currently being discussed the database will most likely be extracted from the Wallet and then if a user wants to use Sled on android they will be able directly call the .flush() function on Tree when they want such as when their app goes off screen or something like that. I'm going to update #567 to remove the part about auto flushing so you can remove the related code and tests from this PR.

You also need to do a cargo fmt which CI checks for. Thanks!

[kafaichoi]

I think your code changes to remove flush from the Database trait and to refactor duplicate code into the ivec_to_u32 function look good.

But I'm having second thoughts on doing the auto-flush. I believe it's not meant to be used on every update since it will hurt performance and on most os/filesystems it is not needed since the os will automatically flush the data to disk if the process dies. For Android users it's reasonable to recommend using sqlite and not sled for now. In future bdk api changes that are currently being discussed the database will most likely be extracted from the Wallet and then if a user wants to use Sled on android they will be able directly call the .flush() function on Tree when they want such as when their app goes off screen or something like that. I'm going to update #567 to remove the part about auto flushing so you can remove the related code and tests from this PR.

You also need to do a cargo fmt which CI checks for. Thanks!

That's very reasonable. Thank you so much for your comment. I have removed the auto-flush behaviour(and ran cargo fmt, clippy for each commit)

[rajarshimaitra]

tACK 6dc9282

With few nits..

[rajarshimaitra]

@notmandatory so we don't need the explicit flush any more for any of the DBs?

[notmandatory]

@notmandatory so we don't need the explicit flush any more for any of the DBs?

The only database that actually implemented flush was Sled, and even for Sled it seems to only be needed for Android. That's why I suggested removing it and that mobile apps use sqlite instead. This #409 is the PR that added it, and it was to trying to fix the issues that @thunderbiscuit ran into on Android.

[afilini]

Maybe we should deprecate for one release before removing it? I don't know if anybody is using it, but it seems it wouldn't cost us anything to just delay the removal and it can make the transition a bit less painful for our users.

Concept ACK for the refactor but I'll take another look later to make sure the code is also good.

[notmandatory]

Maybe we should deprecate for one release before removing it? I don't know if anybody is using it, but it seems it wouldn't cost us anything to just delay the removal and it can make the transition a bit less painful for our users.

Good point, I was assuming no one was using this but I think we did promise to deprecate APIs before removing them. If we do deprecate that should go in a new PR, so this one can be ready to go right after we cut the next release branch.

[rajarshimaitra]

ReACK 0e713f8

[notmandatory]

I've assigned this to the release 0.19.0 and added #577 to deprecate Database::flush() in 0.18.0. This PR can then be merged after we create the release/0.19.0 feature freeze branch next week.

[notmandatory]

Hi, please rebase to pickup changes in #596. Thanks!

[notmandatory]

I had to push this PR to the next release so the team can focus on #593, and after that one you'll probably need to rebase again. But then I promise we get this one in. :-)

[danielabrozzoni]

tACK b1ace3c

[rajarshimaitra]

tACK b1ace3c

This looks ready for merge to me..

[danielabrozzoni]

This PR needs rebase to fix the CHANGELOG.md conflicts and fix the CI, but then it's ready to merge 🙏🏻

[danielabrozzoni]

re-ACK 5ff8320

[notmandatory]

Finally merged, thanks for seeing this one through!

[kafaichoi]

Description

This PR is to remove Database::flush. See this issue for detail #567

Notes to the reviewers

The 2nd commit is a small refactoring of adding a new private ivec_to_u32 to avoid too much code duplication. Please let me know if it's ok to include this in this PR or I should make it into a separate PR

Currently existing test cases are shared across for all Databaes implementation so I am not sure if we should add specific test cases for keyvalue(Tree) for this auto-flush behaviour?(and I feel like it's more a implementation detail). Please let me know how should I proceed for test case in this PR

Checklists

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing

New Features:

• I've added tests for the new feature
• I've added docs for the new feature
• I've updated CHANGELOG.md

Bugfixes:

• This pull request breaks the existing API
• I've added tests to reproduce the issue which are now passing
• I'm linking the issue being fixed by this PR

[notmandatory]

I think your code changes to remove flush from the Database trait and to refactor duplicate code into the ivec_to_u32 function look good.

But I'm having second thoughts on doing the auto-flush. I believe it's not meant to be used on every update since it will hurt performance and on most os/filesystems it is not needed since the os will automatically flush the data to disk if the process dies. For Android users it's reasonable to recommend using sqlite and not sled for now. In future bdk api changes that are currently being discussed the database will most likely be extracted from the Wallet and then if a user wants to use Sled on android they will be able directly call the .flush() function on Tree when they want such as when their app goes off screen or something like that. I'm going to update #567 to remove the part about auto flushing so you can remove the related code and tests from this PR.

You also need to do a cargo fmt which CI checks for. Thanks!

[kafaichoi]

I think your code changes to remove flush from the Database trait and to refactor duplicate code into the ivec_to_u32 function look good.

But I'm having second thoughts on doing the auto-flush. I believe it's not meant to be used on every update since it will hurt performance and on most os/filesystems it is not needed since the os will automatically flush the data to disk if the process dies. For Android users it's reasonable to recommend using sqlite and not sled for now. In future bdk api changes that are currently being discussed the database will most likely be extracted from the Wallet and then if a user wants to use Sled on android they will be able directly call the .flush() function on Tree when they want such as when their app goes off screen or something like that. I'm going to update #567 to remove the part about auto flushing so you can remove the related code and tests from this PR.

You also need to do a cargo fmt which CI checks for. Thanks!

That's very reasonable. Thank you so much for your comment. I have removed the auto-flush behaviour(and ran cargo fmt, clippy for each commit)

[rajarshimaitra]

tACK 6dc9282

With few nits..

[rajarshimaitra]

@notmandatory so we don't need the explicit flush any more for any of the DBs?

[notmandatory]

@notmandatory so we don't need the explicit flush any more for any of the DBs?

The only database that actually implemented flush was Sled, and even for Sled it seems to only be needed for Android. That's why I suggested removing it and that mobile apps use sqlite instead. This #409 is the PR that added it, and it was to trying to fix the issues that @thunderbiscuit ran into on Android.

[afilini]

Maybe we should deprecate for one release before removing it? I don't know if anybody is using it, but it seems it wouldn't cost us anything to just delay the removal and it can make the transition a bit less painful for our users.

Concept ACK for the refactor but I'll take another look later to make sure the code is also good.

[notmandatory]

Maybe we should deprecate for one release before removing it? I don't know if anybody is using it, but it seems it wouldn't cost us anything to just delay the removal and it can make the transition a bit less painful for our users.

Good point, I was assuming no one was using this but I think we did promise to deprecate APIs before removing them. If we do deprecate that should go in a new PR, so this one can be ready to go right after we cut the next release branch.

[rajarshimaitra]

ReACK 0e713f8

[notmandatory]

I've assigned this to the release 0.19.0 and added #577 to deprecate Database::flush() in 0.18.0. This PR can then be merged after we create the release/0.19.0 feature freeze branch next week.

[notmandatory]

Hi, please rebase to pickup changes in #596. Thanks!

[notmandatory]

I had to push this PR to the next release so the team can focus on #593, and after that one you'll probably need to rebase again. But then I promise we get this one in. :-)

[danielabrozzoni]

tACK b1ace3c

[rajarshimaitra]

tACK b1ace3c

This looks ready for merge to me..

[danielabrozzoni]

This PR needs rebase to fix the CHANGELOG.md conflicts and fix the CI, but then it's ready to merge 🙏🏻

[danielabrozzoni]

re-ACK 5ff8320

[notmandatory]

Finally merged, thanks for seeing this one through!