Skip to content

feat(store)!: Support for different key layout#2327

Merged
jmalicevic merged 61 commits intomainfrom
jasmina/1041-support-for-two-key-layouts
Mar 15, 2024
Merged

feat(store)!: Support for different key layout#2327
jmalicevic merged 61 commits intomainfrom
jasmina/1041-support-for-two-key-layouts

Conversation

@jmalicevic
Copy link
Collaborator

@jmalicevic jmalicevic commented Feb 13, 2024

Closes #2057 , #1041

If we keep the current design, we are also closing #1822

Supersedes #1764

This PR implements support for an additional DB key representation. The different key layout sorts the entries by height instead of lexicographically as in the current version of Comet.

When starting this work, we hoped that the new layout would significantly outperform the current layout. As we do not have sufficient real world evidence for this, this PR introduces a DB key layout interface that would allow Comet to easily integrate a more preferential key representation without major breaking changes.

The layout using ordercode is introduced as experimental, allowing users to easily experiment with this.

This layout was thoroughly tested as part of #1044 and all results will be in a report closing the mentioned PR.

Locally tested:

  • Empty stores get initialized with v2
  • Existing stores without a version key get initialized to v1 and the key is set
  • When a nodes' stores are deleted and we boot it up again that node uses v2 while the rest of the nodes use v1

PR checklist

  • Tests written/updated
  • Changelog entry added in .changelog (we use unclog to manage our changelog)
  • Updated relevant documentation (docs/ or spec/) and code comments
  • Title follows the Conventional Commits spec

@jmalicevic jmalicevic added the P:storage-optimization Priority: Give operators greater control over storage and storage optimization label Feb 13, 2024
@jmalicevic jmalicevic self-assigned this Feb 13, 2024
@jmalicevic jmalicevic changed the title feat(store)! :Support for different key layout feat(store)!: Support for different key layout Feb 13, 2024
base int64
height int64

dbKeyLayout BlockKeyLayout
Copy link
Contributor

Choose a reason for hiding this comment

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

Am I wrong in thinking that the BlockStore API is at a too low abstraction level to be effectively implemented in all suitable database backends?

It assumes a global key-value store and segregates relations with prefixes, while using multiple separate KV trees (if supported by the DB engine, which exist in the wild) might allow tuning specifically for different logical data tables. In particular, this API rules out an efficient SQL implementation.

This is an architectural musing, not necessarily calling for action in this PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@mzabaluev that's something to keep in mind, for sure 👍 However, I'm skeptical about switching to an SQL DB (SQLite) in v1 as it requires planning, execution, and evaluation (benchmarking, testing). Maybe this is something we should examine when picking a single DB storage.

@jmalicevic jmalicevic marked this pull request as ready for review February 22, 2024 14:51
@jmalicevic jmalicevic requested a review from a team as a code owner February 22, 2024 14:51
@jmalicevic jmalicevic marked this pull request as ready for review March 12, 2024 12:20
Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>
Copy link
Collaborator

@melekes melekes left a comment

Choose a reason for hiding this comment

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

👍

@jmalicevic jmalicevic enabled auto-merge March 15, 2024 07:31
@jmalicevic jmalicevic disabled auto-merge March 15, 2024 07:32
@jmalicevic jmalicevic enabled auto-merge March 15, 2024 07:43
@jmalicevic jmalicevic added this pull request to the merge queue Mar 15, 2024
Merged via the queue into main with commit 55638e8 Mar 15, 2024
@jmalicevic jmalicevic deleted the jasmina/1041-support-for-two-key-layouts branch March 15, 2024 07:54
@hvanz hvanz mentioned this pull request Mar 15, 2024
4 tasks
@melekes
Copy link
Collaborator

melekes commented Mar 18, 2024

@mergify backport v1.x

@mergify
Copy link
Contributor

mergify bot commented Mar 18, 2024

backport v1.x

✅ Backports have been created

Details

mergify bot pushed a commit that referenced this pull request Mar 18, 2024
Closes #2057 , #1041

If we keep the current design, we are also closing
#1822

Supersedes #1764

This PR implements support for an additional DB key representation. The
different key layout sorts the entries by height instead of
lexicographically as in the current version of Comet.

When starting this work, we hoped that the new layout would
significantly outperform the current layout. As we do not have
sufficient real world evidence for this, this PR introduces a DB key
layout interface that would allow Comet to easily integrate a more
preferential key representation without major breaking changes.

The layout using ordercode is introduced as experimental, allowing users
to easily experiment with this.

This layout was thoroughly tested as part of #1044 and all results will
be in a report closing the mentioned PR.

Locally tested:
- Empty stores get initialized with v2
- Existing stores without a version key get initialized to v1 and the
key is set
- When a nodes' stores are deleted and we boot it up again that node
uses v2 while the rest of the nodes use v1
<!--

Please add a reference to the issue that this PR addresses and indicate
which
files are most critical to review. If it fully addresses a particular
issue,
please include "Closes #XXX" (where "XXX" is the issue number).

If this PR is non-trivial/large/complex, please ensure that you have
either
created an issue that the team's had a chance to respond to, or had some
discussion with the team prior to submitting substantial pull requests.
The team
can be reached via GitHub Discussions or the Cosmos Network Discord
server in
the #cometbft channel. GitHub Discussions is preferred over Discord as
it
allows us to keep track of conversations topically.
https://github.com/cometbft/cometbft/discussions

If the work in this PR is not aligned with the team's current
priorities, please
be advised that it may take some time before it is merged - especially
if it has
not yet been discussed with the team.

See the project board for the team's current priorities:
https://github.com/orgs/cometbft/projects/1

-->

---

#### PR checklist

- [x] Tests written/updated
- [x] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [ ] Updated relevant documentation (`docs/` or `spec/`) and code
comments
- [x] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec

---------

Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>
(cherry picked from commit 55638e8)
melekes pushed a commit that referenced this pull request Mar 18, 2024
Closes #2057 , #1041

If we keep the current design, we are also closing
#1822

Supersedes #1764 

This PR implements support for an additional DB key representation. The
different key layout sorts the entries by height instead of
lexicographically as in the current version of Comet.

When starting this work, we hoped that the new layout would
significantly outperform the current layout. As we do not have
sufficient real world evidence for this, this PR introduces a DB key
layout interface that would allow Comet to easily integrate a more
preferential key representation without major breaking changes.

The layout using ordercode is introduced as experimental, allowing users
to easily experiment with this.

This layout was thoroughly tested as part of #1044 and all results will
be in a report closing the mentioned PR.
 
Locally tested:
- Empty stores get initialized with v2
- Existing stores without a version key get initialized to v1 and the
key is set
- When a nodes' stores are deleted and we boot it up again that node
uses v2 while the rest of the nodes use v1


---

#### PR checklist

- [x] Tests written/updated
- [x] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [ ] Updated relevant documentation (`docs/` or `spec/`) and code
comments
- [x] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec
<hr>This is an automatic backport of pull request #2327 done by
[Mergify](https://mergify.com).

Co-authored-by: Jasmina Malicevic <jasmina.dustinac@gmail.com>
@adizere adizere added this to the 2024-Q1 milestone Mar 27, 2024
cason pushed a commit that referenced this pull request Apr 5, 2024
github-merge-queue bot pushed a commit that referenced this pull request Mar 21, 2025
This was introduced in #2327, maybe by mistake with a double copy

Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P:storage-optimization Priority: Give operators greater control over storage and storage optimization

Projects

No open projects
Status: Done

4 participants