-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Persist ledger #3926
Persist ledger #3926
Conversation
Improve Relational DB Interface safety checks
* @return True if the shard store exists | ||
*/ | ||
bool | ||
shardStoreExists() |
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 function can be const and declared [[nodiscard]].
* @return True if node ledger DB exists. | ||
*/ | ||
bool | ||
existsLedger() | ||
{ | ||
return !!lgrdb_; | ||
return static_cast<bool>(lgrdb_); |
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.
Likely outside this PR's scope but since you are touching this function, it may be a good idea to make it const and declare [[nodiscard]].
* @return True if node transaction DB exists. | ||
*/ | ||
bool | ||
existsTransaction() | ||
{ | ||
return !!txdb_; | ||
return static_cast<bool>(txdb_); |
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.
Likely outside this PR's scope but since you are touching this function, it may be a good idea to make it const and declare [[nodiscard]].
if (existsLedger() && existsTransaction()) | ||
{ | ||
if (!ripple::saveValidatedLedger( | ||
*lgrdb_, *txdb_, app_, ledger, current)) | ||
return false; | ||
} | ||
|
||
if (auto shardStore = app_.getShardStore()) | ||
if (auto shardStore = app_.getShardStore(); shardStore) |
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 second condition check is redundant. Should be: if (auto shardStore = app_.getShardStore())
return false; | ||
} | ||
return true; | ||
}); |
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 don't believe it's necessary to use an info
local. Here is a slightly optimized alternate version.
if (res = ripple::getNewestLedgerInfo(session, j_); res)
return false;
return true;
iterateLedgerForward( | ||
seqToShardIndex(ledgerFirstIndex), | ||
[&](soci::session& session, std::uint32_t shardIndex) { | ||
if (auto info = ripple::getLimitedOldestLedgerInfo( |
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.
Same comment as above pertaining to info
.
std::optional<LedgerInfo> res; | ||
iterateLedgerBack( | ||
{}, [&](soci::session& session, std::uint32_t shardIndex) { | ||
if (auto info = ripple::getLimitedNewestLedgerInfo( |
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.
Same comment as above pertaining to info
.
@miguelportilla Please only review this commit: 8e5c768 Sorry about this--this commit sits upon another commit that has already been approved but not yet in develop. |
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.
Option fix you might want to sneak in. The documentation for iterateTransactionBack
is incorrect. It states iterateTransactionForward
versus iterateTransactionBack
I left a few comments for the first commit, nothing critical. If we decide to postpone those changes now, they can easily be addressed in a future PR.
The second commit LGTM. Thank you for your fixes, Mark!
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 only reviewed 8e5c768
High Level Overview of Change
Context of Change
Type of Change