From 2fafd548515d43c63f04f30468890edcd6e275db Mon Sep 17 00:00:00 2001 From: Mark Travis Date: Thu, 23 Apr 2020 14:14:27 -0700 Subject: [PATCH] Review comment, and handle case of online_delete not having happened before. --- src/ripple/app/ledger/impl/LedgerMaster.cpp | 13 ++------ src/ripple/app/misc/SHAMapStore.h | 36 +++++++++++++++++++-- src/ripple/app/misc/SHAMapStoreImp.cpp | 18 ++++++++--- src/ripple/app/misc/SHAMapStoreImp.h | 8 +---- 4 files changed, 51 insertions(+), 24 deletions(-) diff --git a/src/ripple/app/ledger/impl/LedgerMaster.cpp b/src/ripple/app/ledger/impl/LedgerMaster.cpp index b6c1c0c372c..73ffb997713 100644 --- a/src/ripple/app/ledger/impl/LedgerMaster.cpp +++ b/src/ripple/app/ledger/impl/LedgerMaster.cpp @@ -149,8 +149,7 @@ static bool shouldAcquire ( std::uint32_t const currentLedger, std::uint32_t const ledgerHistory, - boost::optional minSeq, - std::optional const minimumOnline, + boost::optional const minimumOnline, std::uint32_t const candidateLedger, beast::Journal j) { @@ -164,13 +163,8 @@ shouldAcquire ( if (currentLedger - candidateLedger <= ledgerHistory) return true; - // Or if online deletion is enabled, whether greater than or equal to - // the minimum ledger to keep online. - if (minimumOnline.has_value()) - return candidateLedger >= *minimumOnline; - - // Or greater than or equal to the minimum ledger in SQLite, if any. - return minSeq.has_value() && candidateLedger >= *minSeq; + // Or if greater than or equal to a specific minimum ledger. + return minimumOnline.has_value() && candidateLedger >= *minimumOnline; }(); JLOG (j.trace()) @@ -1891,7 +1885,6 @@ void LedgerMaster::doAdvance (std::unique_lock& sl) "tryAdvance discovered missing " << *missing; if ((mFillInProgress == 0 || *missing > mFillInProgress) && shouldAcquire(mValidLedgerSeq, ledger_history_, - minSqlSeq(), app_.getSHAMapStore().minimumOnline(), *missing, m_journal)) { diff --git a/src/ripple/app/misc/SHAMapStore.h b/src/ripple/app/misc/SHAMapStore.h index 350ddaba061..d99dd4ae489 100644 --- a/src/ripple/app/misc/SHAMapStore.h +++ b/src/ripple/app/misc/SHAMapStore.h @@ -24,8 +24,8 @@ #include #include #include +#include #include -#include namespace ripple { @@ -70,8 +70,38 @@ class SHAMapStore virtual int fdRequired() const = 0; /** If online deletion is enabled, return the minimum ledger - * to keep. */ - virtual std::optional const minimumOnline() const = 0; + * to keep online, which is the lower bound for attempting to acquire + * historical ledgers over the peer to peer network. + * If online_delete is not enabled, then the minimum value to + * keep online is the minimum ledger that has been persisted already. + * + * With online_delete,this value is governed by the + * following circumstances: + * + * Upon process startup: + * + * With advisory_delete enabled and online_delete having been executed + * previously: + * Upon process startup, this value is set to one past the value + * that is allowed to be deleted. In other words, anything greater + * than what can be deleted should be acquired from the network and + * then retained. If nothing has yet been deleted, then do not + * rely upon the value of what is allowed to be deleted because it + * could cause fetching of more history than configured by + * ledger_history. + * + * Without advisory_delete or if online_delete has never executed: + * Upon process startup, this value is set to the earliest ledger + * that has been persisted in SQLite. + * + * Each time online_delete executes: + * + * Just prior to clearing SQL databases of historical ledgers, move + * the value forward to one past the greatest ledger being deleted. + * This minimizes fetching of ledgers that are in the process of being + * deleted. + */ + virtual boost::optional minimumOnline() const = 0; }; //------------------------------------------------------------------------------ diff --git a/src/ripple/app/misc/SHAMapStoreImp.cpp b/src/ripple/app/misc/SHAMapStoreImp.cpp index 4b6e456eeb8..06bcf06eafb 100644 --- a/src/ripple/app/misc/SHAMapStoreImp.cpp +++ b/src/ripple/app/misc/SHAMapStoreImp.cpp @@ -333,13 +333,15 @@ SHAMapStoreImp::run() if (advisoryDelete_) { canDelete_ = state_db_.getCanDelete(); - // On startup, don't acquire ledgers from the network that + + // On startup, don't acquire ledgers from the network that we know // can be deleted. - minimumOnline_ = canDelete_.load() + 1; + if (state_db_.getState().lastRotated) + minimumOnline_ = canDelete_.load() + 1; } - else + if (!minimumOnline_) { - // On startup, don't acquire ledgers from the network older than + // Or else don't acquire ledgers from the network older than // the earliest persisted, if any. auto const minSql = app_.getLedgerMaster().minSqlSeq(); if (minSql.has_value()) @@ -747,6 +749,14 @@ SHAMapStoreImp::onChildrenStopped() } } +boost::optional +SHAMapStoreImp::minimumOnline() const +{ + if (deleteInterval_) + return minimumOnline_.load(); + return app_.getLedgerMaster().minSqlSeq(); +} + //------------------------------------------------------------------------------ std::unique_ptr diff --git a/src/ripple/app/misc/SHAMapStoreImp.h b/src/ripple/app/misc/SHAMapStoreImp.h index a716d324d64..fa683887954 100644 --- a/src/ripple/app/misc/SHAMapStoreImp.h +++ b/src/ripple/app/misc/SHAMapStoreImp.h @@ -176,13 +176,7 @@ class SHAMapStoreImp : public SHAMapStore void rendezvous() const override; int fdRequired() const override; - std::optional const - minimumOnline() const override - { - if (deleteInterval_) - return minimumOnline_; - return {}; - } + boost::optional minimumOnline() const override; private: // callback for visitNodes