[FOLD] Address feedback from Scott Schurr

src/ripple/basics/RangeSet.h

@@ -126,6 +126,7 @@ from_string(RangeSet<T>& rs, std::string const& s)
  std::vector<std::string> tokens;
  bool result{true};
 
+ rs.clear();
  boost::split(tokens, s, boost::algorithm::is_any_of(","));
  for (auto const& t : tokens)
  {

src/ripple/nodestore/DatabaseShard.h

@@ -134,7 +134,7 @@ class DatabaseShard : public Database
  @return Information about shards held by this node
  */
  [[nodiscard]] virtual std::unique_ptr<ShardInfo>
- getShardInfo() = 0;
+ getShardInfo() const = 0;
 
  /** Returns the root database directory
  */

src/ripple/nodestore/impl/Database.cpp

@@ -45,7 +45,7 @@ Database::Database(
  DEFAULT_LEDGERS_PER_SHARD))
  , earliestLedgerSeq_(
  get<std::uint32_t>(config, "earliest_seq", XRP_LEDGER_EARLIEST_SEQ))
- , earliestShardIndex_(seqToShardIndex(earliestLedgerSeq_))
+ , earliestShardIndex_((earliestLedgerSeq_ - 1) / ledgersPerShard_)
 {
  if (ledgersPerShard_ == 0 || ledgersPerShard_ % 256 != 0)
  Throw<std::runtime_error>("Invalid ledgers_per_shard");

src/ripple/nodestore/impl/DatabaseShardImp.cpp

@@ -117,7 +117,7 @@ DatabaseShardImp::init()
  if (!app_.config().standalone() && !historicalPaths_.empty())
  {
  // Check historical paths for duplicated file systems
- if (!checkHistoricalPaths())
+ if (!checkHistoricalPaths(lock))
  return false;
  }
 
@@ -294,11 +294,9 @@ DatabaseShardImp::prepareLedger(std::uint32_t validLedgerSeq)
  std::lock_guard lock(mutex_);
  shards_.emplace(*shardIndex, std::move(shard));
  acquireIndex_ = *shardIndex;
+ updatePeers(lock);
  }
 
- // Update peers with shard info
- updatePeers();
-
  return ledgerSeq;
 }
 
@@ -403,9 +401,7 @@ DatabaseShardImp::prepareShards(std::vector<std::uint32_t> const& shardIndexes)
  for (auto const shardIndex : shardIndexes)
  preparedIndexes_.emplace(shardIndex);
 
- // Update peers with shard info
- updatePeers();
-
+ updatePeers(lock);
  return true;
 }
 
@@ -416,10 +412,7 @@ DatabaseShardImp::removePreShard(std::uint32_t shardIndex)
  assert(init_);
 
  if (preparedIndexes_.erase(shardIndex))
- {
- // Update peers with shard info
- updatePeers();
- }
+ updatePeers(lock);
 }
 
 std::string
@@ -451,10 +444,7 @@ DatabaseShardImp::importShard(
 
  // Remove the failed import shard index so it can be retried
  preparedIndexes_.erase(shardIndex);
-
- // Update peers with shard info
- updatePeers();
-
+ updatePeers(lock);
  return false;
  };
 
@@ -698,35 +688,10 @@ DatabaseShardImp::setStored(std::shared_ptr<Ledger const> const& ledger)
 }
 
 std::unique_ptr<ShardInfo>
-DatabaseShardImp::getShardInfo()
+DatabaseShardImp::getShardInfo() const
 {
- std::vector<std::weak_ptr<Shard>> shards;
- std::set<std::uint32_t> preparedIndexes;
-
- {
- std::lock_guard lock(mutex_);
-
- shards.reserve(shards_.size());
- for (auto const& e : shards_)
- shards.push_back(e.second);
-
- preparedIndexes = preparedIndexes_;
- }
-
- auto shardInfo{std::make_unique<ShardInfo>()};
- for (auto const& weak : shards)
- {
- if (auto const shard = weak.lock())
- {
- shardInfo->update(
- shard->index(), shard->getState(), shard->getProgress());
- }
- }
-
- for (auto const shardIndex : preparedIndexes)
- shardInfo->update(shardIndex, ShardState::queued, 0);
-
- return shardInfo;
+ std::lock_guard lock(mutex_);
+ return getShardInfo(lock);
 }
 
 void
@@ -1330,43 +1295,43 @@ DatabaseShardImp::finalizeShard(
  return;
 
  auto const boundaryIndex{shardBoundaryIndex()};
- if (shard->index() < boundaryIndex)
  {
- // This is a historical shard
- if (!historicalPaths_.empty() &&
- shard->getDir().parent_path() == dir_)
- {
- // Shard wasn't placed at a separate historical path
- JLOG(j_.warn()) << "shard " << shard->index()
- << " is not stored at a historical path";
- }
- }
- else
- {
- // Not a historical shard. Shift recent shards if necessary
- assert(!boundaryIndex || shard->index() - boundaryIndex <= 1);
  std::lock_guard lock(mutex_);
 
- relocateOutdatedShards(lock);
-
- auto& recentShard = shard->index() == boundaryIndex
- ? secondLatestShardIndex_
- : latestShardIndex_;
+ if (shard->index() < boundaryIndex)
+ {
+ // This is a historical shard
+ if (!historicalPaths_.empty() &&
+ shard->getDir().parent_path() == dir_)
+ {
+ // Shard wasn't placed at a separate historical path
+ JLOG(j_.warn()) << "shard " << shard->index()
+ << " is not stored at a historical path";
+ }
+ }
+ else
+ {
+ // Not a historical shard. Shift recent shards if necessary
+ assert(!boundaryIndex || shard->index() - boundaryIndex <= 1);
+ relocateOutdatedShards(lock);
 
- // Set the appropriate recent shard index
- recentShard = shard->index();
+ // Set the appropriate recent shard index
+ if (boundaryIndex == shard->index())
+ secondLatestShardIndex_ = boundaryIndex;
+ else
+ latestShardIndex_ = boundaryIndex;
 
- if (shard->getDir().parent_path() != dir_)
- {
- JLOG(j_.warn()) << "shard " << shard->index()
- << " is not stored at the path";
+ if (shard->getDir().parent_path() != dir_)
+ {
+ JLOG(j_.warn()) << "shard " << shard->index()
+ << " is not stored at the path";
+ }
  }
+
+ updatePeers(lock);
  }
 
  updateFileStats();
-
- // Update peers with shard info
- updatePeers();
  });
 }
 
@@ -1788,7 +1753,7 @@ DatabaseShardImp::chooseHistoricalPath(std::lock_guard<std::mutex> const&) const
 }
 
 bool
-DatabaseShardImp::checkHistoricalPaths() const
+DatabaseShardImp::checkHistoricalPaths(std::lock_guard<std::mutex> const&) const
 {
 #if BOOST_OS_LINUX
  // Each historical shard path must correspond
@@ -1868,18 +1833,32 @@ DatabaseShardImp::checkHistoricalPaths() const
  return true;
 }
 
-void
-DatabaseShardImp::updatePeers()
+std::unique_ptr<ShardInfo>
+DatabaseShardImp::getShardInfo(std::lock_guard<std::mutex> const&) const
 {
- if (app_.config().standalone() ||
-  app_.getOPs().getOperatingMode() == OperatingMode::DISCONNECTED)
+ auto shardInfo{std::make_unique<ShardInfo>()};
+ for (auto const& [_, shard] : shards_)
  {
- return;
+ shardInfo->update(
+ shard->index(), shard->getState(), shard->getPercentProgress());
  }
 
- auto const message{getShardInfo()->makeMessage(app_)};
- app_.overlay().foreach(send_always(
- std::make_shared<Message>(message, protocol::mtPEER_SHARD_INFO_V2)));
+ for (auto const shardIndex : preparedIndexes_)
+ shardInfo->update(shardIndex, ShardState::queued, 0);
+
+ return shardInfo;
+}
+
+void
+DatabaseShardImp::updatePeers(std::lock_guard<std::mutex> const& lock) const
+{
+ if (!app_.config().standalone() &&
+ app_.getOPs().getOperatingMode() != OperatingMode::DISCONNECTED)
+ {
+ auto const message{getShardInfo(lock)->makeMessage(app_)};
+ app_.overlay().foreach(send_always(std::make_shared<Message>(
+ message, protocol::mtPEER_SHARD_INFO_V2)));
+ }
 }
 
 //------------------------------------------------------------------------------

src/ripple/nodestore/impl/DatabaseShardImp.h

@@ -13,7 +13,7 @@
  ANY SPECIAL , DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
  WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
  ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
- OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.`
+ OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
 */
 //==============================================================================
 
@@ -74,7 +74,7 @@ class DatabaseShardImp : public DatabaseShard
  setStored(std::shared_ptr<Ledger const> const& ledger) override;
 
  std::unique_ptr<ShardInfo>
- getShardInfo() override;
+ getShardInfo() const override;
 
  boost::filesystem::path const&
  getRootDir() const override
@@ -278,11 +278,14 @@ class DatabaseShardImp : public DatabaseShard
  chooseHistoricalPath(std::lock_guard<std::mutex> const&) const;
 
  bool
- checkHistoricalPaths() const;
+ checkHistoricalPaths(std::lock_guard<std::mutex> const&) const;
+
+ std::unique_ptr<ShardInfo>
+ getShardInfo(std::lock_guard<std::mutex> const&) const;
 
  // Update peers with the status of every complete and incomplete shard
  void
- updatePeers();
+ updatePeers(std::lock_guard<std::mutex> const& lock) const;
 };
 
 } // namespace NodeStore

src/ripple/nodestore/impl/Shard.h

@@ -172,7 +172,7 @@ class Shard final
  the current state of the shard is.
  */
  [[nodiscard]] std::uint32_t
- getProgress() const noexcept
+ getPercentProgress() const noexcept
  {
  return calculatePercent(progress_, maxLedgers_);
  }

src/ripple/overlay/impl/OverlayImpl.cpp

@@ -727,7 +727,6 @@ Json::Value
 OverlayImpl::crawlShards(bool includePublicKey, std::uint32_t relays)
 {
  using namespace std::chrono;
- using namespace std::chrono_literals;
 
  Json::Value jv(Json::objectValue);
 
@@ -748,33 +747,36 @@ OverlayImpl::crawlShards(bool includePublicKey, std::uint32_t relays)
  if (relays == 0 || size() == 0)
  return jv;
 
- // Wait if a request is in progress
- std::unique_lock<std::mutex> lock{csMutex_};
- if (!csIDs_.empty())
- csCV_.wait(lock);
-
  {
- std::lock_guard lock{mutex_};
- for (auto const& id : ids_)
- csIDs_.emplace(id.first);
- }
+ protocol::TMGetPeerShardInfoV2 tmGPS;
+ tmGPS.set_relays(relays);
 
- // Request peer shard info
- protocol::TMGetPeerShardInfoV2 tmGPS;
- tmGPS.set_relays(relays);
- foreach(send_always(
- std::make_shared<Message>(tmGPS, protocol::mtGET_PEER_SHARD_INFO_V2)));
+ // Wait if a request is in progress
+ std::unique_lock<std::mutex> csLock{csMutex_};
+ if (!csIDs_.empty())
+ csCV_.wait(csLock);
 
- if (csCV_.wait_for(lock, seconds(60)) == std::cv_status::timeout)
- {
- csIDs_.clear();
- csCV_.notify_all();
+ {
+ std::lock_guard lock{mutex_};
+ for (auto const& id : ids_)
+ csIDs_.emplace(id.first);
+ }
+
+ // Request peer shard info
+ foreach(send_always(std::make_shared<Message>(
+ tmGPS, protocol::mtGET_PEER_SHARD_INFO_V2)));
+
+ if (csCV_.wait_for(csLock, seconds(60)) == std::cv_status::timeout)
+ {
+ csIDs_.clear();
+ csCV_.notify_all();
+ }
  }
 
  // Combine shard info from peers
  hash_map<PublicKey, NodeStore::ShardInfo> peerShardInfo;
  for_each([&](std::shared_ptr<PeerImp>&& peer) {
- auto const& psi{peer->getPeerShardInfos()};
+ auto const psi{peer->getPeerShardInfos()};
  for (auto const& [publicKey, shardInfo] : psi)
  {
  auto const it{peerShardInfo.find(publicKey)};
@@ -812,7 +814,7 @@ void
 OverlayImpl::endOfPeerChain(std::uint32_t id)
 {
  // Notify threads if all peers have received a reply from all peer chains
- std::lock_guard lock{csMutex_};
+ std::lock_guard csLock{csMutex_};
  csIDs_.erase(id);
  if (csIDs_.empty())
  csCV_.notify_all();
@@ -876,7 +878,7 @@ OverlayImpl::getOverlayInfo()
  pv[jss::complete_ledgers] =
  std::to_string(minSeq) + "-" + std::to_string(maxSeq);
 
- auto const& peerShardInfos{sp->getPeerShardInfos()};
+ auto const peerShardInfos{sp->getPeerShardInfos()};
  auto const it{peerShardInfos.find(sp->getNodePublic())};
  if (it != peerShardInfos.end())
  {

src/ripple/overlay/impl/PeerImp.cpp

@@ -574,7 +574,7 @@ PeerImp::fail(std::string const& name, error_code ec)
  close();
 }
 
-hash_map<PublicKey, NodeStore::ShardInfo> const&
+hash_map<PublicKey, NodeStore::ShardInfo> const
 PeerImp::getPeerShardInfos() const
 {
  std::lock_guard l{shardInfoMutex_};

src/ripple/overlay/impl/PeerImp.h

@@ -391,7 +391,7 @@ class PeerImp : public Peer,
  fail(std::string const& reason);
 
  // Return any known shard info from this peer and its sub peers
- [[nodiscard]] hash_map<PublicKey, NodeStore::ShardInfo> const&
+ [[nodiscard]] hash_map<PublicKey, NodeStore::ShardInfo> const
  getPeerShardInfos() const;
 
  bool