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: kv impl #5212

Closed
wants to merge 67 commits into from
Closed

feat: kv impl #5212

wants to merge 67 commits into from

Conversation

icorderi
Copy link
Contributor

@icorderi icorderi commented Mar 16, 2023

Summary

Initial KV implementation.

  • A new config value StorageEngine has been added to pick which engine to use.
  • This PR also adds initial support for pebbledb.
  • We added a testsuite that uses Golang subtests to run the same test across all engines.

Key Schema

The key schema as written presently uses human readable prefixes, for ease and debugging and development as this storage engine is built. We intend on compressing these values down to a minimal prefix to save on storage later on.

Additionally, there are some optimizations which are already identified which will affect parts of this key schema once implemented. Those optimizations were not pursued as part of this PR as we are looking to reproduce our SQL engine as closely as possible.

This schema is designed to use get to retrieve specific values, and iterators to produce a range of results which can be filtered or returned. Iterators can range over a partial (prefix-matching) key in order to return all keys which fit the prefix. For example, a partial key of online_account_base-ABCDEFG would return the online account base information for the account ABCDEFG for every persisted round. Fully qualifying the key as online_account_base-ABCDEFG-123 would return just the data from round 123.

Accounts

Index 1: "account-<address>"

  • contains the persisted data about the account

Index 2: "account_balance-<balance_amount>-<account>"

  • Allows for searching/sorting by balance amount

Online Accounts

Index 1: "online_account_base-<address>-<round>

  • contains the persisted online account information for an account at a given round

Index 2: "online_account_balance-<round>-<balance>-<account>"

  • Allows for getting the top online accounts on a given round

Resources

"resource-<address>-<index>"

Creator

"creator-<index>"

Application KV

"appkv-<key>"

  • used in UpsertKvPair

Globals and other singleton-ish keys

"global_round"

  • holds the current round

"global_totals-<stage>"

  • holds the totals data for "staging" or "live"

"online_balance_total-<round>"

  • holds the online balance totals at a given round

"txtail-<round>"

  • holds the tx tail data at a given round

"online_account_round_params-<round>"

  • holds the... you get the idea right?

Important Notes

WARNING: The pebbledb engine remains UNDER DEVELOPMENT and is not intended to be used in production.

We are currently still working on making sure the implementations are compatible.

The work was done in collab with @AlgoAxel

Test Plan

We added some initial tests on the storage test suite.
We will slowly move some of the existing tests that focus on "storage" to this test suite.

We have not made a decision on which storage engine the remaining high level test should be using. For now they will remain using sqlite /sqlite (in memory).

@codecov
Copy link

codecov bot commented Apr 4, 2023

Codecov Report

Merging #5212 (e53c6ca) into master (5faa4d0) will decrease coverage by 0.85%.
The diff coverage is 11.26%.

❗ Current head e53c6ca differs from pull request most recent head ebf566a. Consider uploading reports for the commit ebf566a to get more accurate results

@@            Coverage Diff             @@
##           master    #5212      +/-   ##
==========================================
- Coverage   55.44%   54.60%   -0.85%     
==========================================
  Files         447      461      +14     
  Lines       63272    64284    +1012     
==========================================
+ Hits        35083    35103      +20     
- Misses      25810    26783     +973     
- Partials     2379     2398      +19     
Impacted Files Coverage Δ
config/localTemplate.go 60.60% <ø> (ø)
...r/store/trackerdb/generickv/accounts_ext_reader.go 0.00% <0.00%> (ø)
...r/store/trackerdb/generickv/accounts_ext_writer.go 0.00% <0.00%> (ø)
...edger/store/trackerdb/generickv/accounts_reader.go 0.00% <0.00%> (ø)
...edger/store/trackerdb/generickv/accounts_writer.go 0.00% <0.00%> (ø)
ledger/store/trackerdb/generickv/catchpoint.go 0.00% <0.00%> (ø)
ledger/store/trackerdb/generickv/init_accounts.go 0.00% <0.00%> (ø)
ledger/store/trackerdb/generickv/migrations.go 0.00% <0.00%> (ø)
...store/trackerdb/generickv/onlineaccounts_reader.go 0.00% <0.00%> (ø)
...store/trackerdb/generickv/onlineaccounts_writer.go 0.00% <0.00%> (ø)
... and 28 more

... and 10 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@icorderi icorderi marked this pull request as ready for review April 4, 2023 21:16
@cce
Copy link
Contributor

cce commented Apr 12, 2023

Since you can do reads against pending pebble Batches (and even indexed iterators/scans if you use NewIndexedBatch) to see a merged view of the batch and underlying DB — WDYT of exposing this ability to the interface, so that it is possible in certain cases to read against pending writes, like SQL does?
https://pkg.go.dev/github.com/cockroachdb/pebble#Batch

@icorderi icorderi force-pushed the feat/kv-base branch 2 times, most recently from 2e3cb9b to 22a8c70 Compare May 16, 2023 16:23
@icorderi icorderi marked this pull request as draft May 18, 2023 16:28
@icorderi icorderi force-pushed the feat/kv-base branch 3 times, most recently from be15df3 to 52c7ca7 Compare June 2, 2023 13:54
@icorderi icorderi force-pushed the feat/kv-base branch 3 times, most recently from 7514693 to f690185 Compare June 13, 2023 15:30
@icorderi icorderi force-pushed the feat/kv-base branch 4 times, most recently from 2876836 to f23c5bc Compare June 14, 2023 15:18
@algorandskiy
Copy link
Contributor

Merged as sub-PRs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants