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

dev docs: add documentation to the rocksdb configuration #392

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

cchudant
Copy link
Member

Pull Request type

  • (developer) documentation content changes

resolves #231
fixes the outstanding review comments in #379

cc @shamsasari

(note:
the thing i mention about snapshots for consistent db reads in the module docs is something i wanted to fix for a very long time, it's not a new concern. i was waiting for #292 to be merged to fix it.
it can't cause any problem in block production mode so that's why it hasn't yet been fixed)

Copy link
Member

@antiyro antiyro left a comment

Choose a reason for hiding this comment

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

lgtm thanks!

CHANGELOG.md Show resolved Hide resolved
crates/client/db/src/rocksdb_options.rs Show resolved Hide resolved
crates/client/db/src/rocksdb_options.rs Show resolved Hide resolved
//! That last point isn't yet implemented, as we need some changes to the rust rocksdb bindings, see [rust-rocksdb#937]. No issue has yet been
//! found with this as our db is almost only append-only - but this should nonetheless be fixed. (FIXME)
//!
//! This configuration makes a lot of sense because we making a blockchain node. All of our db writes are very big, all or nothing and infrequent.
Copy link
Contributor

Choose a reason for hiding this comment

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

This configuration makes a lot of sense in the context of a blockchain node.

Copy link
Member Author

Choose a reason for hiding this comment

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

thank you very much

crates/client/db/src/rocksdb_options.rs Show resolved Hide resolved
crates/client/db/src/rocksdb_options.rs Show resolved Hide resolved
@Trantorian1 Trantorian1 added the docs Documentation changes or improvements label Nov 20, 2024
Copy link
Collaborator

@Trantorian1 Trantorian1 left a comment

Choose a reason for hiding this comment

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

A few spelling mistakes. Also some of @shamsasari's RFCs have not been addressed yet.

crates/client/db/src/rocksdb_options.rs Show resolved Hide resolved
crates/client/db/src/rocksdb_options.rs Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation changes or improvements
Projects
Status: Backlog
Development

Successfully merging this pull request may close these issues.

feat(docs): Document rocksdb configuration
5 participants