From d358495f0265a811a59bd7a20f431b4d93a6a006 Mon Sep 17 00:00:00 2001 From: Miguel Portilla Date: Wed, 3 Feb 2021 19:58:37 -0500 Subject: [PATCH] Add database counters: Fix bug where DatabaseRotateImp::getBackend and ::sync utilized the writable backend without a lock. ::getBackend was replaced with ::getCounters. --- src/ripple/nodestore/Backend.h | 36 +++++++++++++++---- src/ripple/nodestore/Database.h | 20 ++++++----- .../nodestore/backend/CassandraFactory.cpp | 2 +- .../nodestore/backend/MemoryFactory.cpp | 7 ---- src/ripple/nodestore/backend/NuDBFactory.cpp | 7 ---- src/ripple/nodestore/backend/NullFactory.cpp | 7 ---- .../nodestore/backend/RocksDBFactory.cpp | 7 ---- src/ripple/nodestore/impl/Database.cpp | 15 ++++---- src/ripple/nodestore/impl/DatabaseNodeImp.h | 13 +++---- .../nodestore/impl/DatabaseRotatingImp.cpp | 7 ++++ .../nodestore/impl/DatabaseRotatingImp.h | 18 +--------- .../nodestore/impl/DatabaseShardImp.cpp | 6 ---- src/ripple/nodestore/impl/DatabaseShardImp.h | 3 -- 13 files changed, 65 insertions(+), 83 deletions(-) diff --git a/src/ripple/nodestore/Backend.h b/src/ripple/nodestore/Backend.h index bc14c7711..a67af581d 100644 --- a/src/ripple/nodestore/Backend.h +++ b/src/ripple/nodestore/Backend.h @@ -39,13 +39,27 @@ namespace NodeStore { class Backend { public: + template struct Counters { - std::atomic writeDurationUs{0}; - std::atomic writeRetries{0}; - std::atomic writesDelayed{0}; - std::atomic readRetries{0}; - std::atomic readErrors{0}; + Counters() = default; + Counters(Counters const&) = default; + + template + Counters(Counters const& other) + : writeDurationUs(other.writeDurationUs) + , writeRetries(other.writeRetries) + , writesDelayed(other.writesDelayed) + , readRetries(other.readRetries) + , readErrors(other.readErrors) + { + } + + T writeDurationUs = {}; + T writeRetries = {}; + T writesDelayed = {}; + T readRetries = {}; + T readErrors = {}; }; /** Destroy the backend. @@ -143,8 +157,16 @@ public: virtual int fdRequired() const = 0; - virtual Counters const& - counters() const = 0; + /** Returns read and write stats. + + @note The Counters struct is specific to and only used + by CassandraBackend. + */ + virtual std::optional> + counters() const + { + return std::nullopt; + } /** Returns true if the backend uses permanent storage. */ bool diff --git a/src/ripple/nodestore/Database.h b/src/ripple/nodestore/Database.h index da0677292..25c4702e3 100644 --- a/src/ripple/nodestore/Database.h +++ b/src/ripple/nodestore/Database.h @@ -20,8 +20,6 @@ #ifndef RIPPLE_NODESTORE_DATABASE_H_INCLUDED #define RIPPLE_NODESTORE_DATABASE_H_INCLUDED -#include -#include #include #include #include @@ -178,9 +176,6 @@ public: virtual void sweep() = 0; - virtual Backend& - getBackend() = 0; - /** Gather statistics pertaining to read and write activities. * * @param obj Json object reference into which to place counters. @@ -258,10 +253,6 @@ protected: storeSz_ += sz; } - // Called by the public asyncFetch function - void - asyncFetch(uint256 const& hash, std::uint32_t ledgerSeq); - // Called by the public import function void importInternal(Backend& dstBackend, Database& srcDB); @@ -322,6 +313,17 @@ private: virtual void for_each(std::function)> f) = 0; + /** Retrieve backend read and write stats. + + @note The Counters struct is specific to and only used + by CassandraBackend. + */ + virtual std::optional> + getCounters() const + { + return std::nullopt; + } + void threadEntry(); }; diff --git a/src/ripple/nodestore/backend/CassandraFactory.cpp b/src/ripple/nodestore/backend/CassandraFactory.cpp index 5ac487d8b..a568ce58b 100644 --- a/src/ripple/nodestore/backend/CassandraFactory.cpp +++ b/src/ripple/nodestore/backend/CassandraFactory.cpp @@ -826,7 +826,7 @@ public: return 0; } - Counters const& + std::optional> counters() const override { return counters_; diff --git a/src/ripple/nodestore/backend/MemoryFactory.cpp b/src/ripple/nodestore/backend/MemoryFactory.cpp index e359fba11..e9433fd49 100644 --- a/src/ripple/nodestore/backend/MemoryFactory.cpp +++ b/src/ripple/nodestore/backend/MemoryFactory.cpp @@ -220,13 +220,6 @@ public: { return 0; } - - Counters const& - counters() const override - { - static Counters counters; - return counters; - } }; //------------------------------------------------------------------------------ diff --git a/src/ripple/nodestore/backend/NuDBFactory.cpp b/src/ripple/nodestore/backend/NuDBFactory.cpp index c5252af45..5a2a23d47 100644 --- a/src/ripple/nodestore/backend/NuDBFactory.cpp +++ b/src/ripple/nodestore/backend/NuDBFactory.cpp @@ -328,13 +328,6 @@ public: { return 3; } - - Counters const& - counters() const override - { - static Counters counters; - return counters; - } }; //------------------------------------------------------------------------------ diff --git a/src/ripple/nodestore/backend/NullFactory.cpp b/src/ripple/nodestore/backend/NullFactory.cpp index 8e1b43c69..453d290d3 100644 --- a/src/ripple/nodestore/backend/NullFactory.cpp +++ b/src/ripple/nodestore/backend/NullFactory.cpp @@ -115,13 +115,6 @@ public: return 0; } - Counters const& - counters() const override - { - static Counters counters; - return counters; - } - private: }; diff --git a/src/ripple/nodestore/backend/RocksDBFactory.cpp b/src/ripple/nodestore/backend/RocksDBFactory.cpp index 1a69ab9bd..bf3f5e6a9 100644 --- a/src/ripple/nodestore/backend/RocksDBFactory.cpp +++ b/src/ripple/nodestore/backend/RocksDBFactory.cpp @@ -436,13 +436,6 @@ public: { return fdRequired_; } - - Counters const& - counters() const override - { - static Counters counters; - return counters; - } }; //------------------------------------------------------------------------------ diff --git a/src/ripple/nodestore/impl/Database.cpp b/src/ripple/nodestore/impl/Database.cpp index 579443ace..8c08b6cf2 100644 --- a/src/ripple/nodestore/impl/Database.cpp +++ b/src/ripple/nodestore/impl/Database.cpp @@ -325,12 +325,15 @@ Database::getCountsJson(Json::Value& obj) obj[jss::node_written_bytes] = std::to_string(storeSz_); obj[jss::node_read_bytes] = std::to_string(fetchSz_); obj[jss::node_reads_duration_us] = std::to_string(fetchDurationUs_); - auto const& c = getBackend().counters(); - obj[jss::node_read_errors] = std::to_string(c.readErrors); - obj[jss::node_read_retries] = std::to_string(c.readRetries); - obj[jss::node_write_retries] = std::to_string(c.writeRetries); - obj[jss::node_writes_delayed] = std::to_string(c.writesDelayed); - obj[jss::node_writes_duration_us] = std::to_string(c.writeDurationUs); + + if (auto c = getCounters()) + { + obj[jss::node_read_errors] = std::to_string(c->readErrors); + obj[jss::node_read_retries] = std::to_string(c->readRetries); + obj[jss::node_write_retries] = std::to_string(c->writeRetries); + obj[jss::node_writes_delayed] = std::to_string(c->writesDelayed); + obj[jss::node_writes_duration_us] = std::to_string(c->writeDurationUs); + } } } // namespace NodeStore diff --git a/src/ripple/nodestore/impl/DatabaseNodeImp.h b/src/ripple/nodestore/impl/DatabaseNodeImp.h index 547e89488..3c6309e39 100644 --- a/src/ripple/nodestore/impl/DatabaseNodeImp.h +++ b/src/ripple/nodestore/impl/DatabaseNodeImp.h @@ -20,6 +20,7 @@ #ifndef RIPPLE_NODESTORE_DATABASENODEIMP_H_INCLUDED #define RIPPLE_NODESTORE_DATABASENODEIMP_H_INCLUDED +#include #include #include @@ -133,12 +134,6 @@ public: void sweep() override; - Backend& - getBackend() override - { - return *backend_; - }; - private: // Cache for database objects. This cache is not always initialized. Check // for null before using. @@ -157,6 +152,12 @@ private: { backend_->for_each(f); } + + std::optional> + getCounters() const override + { + return backend_->counters(); + } }; } // namespace NodeStore diff --git a/src/ripple/nodestore/impl/DatabaseRotatingImp.cpp b/src/ripple/nodestore/impl/DatabaseRotatingImp.cpp index 31df09f2b..034efb0e6 100644 --- a/src/ripple/nodestore/impl/DatabaseRotatingImp.cpp +++ b/src/ripple/nodestore/impl/DatabaseRotatingImp.cpp @@ -93,6 +93,13 @@ DatabaseRotatingImp::storeLedger(std::shared_ptr const& srcLedger) return Database::storeLedger(*srcLedger, backend); } +void +DatabaseRotatingImp::sync() +{ + std::lock_guard lock(mutex_); + writableBackend_->sync(); +} + void DatabaseRotatingImp::store( NodeObjectType type, diff --git a/src/ripple/nodestore/impl/DatabaseRotatingImp.h b/src/ripple/nodestore/impl/DatabaseRotatingImp.h index f066990af..304eb6a58 100644 --- a/src/ripple/nodestore/impl/DatabaseRotatingImp.h +++ b/src/ripple/nodestore/impl/DatabaseRotatingImp.h @@ -74,10 +74,7 @@ public: override; void - sync() override - { - writableBackend_->sync(); - } + sync() override; bool storeLedger(std::shared_ptr const& srcLedger) override; @@ -85,12 +82,6 @@ public: void sweep() override; - Backend& - getBackend() override - { - return *writableBackend_; - } - private: std::shared_ptr writableBackend_; std::shared_ptr archiveBackend_; @@ -102,13 +93,6 @@ private: std::shared_ptr const& archiveBackend; }; - Backends - getBackends() const - { - std::lock_guard lock(mutex_); - return Backends{writableBackend_, archiveBackend_}; - } - std::shared_ptr fetchNodeObject( uint256 const& hash, diff --git a/src/ripple/nodestore/impl/DatabaseShardImp.cpp b/src/ripple/nodestore/impl/DatabaseShardImp.cpp index 838e61665..211248aab 100644 --- a/src/ripple/nodestore/impl/DatabaseShardImp.cpp +++ b/src/ripple/nodestore/impl/DatabaseShardImp.cpp @@ -540,12 +540,6 @@ DatabaseShardImp::importShard( return true; } -Backend& -DatabaseShardImp::getBackend() -{ - return app_.getNodeStore().getBackend(); -} - std::shared_ptr DatabaseShardImp::fetchLedger(uint256 const& hash, std::uint32_t ledgerSeq) { diff --git a/src/ripple/nodestore/impl/DatabaseShardImp.h b/src/ripple/nodestore/impl/DatabaseShardImp.h index 4bb0ae202..a3fa60948 100644 --- a/src/ripple/nodestore/impl/DatabaseShardImp.h +++ b/src/ripple/nodestore/impl/DatabaseShardImp.h @@ -161,9 +161,6 @@ public: void sweep() override; - Backend& - getBackend() override; - private: enum class PathDesignation : uint8_t { none, // No path specified