Move shard store init to Application #2995
Conversation
Jenkins Build SummaryBuilt from this commit Built at 20190726 - 20:23:32 Test Results
|
miguelportilla
force-pushed
the
shardstore_init
branch
from
9a390a2
to
ffe0cc3
Compare
miguelportilla
force-pushed
the
shardstore_init
branch
from
ffe0cc3
to
b288a78
Compare
src/ripple/app/main/Application.cpp
Outdated
| std::vector<std::string> | ||
| getSchema (DatabaseCon& dbc, std::string const& dbName) | ||
| void | ||
| ApplicationImp::updateTxnDB() |
There was a problem hiding this comment.
This method looks like ancient database migration code dating back to 2013. I can't imagine we even support such an upgrade anymore. We should remove this migration code. If we don't want to do it here, I'd open an issue to track it.
There was a problem hiding this comment.
Agreed, removed. 👍
There was a problem hiding this comment.
The AccountTransactions has a primary key check can probably also be removed but I'll leave it in unless someone feels strongly about removing it.
miguelportilla
added
the
Passed
miguelportilla
force-pushed
the
shardstore_init
branch
from
e96f9e9
to
271d9c9
Compare
miguelportilla
removed
the
Passed
|
@mellery451 @jwbusch I added a new commit (to be folded) that has additional changes belonging to this PR. Apologies for adding the additional work, it was going to be a separate PR but the changes really belong here. Thanks! |
| @@ -21,6 +21,7 @@ | |||
| #include <ripple/app/ledger/TransactionMaster.h> | |||
| #include <ripple/app/misc/NetworkOPs.h> | |||
| #include <ripple/app/misc/SHAMapStoreImp.h> | |||
| #include <ripple/app/misc/SHAMapStoreImp.h> | |||
There was a problem hiding this comment.
this include got repeated for some reason
There was a problem hiding this comment.
Bizarre, thanks. 👍
miguelportilla
force-pushed
the
shardstore_init
branch
3 times, most recently
from
c72bef7
to
85da4d4
Compare
| else | ||
| boost::filesystem::create_directories(dir_); | ||
|
|
||
| if (!get_if_exists<std::uint64_t>(section, "max_size_gb", maxDiskSpace_)) |
There was a problem hiding this comment.
I'm pretty sure the existing comparisons with this value are in terms of bytes, but this value is gb. The old code converted to bytes before passing to this class, so we should probably do it here after reading and checking overflow.
miguelportilla
force-pushed
the
shardstore_init
branch
from
72a1799
to
9030162
Compare
Codecov Report
@@ Coverage Diff @@
## develop #2995 +/- ##
===========================================
- Coverage 69.46% 69.45% -0.02%
===========================================
Files 709 709
Lines 56448 56271 -177
===========================================
- Hits 39211 39081 -130
+ Misses 17237 17190 -47
Continue to review full report at Codecov.
|
| if (!get_if_exists<std::uint64_t>(section, "max_size_gb", maxDiskSpace_)) | ||
| return fail("'max_size_gb' missing"); | ||
|
|
||
| if (maxDiskSpace_ < minDiskSpace_) |
There was a problem hiding this comment.
is minDiskSpace also in GB? does it need to be converted to bytes too?
There was a problem hiding this comment.
Might be helpful to use labels for units -> minDiskSpace_ means it's in bytes, minDiskSpaceGB_ means it's in gigabytes... since we don't have any types for these units afaik. Maybe we need some std::chrono-like system :)
There was a problem hiding this comment.
My bad, minDiskSpace is always in GB and before the conversion (at this point), so is maxDiskSpace_. So the code is correct as is. I am not a fan of the label idea but using a temp here and assigning a converted value to maxDiskSpace_ will help avoid confusion. I'll also narrow minDiskSpace's scope to be local since its only used in one place.
miguelportilla
force-pushed
the
shardstore_init
branch
2 times, most recently
from
3a1ff20
to
5e4215e
Compare
src/ripple/core/impl/Stoppable.cpp
Outdated
| parent.isStopping() || | ||
| std::addressof(m_root) != std::addressof(parent.m_root)) | ||
| { | ||
| LogicError("Invalid parent"); |
There was a problem hiding this comment.
can you elaborate on the switch from assert to LogicError? I feel like these would be the result of programming errors and should still be asserts but I might be thinking about it wrong.
There was a problem hiding this comment.
I suggested it, as it's not something that could get accidentally #ifdef'd out. As soon as the programmer tried to run their program w/ the wrong code they'd hit the crash and have to fix it. But if they happened to be running w/ asserts off, they might miss it.
There was a problem hiding this comment.
@jwbusch That applies to all asserts. Nice thing is they get optimized away when turned off. I am leaning to asserting.
|
still 👍 with latest commit |
miguelportilla
force-pushed
the
shardstore_init
branch
from
439afc8
to
59e5bc0
Compare
miguelportilla
added
the
Passed
|
Thanks for the excellent feedback and review! @mellery451 @jwbusch |
miguelportilla
force-pushed
the
shardstore_init
branch
from
59e5bc0
to
18002e7
Compare
|
This PR includes & closes #2978. |
This PR is a small step in preparation for self-contained portable shards. The initialization of the shard store is moved to the Application object. Once the shard store relieves the node store of duty, there will be no need for the SHAMapStore class and will be eventually removed.