-
Notifications
You must be signed in to change notification settings - Fork 211
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
kvdb-rocksdb: configurable memory budget per column #256
Conversation
I think, we need to make configurable the following things:
|
Relates to openethereum/parity-ethereum#11233 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to avoid heavy migration on adding a column due to change in rocksdb config parameters (esp. painful for archive nodes).
Not sure I understand this last part, can you elaborate?
Each time we add a new column in parity-ethereum, it kicks off a migration process, the budget for each column changes, which in turn may trigger compaction process that might take quite some time on archive nodes, so we would like to avoid that. (cc openethereum/parity-ethereum#11233) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea here is to make minimal changes to leave the old API as intact as possible to avoid breakage for consuming code?
Random thoughts: I think using memory_budget
instead of columns
creates a readability problem.
I wonder if we should try to make the DatabaseConf
a richer type with a columns()
getter returning all the data we need to manage a column family.
Or maybe DatabaseConf.columns
should be an IndexMap
so we can access it both by name and index, and have it store a new ColFam
type where we store, dunno, name
, mem_budget
and the cf_handle
?
// Memtable size is controlled by the option `write_buffer_size`. | ||
// If you increase your memtable size, be sure to also increase your L1 size! | ||
// L1 size is controlled by the option `max_bytes_for_level_base`. | ||
opts.set_parsed_options(&format!("max_bytes_for_level_base={}", budget)).map_err(other_io_err)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the advice is to set max_bytes_for_level
to 2x the memtable size?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've set it to the same value as write_buffer_size
, couldn't find any advice on that.
/// Set number of columns | ||
pub columns: Option<u32>, | ||
/// Specify the maximal number of info log files to be kept. | ||
pub keep_log_file_num: i32, | ||
} | ||
|
||
impl DatabaseConfig { | ||
/// Create new `DatabaseConfig` with default parameters and specified set of columns. | ||
/// Note that cache sizes must be explicitly set. | ||
pub fn with_columns(columns: Option<u32>) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't new_with_columns
be a more idiomatic name for a constructor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If changing any of the options we expose requires a migration or risk making the DB inaccessible we need to document that thoroughly. I'm thinking about things like initial_file_size
or block_size
: can they be changed freely?
* master: upgrade tiny-keccak to 2.0 (#260)
40ac3f7
to
c553a3f
Compare
@dvdplm I've changed the impl to keep the |
Yes, but I'm open to ideas how to make the API more ergonomic while keeping the migration overhead low. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm modulo a couple of minor nits. One thing that would be good is having more module level docs explaining how to configure memory.
I'm not sure how important it is to make concessions to keep changes to a minimum. If we need a migration anyway, we might just as well make the API easy to use? Either way, This is better than what we have so imo let's go ahead and merge it.
opts.optimize_level_style_compaction(memory_budget_per_col as i32); | ||
opts.set_target_file_size_base(self.compaction.initial_file_size); | ||
|
||
opts.set_parsed_options("compression_per_level=").map_err(other_io_err)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not new code but shouldn't there be a value passed in here? (set_parsed_option
is pretty terrible imo).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was added in openethereum/parity-ethereum@3cf1aac by @andresilva with the intention of enabling compression on all levels, otherwise optimize_level_style_compaction will disable compression for the first two levels IIUC. #257 uses https://docs.rs/rocksdb/0.13.0/rocksdb/struct.Options.html#method.set_compression_per_level instead of set_parsed_option
.
…ritytech/parity-common into ao-rocksdb-configurable-column-budget * 'ao-rocksdb-configurable-column-budget' of github.com:paritytech/parity-common: Update kvdb-rocksdb/src/lib.rs kvdb-rocksdb: other minor improvements kvdb-rocksdb: make column_config a method on config kvdb-rocksdb: make memory_budget_per_col private kvdb-rocksdb: small cleanup kvdb-rocksdb: less invasive changes kvdb-rocksdb: export keep_log_file_num kvdb-rocksdb: configurable memory budget per column
The API for setting the write limiter is not available upstream and we only want to set it for an HDD, which is a poor target for parity-ethereum-like workload anyway.
I've updated the default block size for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
* master: kvdb-rocksdb: configurable memory budget per column (#256)
* master: Compile triehash for no_std (#280) [kvdb-rocksdb] Use "pinned" gets to avoid allocations (#274) [kvdb-rocksdb] Release 0.2 (#273) [kvdb-rocksdb] switch to upstream (#257) travis: try to fix wasmpack chrome test on macOS (#263) Use 2018 edition for rustfmt (#266) [fixed-hash]: re-export `alloc_` (#268) kvdb-web: async-awaitify (#259) kvdb-rocksdb: configurable memory budget per column (#256) Bump rlp crate version. (#270) Introduce Rlp::at_with_offset method. (#269) Make fixed-hash test structs public (#267) Migrate primitive types to 2018 edition (#262) upgrade tiny-keccak to 2.0 (#260)
Closes #135.
This is a breaking change (changes
DatabaseConfig
, makesmemory_budget_per_col
private, removedwrite_rate_limit
option fromCompactionProfile
) .