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

[ADP-2871] use a query store for tx metas #3828

Merged
merged 4 commits into from
Apr 6, 2023

Conversation

paolino
Copy link
Collaborator

@paolino paolino commented Apr 3, 2023

  • Create a QueryStore for TxMeta so that we can use an (only) on-disk store for them

ADP-2871

@paolino paolino self-assigned this Apr 3, 2023
@paolino paolino force-pushed the paolino/ADP-2871/use-a-query-store-for-tx-metas branch 2 times, most recently from a72f0f3 to c79149f Compare April 4, 2023 11:27
@paolino paolino marked this pull request as ready for review April 4, 2023 11:41
Copy link
Contributor

@HeinrichApfelmus HeinrichApfelmus left a comment

Choose a reason for hiding this comment

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

Excellent, very clean, I like it! 😊 Unfortunately, there is a off-by-one error which was not caught by prop_StoreUpdates. Darn. 😳

In order to fix the off-by-one error, I think the simplest approach is to make a separate query GetAfterSlot, where the lower bound is exclusive rather than inclusive as it is for GetAll. (We could change GetAll to be exclusive on the lower bound, but this would change the semantics of listTransactions – which would be out of scope here.)

Also, I would suggest renaming GetAll to GetRange, as it's probably more appropriate.

Small request on the in-memory implementation of GetOne.

lib/wallet/src/Cardano/Wallet/DB/Store/Meta/Layer.hs Outdated Show resolved Hide resolved
lib/wallet/src/Cardano/Wallet/DB/Store/Wallets/Layer.hs Outdated Show resolved Hide resolved
lib/wallet/src/Cardano/Wallet/DB/Store/Wallets/Store.hs Outdated Show resolved Hide resolved
lib/wallet/src/Cardano/Wallet/DB/Store/Meta/Layer.hs Outdated Show resolved Hide resolved
lib/wallet/src/Cardano/Wallet/DB/Store/Meta/Layer.hs Outdated Show resolved Hide resolved
@paolino paolino force-pushed the paolino/ADP-2871/use-a-query-store-for-tx-metas branch from c79149f to 0154d05 Compare April 5, 2023 09:57
Copy link
Contributor

@HeinrichApfelmus HeinrichApfelmus left a comment

Choose a reason for hiding this comment

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

Excellent, thank you! 😊 Would like to see remaining comments addressed before merge, though. Importantly, I would be in favor of using GetAfterSlot for both the Nothing and the Just case, as this will then ensure unit test coverage for the rollback. (One unit test checks that the queries are the same, and one unit test checks that the query is suitable for a rollback.)

@paolino paolino force-pushed the paolino/ADP-2871/use-a-query-store-for-tx-metas branch 2 times, most recently from 164bdfa to f299e67 Compare April 6, 2023 09:48
@paolino paolino force-pushed the paolino/ADP-2871/use-a-query-store-for-tx-metas branch from f299e67 to 3106c56 Compare April 6, 2023 10:13
@paolino
Copy link
Collaborator Author

paolino commented Apr 6, 2023

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Apr 6, 2023

Build succeeded:

@iohk-bors iohk-bors bot merged commit 48b612e into master Apr 6, 2023
@iohk-bors iohk-bors bot deleted the paolino/ADP-2871/use-a-query-store-for-tx-metas branch April 6, 2023 11:01
WilliamKingNoel-Bot pushed a commit that referenced this pull request Apr 6, 2023
…aolino a=paolino - [x] Create a QueryStore for TxMeta so that we can use an (only) on-disk store for them ADP-2871
 Co-authored-by: paolino <[email protected]> Source commit: 48b612e
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.

2 participants