migrate_database_lmdb_to_rocksdb improvements #4647
Conversation
|
This should be using node logger not cout, otherwise logs aren't saved to disk. There are some places in the existing code that are using cout too, but those are leftovers. Also, printing progress every x converted entries would be nice. |
|
I have been unable to get node logger implemented. I don't think it's important since the migration is a one time process.
|
And that's exactly the reason why it should be logged well, in line with production ready code. In case something goes wrong there should be a persistent record. |
RickiNano
force-pushed
the
lmdb-to-rocksdb
branch
from
ce5a2f3
to
48d4e6f
Compare
|
@pwojcikdev |
RickiNano
force-pushed
the
lmdb-to-rocksdb
branch
from
cdfaa6a
to
e8ba8ba
Compare
nano/nano_node/entry.cpp
Outdated
| } | ||
|
|
||
| int main (int argc, char * const * argv) | ||
| { | ||
| nano::set_umask (); // Make sure the process umask is set before any files are created | ||
| nano::logger::initialize (nano::log_config::cli_default ()); | ||
|
|
||
| nano::set_file_descriptor_limit (OPEN_FILE_DESCRIPTORS_LIMIT); | ||
| auto const file_descriptor_limit = nano::get_file_descriptor_limit (); | ||
| nano::default_logger ().info (nano::log::type::daemon, "File descriptors limit: {}", file_descriptor_limit); |
There was a problem hiding this comment.
This shouldn't be moved, this won't log anything.
There was a problem hiding this comment.
We could split setting the file descriptor limit with logging the warning about it being too low, though setting the file descriptor limit was moved here since it was only set when called with "--daemon" before and it needed to be set at least for "--migrate_database_lmdb_to_rocksdb" if not other commands when used with rocksdb when it has many files.
There was a problem hiding this comment.
Setting the limit here is not a problem, it's a good place for it. The problem is trying to log when logging is still in CLI mode. CLI commands usually have a very specific output format and to avoid polluting it with node details, only critical errors are logged. The warning probably needs to be logged via cerr, and the "File descriptors limit: {}"... line moved back into the daemon code, so it leaves a trace in log files if we ever need to debug it.
nano::log_config nano::log_config::cli_default ()
{
log_config config{};
config.default_level = nano::log::level::critical;
config.console.colors = false; // to avoid printing warning about cerr and colors
config.console.to_cerr = true; // Use cerr to avoid interference with CLI output that goes to stdout
config.file.enable = false;
return config;
}
There was a problem hiding this comment.
I pushed a commit which should fix the logging. Also, as setting the limit was moved away from daemon class, targets other than cli wallet would no longer set it.
nano/secure/ledger.cpp
Outdated
| auto rocksdb_store = nano::make_store (logger, data_path_a, nano::dev::constants, false, true, rocksdb_config); | ||
|
|
||
| if (!rocksdb_store->init_error ()) | ||
| { | ||
| logger.info (nano::log::type::ledger, "Step 1 of 7: Converting blocks table"); | ||
| std::atomic<std::size_t> count = 0; | ||
| auto refresh_interval = 20ms; |
There was a problem hiding this comment.
Why such a short refresh interval?
pwojcikdev
force-pushed
the
lmdb-to-rocksdb
branch
from
6a2923b
to
fa9180f
Compare
…ons will be able to open more files.
RickiNano
force-pushed
the
lmdb-to-rocksdb
branch
from
3ebe2fb
to
8191ce7
Compare
The
migrate_database_lmdb_to_rocksdboption is running for a very long time with the current ledger size (almost 200 million blocks). On my local machine it took 65 minutes to complete. Nothing is written on screen during this process and users may think the process has stalled.This PR adds some progress feedback. One update for each of the 7 tables that are migrated.
It also adds a simple disk space check to warn users if they might not have enough space to complete the migration.
The current converted RocksDb database is 73 GB, and the warning is given if the system has less than 75GB available.The warning is given based on the size of the LMDB database that is being migrated. The final RocksDb size is approximately 65% of the LMDB space.