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

Remove RocksDB #1058

Merged
merged 4 commits into from
Dec 13, 2023
Merged

Remove RocksDB #1058

merged 4 commits into from
Dec 13, 2023

Conversation

ogenev
Copy link
Member

@ogenev ogenev commented Dec 4, 2023

What was wrong?

Remove RocksDB and switch to SQLite for storing content values.

How was it fixed?

  • Remove RocksDB code and library.
  • Add content_value column to SQLite main db table and use this column to store and lookup data.
  • Add a "network" column to the main db table
  • Remove purge_db binary.
  • Remove state network TrieDB because it is based on RocksDB.

Tested this PR locally with portal-hive.

To-Do

@ogenev ogenev self-assigned this Dec 4, 2023
@ogenev ogenev force-pushed the remove-rocksdb branch 5 times, most recently from a854cec to 2103ffa Compare December 4, 2023 10:12
@ogenev ogenev marked this pull request as ready for review December 4, 2023 11:31
Copy link
Member

@KolbyML KolbyML left a comment

Choose a reason for hiding this comment

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

Looks good a few minor concerns but nothing blocking.

.circleci/config.yml Show resolved Hide resolved

const INSERT_QUERY: &str =
"INSERT OR IGNORE INTO content_metadata (content_id_long, content_id_short, content_key, content_size)
VALUES (?1, ?2, ?3, ?4)";
"INSERT OR IGNORE INTO content_data (content_id_long, content_id_short, content_key, content_value, content_size)
Copy link
Member

@KolbyML KolbyML Dec 6, 2023

Choose a reason for hiding this comment

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

Before we deploy this change we should add a field to tag which network the content is for. We can probably extract it from the content_key, but the properties letting this hold true won't work for custom networks built on trin. I also want to avoid byte parsing when calculating what distribution of storage is taken by each network.

So if we can have a network tag field that would be super nice to avoid the 2 cases above. This doesn't have to be done in this PR. But it should be done before we deploy these breaking changes so we don't have to deploy breaking changes twice.

The reason for this change is so we can properly report how much of storage is being taken by each network. Right now we are incorrectly reporting this metric

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I agree, it also makes sense to use a separate db table per network, we will discuss this.

.circleci/config.yml Show resolved Hide resolved
Cargo.toml Show resolved Hide resolved
trin-state/Cargo.toml Outdated Show resolved Hide resolved
@@ -1,181 +0,0 @@
use anyhow::Result;
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's not obvious to me that this is worth removing rather than transforming it to work with sqlite? I guess ultimately, we're not currently using this script in devops. But at some point I imagine we will? But maybe that's a big effort, and not really worth tackling at this point.

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought about this but we still don't know what exactly our database would look like when we refactor it to support beacon and state. I think it is more productive to remove it now and rewrite it if needed at a later stage.

portalnet/src/storage.rs Outdated Show resolved Hide resolved
portalnet/src/storage.rs Outdated Show resolved Hide resolved
@ogenev ogenev force-pushed the remove-rocksdb branch 2 times, most recently from b851560 to 59e6642 Compare December 7, 2023 08:45
portalnet/src/storage.rs Outdated Show resolved Hide resolved
@ogenev
Copy link
Member Author

ogenev commented Dec 8, 2023

Added a "network" column to the main db table as discussed in Trin's sync.

@ogenev ogenev force-pushed the remove-rocksdb branch 2 times, most recently from 835a748 to 2ac6f3e Compare December 11, 2023 08:33
@ogenev ogenev changed the base branch from master to remove-rocksdb December 13, 2023 08:04
@ogenev
Copy link
Member Author

ogenev commented Dec 13, 2023

@njgheorghita Going to merge this to the upstream remove-rocksdb branch.

@ogenev ogenev merged commit 819a47c into ethereum:remove-rocksdb Dec 13, 2023
2 checks passed
@ogenev ogenev deleted the remove-rocksdb branch December 13, 2023 08:23
ogenev added a commit that referenced this pull request Dec 15, 2023
* refactor: remove RocksDB

* refactor: remove `purge_db`d

* fix: address remove RocksDB PR comments

* feat(db): add "network" column to content table
ogenev added a commit that referenced this pull request Jan 3, 2024
* refactor: remove RocksDB

* refactor: remove `purge_db`d

* fix: address remove RocksDB PR comments

* feat(db): add "network" column to content table
ogenev added a commit that referenced this pull request Jan 8, 2024
* refactor: remove RocksDB

* refactor: remove `purge_db`d

* fix: address remove RocksDB PR comments

* feat(db): add "network" column to content table
ogenev added a commit that referenced this pull request Jan 10, 2024
* refactor: remove RocksDB

* refactor: remove `purge_db`d

* fix: address remove RocksDB PR comments

* feat(db): add "network" column to content table
ogenev added a commit that referenced this pull request Jan 12, 2024
* refactor: remove RocksDB

* refactor: remove `purge_db`d

* fix: address remove RocksDB PR comments

* feat(db): add "network" column to content table
ogenev added a commit that referenced this pull request Jan 15, 2024
* refactor: remove RocksDB

* refactor: remove `purge_db`d

* fix: address remove RocksDB PR comments

* feat(db): add "network" column to content table
ogenev added a commit that referenced this pull request Jan 15, 2024
* refactor: remove RocksDB

* refactor: remove `purge_db`d

* fix: address remove RocksDB PR comments

* feat(db): add "network" column to content table
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.

4 participants