From fe89a39adf76bd4853e302014d970fcd1f5f0f13 Mon Sep 17 00:00:00 2001 From: Radoslaw Zarzynski Date: Wed, 2 Dec 2020 10:26:29 +0100 Subject: [PATCH] crimson: use explicit, abstract interfaces for backfill's facades. Signed-off-by: Radoslaw Zarzynski --- src/crimson/osd/backfill_facades.h | 37 +++++++++++----------- src/crimson/osd/backfill_state.cc | 5 --- src/crimson/osd/backfill_state.h | 31 +++++++++++++++++++ src/crimson/osd/pg.h | 2 +- src/crimson/osd/pg_recovery.cc | 4 +-- src/test/crimson/CMakeLists.txt | 1 - src/test/crimson/test_backfill.cc | 25 +++++++-------- src/test/crimson/test_backfill_facades.h | 39 ------------------------ 8 files changed, 65 insertions(+), 79 deletions(-) delete mode 100644 src/test/crimson/test_backfill_facades.h diff --git a/src/crimson/osd/backfill_facades.h b/src/crimson/osd/backfill_facades.h index 21b18a8b3b0..683dc6ea649 100644 --- a/src/crimson/osd/backfill_facades.h +++ b/src/crimson/osd/backfill_facades.h @@ -9,47 +9,46 @@ namespace crimson::osd { -// PeeringFacade -- a facade (in the GoF-defined meaning) simplifying -// the interface of PeeringState. The motivation is to have an inventory -// of behaviour that must be provided by a unit test's mock. -struct BackfillState::PeeringFacade { +// PeeringFacade -- main implementation of the BackfillState::PeeringFacade +// interface. We have the abstraction to decuple BackfillState from Peering +// State, and thus cut depedencies in unit testing. The second implemention +// is BackfillFixture::PeeringFacade and sits in test_backfill.cc. +struct PeeringFacade final : BackfillState::PeeringFacade { PeeringState& peering_state; - decltype(auto) earliest_backfill() const { + hobject_t earliest_backfill() const override { return peering_state.earliest_backfill(); } - decltype(auto) get_backfill_targets() const { + const std::set& get_backfill_targets() const override { return peering_state.get_backfill_targets(); } - decltype(auto) get_peer_last_backfill(pg_shard_t peer) const { + const hobject_t& get_peer_last_backfill(pg_shard_t peer) const override { return peering_state.get_peer_info(peer).last_backfill; } - decltype(auto) get_last_update() const { + const eversion_t& get_last_update() const override { return peering_state.get_info().last_update; } - decltype(auto) get_log_tail() const { + const eversion_t& get_log_tail() const override { return peering_state.get_info().log_tail; } - template - void scan_log_after(Args&&... args) const { - peering_state.get_pg_log().get_log().scan_log_after( - std::forward(args)...); + void scan_log_after(eversion_t v, scan_log_func_t f) const override { + peering_state.get_pg_log().get_log().scan_log_after(v, std::move(f)); } - bool is_backfill_target(pg_shard_t peer) const { + bool is_backfill_target(pg_shard_t peer) const override { return peering_state.is_backfill_target(peer); } void update_complete_backfill_object_stats(const hobject_t &hoid, - const pg_stat_t &stats) { - return peering_state.update_complete_backfill_object_stats(hoid, stats); + const pg_stat_t &stats) override { + peering_state.update_complete_backfill_object_stats(hoid, stats); } - bool is_backfilling() const { + bool is_backfilling() const override { return peering_state.is_backfilling(); } @@ -61,10 +60,10 @@ struct BackfillState::PeeringFacade { // PGFacade -- a facade (in the GoF-defined meaning) simplifying the huge // interface of crimson's PG class. The motivation is to have an inventory // of behaviour that must be provided by a unit test's mock. -struct BackfillState::PGFacade { +struct PGFacade final : BackfillState::PGFacade { PG& pg; - decltype(auto) get_projected_last_update() const { + const eversion_t& get_projected_last_update() const override { return pg.projected_last_update; } diff --git a/src/crimson/osd/backfill_state.cc b/src/crimson/osd/backfill_state.cc index 4a8b11f5976..fe4cb328821 100644 --- a/src/crimson/osd/backfill_state.cc +++ b/src/crimson/osd/backfill_state.cc @@ -5,11 +5,6 @@ #include #include "crimson/osd/backfill_state.h" -#ifndef BACKFILL_UNITTEST -#include "crimson/osd/backfill_facades.h" -#else -#include "test/crimson/test_backfill_facades.h" -#endif namespace { seastar::logger& logger() { diff --git a/src/crimson/osd/backfill_state.h b/src/crimson/osd/backfill_state.h index e742fd10a9a..00b111dc381 100644 --- a/src/crimson/osd/backfill_state.h +++ b/src/crimson/osd/backfill_state.h @@ -308,6 +308,37 @@ struct BackfillState::BackfillListener { virtual ~BackfillListener() = default; }; +// PeeringFacade -- a facade (in the GoF-defined meaning) simplifying +// the interface of PeeringState. The motivation is to have an inventory +// of behaviour that must be provided by a unit test's mock. +struct BackfillState::PeeringFacade { + virtual hobject_t earliest_backfill() const = 0; + virtual const std::set& get_backfill_targets() const = 0; + virtual const hobject_t& get_peer_last_backfill(pg_shard_t peer) const = 0; + virtual const eversion_t& get_last_update() const = 0; + virtual const eversion_t& get_log_tail() const = 0; + + // the performance impact of `std::function` has not been considered yet. + // If there is any proof (from e.g. profiling) about its significance, we + // can switch back to the template variant. + using scan_log_func_t = std::function; + virtual void scan_log_after(eversion_t, scan_log_func_t) const = 0; + + virtual bool is_backfill_target(pg_shard_t peer) const = 0; + virtual void update_complete_backfill_object_stats(const hobject_t &hoid, + const pg_stat_t &stats) = 0; + virtual bool is_backfilling() const = 0; + virtual ~PeeringFacade() {} +}; + +// PGFacade -- a facade (in the GoF-defined meaning) simplifying the huge +// interface of crimson's PG class. The motivation is to have an inventory +// of behaviour that must be provided by a unit test's mock. +struct BackfillState::PGFacade { + virtual const eversion_t& get_projected_last_update() const = 0; + virtual ~PGFacade() {} +}; + class BackfillState::ProgressTracker { // TODO: apply_stat, enum class op_stage_t { diff --git a/src/crimson/osd/pg.h b/src/crimson/osd/pg.h index f7db6ba6cda..35b5f01a946 100644 --- a/src/crimson/osd/pg.h +++ b/src/crimson/osd/pg.h @@ -670,7 +670,7 @@ private: friend class PeeringEvent; friend class RepRequest; friend class BackfillRecovery; - friend struct BackfillState::PGFacade; + friend struct PGFacade; private: seastar::future find_unfound() { return seastar::make_ready_future(true); diff --git a/src/crimson/osd/pg_recovery.cc b/src/crimson/osd/pg_recovery.cc index cdf26274533..390449728cc 100644 --- a/src/crimson/osd/pg_recovery.cc +++ b/src/crimson/osd/pg_recovery.cc @@ -523,8 +523,8 @@ void PGRecovery::on_backfill_reserved() using BackfillState = crimson::osd::BackfillState; backfill_state = std::make_unique( *this, - std::make_unique(pg->get_peering_state()), - std::make_unique( + std::make_unique(pg->get_peering_state()), + std::make_unique( *static_cast(pg))); // yes, it's **not** backfilling yet. The PG_STATE_BACKFILLING // will be set after on_backfill_reserved() returns. diff --git a/src/test/crimson/CMakeLists.txt b/src/test/crimson/CMakeLists.txt index 2882cc6e541..3dedce103f6 100644 --- a/src/test/crimson/CMakeLists.txt +++ b/src/test/crimson/CMakeLists.txt @@ -5,7 +5,6 @@ add_executable(unittest-crimson-backfill ${PROJECT_SOURCE_DIR}/src/crimson/osd/backfill_state.cc ${PROJECT_SOURCE_DIR}/src/osd/recovery_types.cc) add_ceph_unittest(unittest-crimson-backfill) -target_compile_definitions(unittest-crimson-backfill PRIVATE -DBACKFILL_UNITTEST) target_link_libraries(unittest-crimson-backfill crimson GTest::Main) add_executable(unittest-seastar-buffer diff --git a/src/test/crimson/test_backfill.cc b/src/test/crimson/test_backfill.cc index 01f435fd416..14309f96606 100644 --- a/src/test/crimson/test_backfill.cc +++ b/src/test/crimson/test_backfill.cc @@ -17,7 +17,6 @@ #include "common/hobject.h" #include "crimson/osd/backfill_state.h" #include "osd/recovery_types.h" -#include "test/crimson/test_backfill_facades.h" // The sole purpose is to convert from the string representation. @@ -172,11 +171,19 @@ struct BackfillFixture::PeeringFacade : public crimson::osd::BackfillState::PeeringFacade { FakePrimary& backfill_source; std::map& backfill_targets; + // sorry, this is duplicative but that's the interface + std::set backfill_targets_as_set; PeeringFacade(FakePrimary& backfill_source, std::map& backfill_targets) : backfill_source(backfill_source), backfill_targets(backfill_targets) { + std::transform( + std::begin(backfill_targets), std::end(backfill_targets), + std::inserter(backfill_targets_as_set, std::end(backfill_targets_as_set)), + [](auto pair) { + return pair.first; + }); } hobject_t earliest_backfill() const override { @@ -186,15 +193,8 @@ struct BackfillFixture::PeeringFacade } return e; } - std::set get_backfill_targets() const override { - std::set result; - std::transform( - std::begin(backfill_targets), std::end(backfill_targets), - std::inserter(result, std::end(result)), - [](auto pair) { - return pair.first; - }); - return result; + const std::set& get_backfill_targets() const override { + return backfill_targets_as_set; } const hobject_t& get_peer_last_backfill(pg_shard_t peer) const override { return backfill_targets.at(peer).last_backfill; @@ -205,8 +205,9 @@ struct BackfillFixture::PeeringFacade const eversion_t& get_log_tail() const override { return backfill_source.log_tail; } - template - void scan_log_after(Args&&... args) const { + + void scan_log_after(eversion_t, scan_log_func_t) const override { + /* NOP */ } bool is_backfill_target(pg_shard_t peer) const override { diff --git a/src/test/crimson/test_backfill_facades.h b/src/test/crimson/test_backfill_facades.h deleted file mode 100644 index d54d14ca5b4..00000000000 --- a/src/test/crimson/test_backfill_facades.h +++ /dev/null @@ -1,39 +0,0 @@ -// -*- mode:C++; tab-width:8; c-basic-offset:2; indent-tabs-mode:t -*- -// vim: ts=8 sw=2 smarttab - -#pragma once - -#include "crimson/osd/backfill_state.h" - -namespace crimson::osd { - -// PeeringFacade -- a facade (in the GoF-defined meaning) simplifying -// the interface of PeeringState. The motivation is to have an inventory -// of behaviour that must be provided by a unit test's mock. -struct BackfillState::PeeringFacade { - virtual hobject_t earliest_backfill() const = 0; - virtual std::set get_backfill_targets() const = 0; - virtual const hobject_t& get_peer_last_backfill(pg_shard_t peer) const = 0; - virtual const eversion_t& get_last_update() const = 0; - virtual const eversion_t& get_log_tail() const = 0; - - template - void scan_log_after(Args&&... args) const { - } - - virtual bool is_backfill_target(pg_shard_t peer) const = 0; - virtual void update_complete_backfill_object_stats(const hobject_t &hoid, - const pg_stat_t &stats) = 0; - virtual bool is_backfilling() const = 0; - virtual ~PeeringFacade() {} -}; - -// PGFacade -- a facade (in the GoF-defined meaning) simplifying the huge -// interface of crimson's PG class. The motivation is to have an inventory -// of behaviour that must be provided by a unit test's mock. -struct BackfillState::PGFacade { - virtual const eversion_t& get_projected_last_update() const = 0; - virtual ~PGFacade() {} -}; - -} // namespace crimson::osd -- 2.39.5