Vacuum feature #411
Vacuum feature #411
Conversation
rai/node/node.hpp
Outdated
| @@ -492,6 +492,7 @@ class node : public std::enable_shared_from_this <rai::node> | |||
| { | |||
| alarm.service.post (action_a); | |||
| } | |||
| void copy_with_compaction (); | |||
| void send_keepalive (rai::endpoint const &); | |||
There was a problem hiding this comment.
I avoided fixing existing indentation problems, since there's a patch already for that.
rai/node/node.cpp
Outdated
| @@ -1527,6 +1527,17 @@ rai::node::~node () | |||
| stop(); | |||
| } | |||
|
|
|||
| void rai::node::copy_with_compaction () | |||
| { | |||
| auto vacuum_path = application_path / "vacuumed.ldb"; | |||
There was a problem hiding this comment.
The output file path should probably an argument.
This might not need to be a function either, since it's pretty common practice throughout the code to access node.store directly.
There was a problem hiding this comment.
Not sure why that would be useful. The fact that it's a separate file is an implementation detail that the user shouldn't are about. In fact, it should probably write to a tmp-file with a unique name.
There was a problem hiding this comment.
How does the caller know what the output file name is, to use it?
There was a problem hiding this comment.
Well, that would need to be communicated if I change this to use tmp-files. But for now, using a fixed name seems reasonable enough - it's not like anyone should put conflicting ldb files into the data folder :)
There was a problem hiding this comment.
You could move this entirely into the other block, since it only has one caller, and only one potential caller with the hardcoded name - and avoid spreading the hardcoded names any further.
There was a problem hiding this comment.
Please note the comment above copy_with_compaction. The inactive_node needs to go out of scope before renaming is possible, otherwise the data.ldb file is locked (mdb_env closes the files when the node goes out of scope)
There was a problem hiding this comment.
auto data_path = application_path / "data.ldb";
auto vacuum_path = application_path / "vacuumed.ldb";
bool success = false;
{
inactive_node node (data_path);
success = !mdb_env_copy2 (node.node->store.environment.environment, vacuum_path.string ().c_str (), MDB_CP_COMPACT);
}
if (success)
{
std::cout << "Finalizing" << std::endl;
// Note that these throw on failure
boost::filesystem::remove (data_path);
boost::filesystem::rename (vacuum_path, data_path);
std::cout << "Vacuum completed" << std::endl;
}
There was a problem hiding this comment.
Or, if it's really a useful function that needs to be on node, it should be more generic and either return the path or take a path. Either way makes sense.
There was a problem hiding this comment.
Yeah, I can make it take a path, makes sense.
rai/node/node.cpp
Outdated
| std::cout << "Finalizing" << std::endl; | ||
|
|
||
| // Note that these throw on failure | ||
| boost::filesystem::remove (data_path / "data.ldb"); |
There was a problem hiding this comment.
Maybe rename this to backup.ldb instead of removing, and remove backup.ldb?
There was a problem hiding this comment.
Could you elaborate? The last step after mdb's compaction copy is to replace the original with the vacuumed copy.
There was a problem hiding this comment.
It would be a nice bonus to retain the previous data.ldb for at least one more run, in case something bad happens (not indicated by the return code) during the vacuum.
There was a problem hiding this comment.
something like:
generate vacuum.ldb
delete backup.ldb
rename data.ldb backup.ldb
rename vacuum.ldb data.ldb
then you're left with backup.ldb and data.ldb at the end of the process.
There was a problem hiding this comment.
@lukealonso convinced me to retain the pre-vacuum db as a backup. I'll prepare a new commit.
|
@androm3da The backup functionality is already committed. I'll resolve the conflicts after the flurry :) |
|
@androm3da rebased |
|
@androm3da Good to go? Still says requested changes after the last commit. |
rai/node/node.cpp
Outdated
| boost::filesystem::path data_path; | ||
| if (vm.count ("data_path")) | ||
| { | ||
| data_path = boost::filesystem::path (vm["data_path"].as <std::string> ()); |
There was a problem hiding this comment.
Please change this to a ternary expression.
rai/node/node.cpp
Outdated
| } | ||
| catch (const boost::filesystem::filesystem_error& ex) | ||
| { | ||
| std::cout << "Vacuum failed during a file operation: " << ex.what() << std::endl; |
There was a problem hiding this comment.
Please write this to std:cerr instead.
rai/node/node.cpp
Outdated
| } | ||
| catch (...) | ||
| { | ||
| std::cout << "Vacuum failed" << std::endl; |
|
@androm3da updated, thanks for the feedback. |
cryptocode
force-pushed
the
compaction
branch
from
1ce6959
to
3b5f0a0
Compare
|
from https://travis-ci.org/clemahieu/raiblocks/jobs/327608083 |
cryptocode
force-pushed
the
compaction
branch
from
3b5f0a0
to
01fc734
Compare
|
@androm3da CI succeeded on the first commits, the failure comes on the last commit where I changed to the ternary, as you requested. I have a hard time imagining that causing a double free ;) A lot of CI tsan errors also appear in CI failures in commits on master. Could that be the reason for the bootstrap_no_publish issue? Also, won't the failures on master trickle into rebased PR's? |
cryptocode
force-pushed
the
compaction
branch
from
01fc734
to
00ef971
Compare
This PR introduces a --vacuum feature on rai_node / rai_wallet.
Test case, 13GB wallet on a fast SSD:
Since lmdb doesn't offer inline vacuuming, the vacuuming dumps live mdb pages to a copy of the db, which is finally replaced by the compacted db.
A general question about the code base: There doesn't seem to be a good place for shared constants (such as strings) in the various subprojects. Should we introduce some sort of
constants.hpp? Sprinkling copies of string constants like "data.ldb" around is brittle and hard to maintain.