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

Modularize beacon node backend #4718

Open
wants to merge 104 commits into
base: unstable
Choose a base branch
from

Conversation

eserilev
Copy link
Collaborator

@eserilev eserilev commented Sep 9, 2023

Issue Addressed

#4669

Proposed Changes

Modularize the beacon node backend to make it easier to add new database implementations

Additional Info

The existing KeyValueStore trait already does some abstraction, however the codebases assumes the KV store is always LevelDB.

I created a BeaconNodeBackend type that implements the KeyValueStore and ItemStore traits. I then replaced all references of LevelDb to the new BeaconNodeBackend type. Within the BeaconNodeBackend type I used the cfg macro which should allow us to switch between different database implementations via config changes.

@eserilev eserilev changed the title [WIP] Modularize beacon node backend Modularize beacon node backend Sep 9, 2023
@eserilev
Copy link
Collaborator Author

eserilev commented Jan 30, 2024

I'm currently working on adding Redb to the beacon node backend. Here are some notes I've taken so far on my work

Redb

Theres a few difference's between LevelDB and Redb that I'll describe below

fsync

Info about redb transaction durability can be found here
https://docs.rs/redb/latest/redb/enum.Durability.html#

For now I translate WriteOptions.sync = true to be Durability::Immediate and WriteOptions.sync = false to be Durability::Eventual

We could decide to use Paranoid instead of Immediate, but I think the additional guarantees of Paranoid are probably unecessary for our use case.

Compaction

LevelDB allows for compacting over a range of values. Redb only allows for compacting the full db. It seems like our LevelDB implementation compacts across all keys in the DB. If thats the case we should be all good here.

Tables

Redb introduces the concept of tables. We can either use one table for everything, or spin up tables by column name. I'm currently going down the path of spinning up tables by column name. This causes issues with do_atomically. We may need to make changes to this function to also accept a col: &str as an argument.

key.matches_column(column) && predicate(trimmed_key, trimmed_start_key)
})
.map(move |(bytes_key, value)| {
let key = bytes_key.remove_column_variable(column).ok_or_else(|| {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should add the metric here and increase DISK_DB_READ_BYTES with value to have the same meaning as the the usage with the per-item gettter

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok(Box::new(
iter.take_while(move |key| key.matches_column(column))
.map(move |bytes_key| {
metrics::inc_counter_vec(&metrics::DISK_DB_READ_COUNT, &[column.into()]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's not ideal to count reads of keys (this function) as reads of entries (other uses of DISK_DB_READ_COUNT). If this iterators just read keys, maybe we should use a different metric DISK_DB_KEY_READ_COUNT or similar

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yep makes sense, I've added separate metrics for db key read

@eserilev
Copy link
Collaborator Author

I've opened a separate issue around temp state cleanup performance for Redb. I'm going to handle that in a separate PR if thats ok with you guys

#6332

@eserilev
Copy link
Collaborator Author

eserilev commented Sep 1, 2024

Compaction in redb v2.1.2 is craaazy slow: cberner/redb#852

For now lets just make sure we stay on 2.1.1 until the issue is resolved

@eserilev
Copy link
Collaborator Author

the last compaction run took 7 min to complete

image

I think we were running an older version of Redb that had introduced a regression to compaction. I'm really hoping the new version of Redb, which fixed the regression and introduced some optimizations, will improve those times. We should see compaction run on gown on sep 23rd.. will update here once that runs.

If compaction speeds still suck, one option is to have two separate databases for redb. The first contains the tables we dont want to run compaction for, the second contains tables we do want to run compaction for. That way we can run compaction against these three tables:

  DBColumn::BeaconState,
  DBColumn::BeaconStateSummary,
  DBColumn::BeaconBlock,

And not against the full set of tables (mimicking leveldb functionality).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
database ready-for-review The code is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants