From 1d21b938ab838d8e0138d19dd4bd0f60f374eaf0 Mon Sep 17 00:00:00 2001 From: Henning Baldersheim Date: Tue, 22 Feb 2022 22:19:00 +0100 Subject: Revert "Revert "Add support for creating ConfigContext with externally provided FNET_…"" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../tests/file_subscription/file_subscription.cpp | 2 +- .../tests/frtconnectionpool/frtconnectionpool.cpp | 37 ++++++++++++++++------ config/src/tests/subscriber/subscriber.cpp | 2 +- config/src/vespa/config/common/cancelhandler.h | 2 +- config/src/vespa/config/common/configcontext.cpp | 4 +-- config/src/vespa/config/common/configmanager.cpp | 6 ++-- config/src/vespa/config/common/configmanager.h | 12 +++---- config/src/vespa/config/common/reloadhandler.h | 2 +- config/src/vespa/config/common/sourcefactory.h | 1 - config/src/vespa/config/frt/connectionfactory.h | 4 +-- config/src/vespa/config/frt/frtconnectionpool.cpp | 27 ++++++++++------ config/src/vespa/config/frt/frtconnectionpool.h | 18 ++++++++--- config/src/vespa/config/frt/frtsourcefactory.cpp | 6 +++- config/src/vespa/config/frt/frtsourcefactory.h | 12 ++++--- .../config/subscription/configsubscriptionset.cpp | 2 +- .../src/vespa/config/subscription/sourcespec.cpp | 20 ++++++++---- config/src/vespa/config/subscription/sourcespec.h | 13 ++------ 17 files changed, 105 insertions(+), 65 deletions(-) (limited to 'config') diff --git a/config/src/tests/file_subscription/file_subscription.cpp b/config/src/tests/file_subscription/file_subscription.cpp index b4a2b890bb5..6b900876bda 100644 --- a/config/src/tests/file_subscription/file_subscription.cpp +++ b/config/src/tests/file_subscription/file_subscription.cpp @@ -58,7 +58,7 @@ TEST("requireThatFileSpecGivesCorrectSource") { writeFile("my.cfg", "foobar"); FileSpec spec("my.cfg"); - SourceFactory::UP factory(spec.createSourceFactory(TimingValues())); + auto factory = spec.createSourceFactory(TimingValues()); ASSERT_TRUE(factory); auto holder = std::make_shared(); std::unique_ptr src = factory->createSource(holder, ConfigKey("my", "my", "bar", "foo")); diff --git a/config/src/tests/frtconnectionpool/frtconnectionpool.cpp b/config/src/tests/frtconnectionpool/frtconnectionpool.cpp index c2ed60370e0..46e1f698091 100644 --- a/config/src/tests/frtconnectionpool/frtconnectionpool.cpp +++ b/config/src/tests/frtconnectionpool/frtconnectionpool.cpp @@ -3,6 +3,9 @@ #include #include #include +#include +#include +#include #include #include #include @@ -12,8 +15,12 @@ using namespace config; class Test : public vespalib::TestApp { private: static ServerSpec::HostSpecList _sources; + FastOS_ThreadPool _threadPool; + FNET_Transport _transport; void verifyAllSourcesInRotation(FRTConnectionPool& sourcePool); public: + Test(); + ~Test() override; int Main() override; void testBasicRoundRobin(); void testBasicHashBasedSelection(); @@ -25,6 +32,18 @@ public: void testManySources(); }; +Test::Test() + : vespalib::TestApp(), + _threadPool(64_Ki), + _transport() +{ + _transport.Start(&_threadPool); +} + +Test::~Test() { + _transport.ShutDown(true); +} + TEST_APPHOOK(Test); ServerSpec::HostSpecList Test::_sources; @@ -79,7 +98,7 @@ void Test::verifyAllSourcesInRotation(FRTConnectionPool& sourcePool) { */ void Test::testBasicRoundRobin() { const ServerSpec spec(_sources); - FRTConnectionPool sourcePool(spec, timingValues); + FRTConnectionPool sourcePool(_transport, spec, timingValues); for (int i = 0; i < 9; i++) { int j = i % _sources.size(); std::stringstream s; @@ -93,7 +112,7 @@ void Test::testBasicRoundRobin() { */ void Test::testBasicHashBasedSelection() { const ServerSpec spec(_sources); - FRTConnectionPool sourcePool(spec, timingValues); + FRTConnectionPool sourcePool(_transport, spec, timingValues); sourcePool.setHostname("a.b.com"); for (int i = 0; i < 9; i++) { EXPECT_EQUAL("host1", sourcePool.getNextHashBased()->getAddress()); @@ -108,7 +127,7 @@ void Test::testBasicHashBasedSelection() { hostnames.push_back("stroustrup-02.example.yahoo.com"); hostnames.push_back("alexandrescu-03.example.yahoo.com"); const ServerSpec spec2(hostnames); - FRTConnectionPool sourcePool2(spec2, timingValues); + FRTConnectionPool sourcePool2(_transport, spec2, timingValues); sourcePool2.setHostname("sutter-01.example.yahoo.com"); EXPECT_EQUAL("stroustrup-02.example.yahoo.com", sourcePool2.getNextHashBased()->getAddress()); sourcePool2.setHostname("stroustrup-02.example.yahoo.com"); @@ -123,7 +142,7 @@ void Test::testBasicHashBasedSelection() { */ void Test::testSetErrorRoundRobin() { const ServerSpec spec(_sources); - FRTConnectionPool sourcePool(spec, timingValues); + FRTConnectionPool sourcePool(_transport, spec, timingValues); FRTConnection* source = sourcePool.getNextRoundRobin(); source->setError(FRTE_RPC_CONNECTION); for (int i = 0; i < 9; i++) { @@ -138,7 +157,7 @@ void Test::testSetErrorRoundRobin() { */ void Test::testSetErrorAllRoundRobin() { const ServerSpec spec(_sources); - FRTConnectionPool sourcePool(spec, timingValues); + FRTConnectionPool sourcePool(_transport, spec, timingValues); for (int i = 0; i < (int)_sources.size(); i++) { FRTConnection* source = sourcePool.getNextRoundRobin(); source->setError(FRTE_RPC_CONNECTION); @@ -152,7 +171,7 @@ void Test::testSetErrorAllRoundRobin() { */ void Test::testSetErrorHashBased() { const ServerSpec spec(_sources); - FRTConnectionPool sourcePool(spec, timingValues); + FRTConnectionPool sourcePool(_transport, spec, timingValues); FRTConnection* source = sourcePool.getNextHashBased(); source->setError(FRTE_RPC_CONNECTION); for (int i = 0; i < (int)_sources.size(); i++) { @@ -167,7 +186,7 @@ void Test::testSetErrorHashBased() { */ void Test::testSetErrorAllHashBased() { const ServerSpec spec(_sources); - FRTConnectionPool sourcePool(spec, timingValues); + FRTConnectionPool sourcePool(_transport, spec, timingValues); FRTConnection* firstSource = sourcePool.getNextHashBased(); auto readySources = sourcePool.getReadySources(); for (int i = 0; i < (int)readySources.size(); i++) { @@ -199,7 +218,7 @@ void Test::testSetErrorAllHashBased() { */ void Test::testSuspensionTimeout() { const ServerSpec spec(_sources); - FRTConnectionPool sourcePool(spec, timingValues); + FRTConnectionPool sourcePool(_transport, spec, timingValues); Connection* source = sourcePool.getCurrent(); source->setTransientDelay(1s); source->setError(FRTE_RPC_CONNECTION); @@ -227,7 +246,7 @@ void Test::testManySources() { twoSources.push_back("host1"); const ServerSpec spec(twoSources); - FRTConnectionPool sourcePool(spec, timingValues); + FRTConnectionPool sourcePool(_transport, spec, timingValues); for (int i = 0; i < (int)hostnames.size(); i++) { sourcePool.setHostname(hostnames[i]); diff --git a/config/src/tests/subscriber/subscriber.cpp b/config/src/tests/subscriber/subscriber.cpp index 548e54e1d18..d7320bb6a2f 100644 --- a/config/src/tests/subscriber/subscriber.cpp +++ b/config/src/tests/subscriber/subscriber.cpp @@ -85,7 +85,7 @@ namespace { return std::make_shared(0, key, holder, std::make_unique()); } - void unsubscribe(const ConfigSubscription::SP & subscription) override { + void unsubscribe(const ConfigSubscription & subscription) override { (void) subscription; numCancel++; } diff --git a/config/src/vespa/config/common/cancelhandler.h b/config/src/vespa/config/common/cancelhandler.h index 8dde83ba7d9..694f0185e7c 100644 --- a/config/src/vespa/config/common/cancelhandler.h +++ b/config/src/vespa/config/common/cancelhandler.h @@ -13,7 +13,7 @@ struct CancelHandler * * @param subscription ConfigSubscription to cancel */ - virtual void unsubscribe(const std::shared_ptr & subscription) = 0; + virtual void unsubscribe(const ConfigSubscription & subscription) = 0; virtual ~CancelHandler() = default; }; diff --git a/config/src/vespa/config/common/configcontext.cpp b/config/src/vespa/config/common/configcontext.cpp index 2c8c981862d..eb56202bf06 100644 --- a/config/src/vespa/config/common/configcontext.cpp +++ b/config/src/vespa/config/common/configcontext.cpp @@ -6,9 +6,7 @@ namespace config { ConfigContext::ConfigContext(const SourceSpec & spec) - : _timingValues(), - _generation(1), - _manager(std::make_unique(spec.createSourceFactory(_timingValues), _generation)) + : ConfigContext(TimingValues(), spec) { } ConfigContext::ConfigContext(const TimingValues & timingValues, const SourceSpec & spec) diff --git a/config/src/vespa/config/common/configmanager.cpp b/config/src/vespa/config/common/configmanager.cpp index 983cccb0b0b..1417aed79ae 100644 --- a/config/src/vespa/config/common/configmanager.cpp +++ b/config/src/vespa/config/common/configmanager.cpp @@ -11,7 +11,7 @@ LOG_SETUP(".config.common.configmanager"); namespace config { -ConfigManager::ConfigManager(SourceFactory::UP sourceFactory, int64_t initialGeneration) +ConfigManager::ConfigManager(std::unique_ptr sourceFactory, int64_t initialGeneration) : _idGenerator(0), _sourceFactory(std::move(sourceFactory)), _generation(initialGeneration), @@ -53,10 +53,10 @@ ConfigManager::subscribe(const ConfigKey & key, vespalib::duration timeout) } void -ConfigManager::unsubscribe(const ConfigSubscription::SP & subscription) +ConfigManager::unsubscribe(const ConfigSubscription & subscription) { std::lock_guard guard(_lock); - const SubscriptionId id(subscription->getSubscriptionId()); + const SubscriptionId id(subscription.getSubscriptionId()); if (_subscriptionMap.find(id) != _subscriptionMap.end()) _subscriptionMap.erase(id); } diff --git a/config/src/vespa/config/common/configmanager.h b/config/src/vespa/config/common/configmanager.h index 09d263d9c25..a622bce469e 100644 --- a/config/src/vespa/config/common/configmanager.h +++ b/config/src/vespa/config/common/configmanager.h @@ -21,19 +21,19 @@ struct TimingValues; class ConfigManager : public IConfigManager { public: - ConfigManager(SourceFactory::UP sourceFactory, int64_t initialGeneration); + ConfigManager(std::unique_ptr sourceFactory, int64_t initialGeneration); ~ConfigManager() override; ConfigSubscription::SP subscribe(const ConfigKey & key, vespalib::duration timeout) override; - void unsubscribe(const ConfigSubscription::SP & subscription) override; + void unsubscribe(const ConfigSubscription & subscription) override; void reload(int64_t generation) override; private: - std::atomic _idGenerator; - SourceFactory::UP _sourceFactory; - int64_t _generation; + std::atomic _idGenerator; + std::unique_ptr _sourceFactory; + int64_t _generation; - typedef std::map SubscriptionMap; + using SubscriptionMap = std::map; SubscriptionMap _subscriptionMap; std::mutex _lock; }; diff --git a/config/src/vespa/config/common/reloadhandler.h b/config/src/vespa/config/common/reloadhandler.h index 73016a2f883..cd329e8d525 100644 --- a/config/src/vespa/config/common/reloadhandler.h +++ b/config/src/vespa/config/common/reloadhandler.h @@ -9,7 +9,7 @@ struct ReloadHandler * Reload any configs with a given generation. */ virtual void reload(int64_t generation) = 0; - virtual ~ReloadHandler() { } + virtual ~ReloadHandler() = default; }; } diff --git a/config/src/vespa/config/common/sourcefactory.h b/config/src/vespa/config/common/sourcefactory.h index 0236bea2802..f11a070fd95 100644 --- a/config/src/vespa/config/common/sourcefactory.h +++ b/config/src/vespa/config/common/sourcefactory.h @@ -13,7 +13,6 @@ class IConfigHolder; */ class SourceFactory { public: - typedef std::unique_ptr UP; virtual std::unique_ptr createSource(std::shared_ptr holder, const ConfigKey & key) const = 0; virtual ~SourceFactory() = default; }; diff --git a/config/src/vespa/config/frt/connectionfactory.h b/config/src/vespa/config/frt/connectionfactory.h index dc1eadf3cf6..eb7ca10fa0d 100644 --- a/config/src/vespa/config/frt/connectionfactory.h +++ b/config/src/vespa/config/frt/connectionfactory.h @@ -12,12 +12,10 @@ class Connection; class ConnectionFactory { public: - typedef std::unique_ptr UP; - typedef std::shared_ptr SP; virtual Connection * getCurrent() = 0; virtual void syncTransport() = 0; virtual FNET_Scheduler * getScheduler() = 0; - virtual ~ConnectionFactory() { } + virtual ~ConnectionFactory() = default; }; } diff --git a/config/src/vespa/config/frt/frtconnectionpool.cpp b/config/src/vespa/config/frt/frtconnectionpool.cpp index 629baf67cf8..15ba8472af0 100644 --- a/config/src/vespa/config/frt/frtconnectionpool.cpp +++ b/config/src/vespa/config/frt/frtconnectionpool.cpp @@ -27,10 +27,8 @@ FRTConnectionPool::FRTConnectionKey::operator==(const FRTConnectionKey& right) c return _hostname == right._hostname; } -FRTConnectionPool::FRTConnectionPool(const ServerSpec & spec, const TimingValues & timingValues) - : _threadPool(std::make_unique(60_Ki)), - _transport(std::make_unique()), - _supervisor(std::make_unique(_transport.get())), +FRTConnectionPool::FRTConnectionPool(FNET_Transport & transport, const ServerSpec & spec, const TimingValues & timingValues) + : _supervisor(std::make_unique(& transport)), _selectIdx(0), _hostname("") { @@ -39,13 +37,9 @@ FRTConnectionPool::FRTConnectionPool(const ServerSpec & spec, const TimingValues _connections[key] = std::make_shared(spec.getHost(i), *_supervisor, timingValues); } setHostname(); - _transport->Start(_threadPool.get()); } -FRTConnectionPool::~FRTConnectionPool() -{ - _transport->ShutDown(true); -} +FRTConnectionPool::~FRTConnectionPool() = default; void FRTConnectionPool::syncTransport() @@ -150,4 +144,19 @@ FRTConnectionPool::getScheduler() { return _supervisor->GetScheduler(); } +FRTConnectionPoolWithTransport::FRTConnectionPoolWithTransport(std::unique_ptr threadPool, + std::unique_ptr transport, + const ServerSpec & spec, const TimingValues & timingValues) + : FRTConnectionPool(*transport, spec, timingValues), + _threadPool(std::move(threadPool)), + _transport(std::move(transport)) +{ + _transport->Start(_threadPool.get()); +} + +FRTConnectionPoolWithTransport::~FRTConnectionPoolWithTransport() +{ + _transport->ShutDown(true); +} + } diff --git a/config/src/vespa/config/frt/frtconnectionpool.h b/config/src/vespa/config/frt/frtconnectionpool.h index 6b387fbf67f..b7530f8536e 100644 --- a/config/src/vespa/config/frt/frtconnectionpool.h +++ b/config/src/vespa/config/frt/frtconnectionpool.h @@ -13,7 +13,6 @@ class FastOS_ThreadPool; namespace config { class FRTConnectionPool : public ConnectionFactory { - private: /** @@ -32,8 +31,6 @@ private: int operator==(const FRTConnectionKey& right) const; }; - std::unique_ptr _threadPool; - std::unique_ptr _transport; std::unique_ptr _supervisor; int _selectIdx; vespalib::string _hostname; @@ -41,7 +38,7 @@ private: ConnectionMap _connections; public: - FRTConnectionPool(const ServerSpec & spec, const TimingValues & timingValues); + FRTConnectionPool(FNET_Transport & transport, const ServerSpec & spec, const TimingValues & timingValues); FRTConnectionPool(const FRTConnectionPool&) = delete; FRTConnectionPool& operator=(const FRTConnectionPool&) = delete; ~FRTConnectionPool() override; @@ -130,5 +127,18 @@ public: static int hashCode(const vespalib::string & s); }; +class FRTConnectionPoolWithTransport : public FRTConnectionPool { +public: + FRTConnectionPoolWithTransport(std::unique_ptr threadPool, + std::unique_ptr transport, + const ServerSpec & spec, const TimingValues & timingValues); + FRTConnectionPoolWithTransport(const FRTConnectionPoolWithTransport&) = delete; + FRTConnectionPoolWithTransport& operator=(const FRTConnectionPoolWithTransport&) = delete; + ~FRTConnectionPoolWithTransport() override; +private: + std::unique_ptr _threadPool; + std::unique_ptr _transport; +}; + } // namespace config diff --git a/config/src/vespa/config/frt/frtsourcefactory.cpp b/config/src/vespa/config/frt/frtsourcefactory.cpp index 9706e3bbd5e..1c740534730 100644 --- a/config/src/vespa/config/frt/frtsourcefactory.cpp +++ b/config/src/vespa/config/frt/frtsourcefactory.cpp @@ -1,17 +1,21 @@ // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. + #include "frtsourcefactory.h" #include "frtsource.h" #include "frtconfigagent.h" +#include "connectionfactory.h" namespace config { -FRTSourceFactory::FRTSourceFactory(ConnectionFactory::UP connectionFactory, const TimingValues & timingValues, int traceLevel, const VespaVersion & vespaVersion, const CompressionType & compressionType) +FRTSourceFactory::FRTSourceFactory(std::unique_ptr connectionFactory, const TimingValues & timingValues, int traceLevel, const VespaVersion & vespaVersion, const CompressionType & compressionType) : _connectionFactory(std::move(connectionFactory)), _requestFactory(traceLevel, vespaVersion, compressionType), _timingValues(timingValues) { } +FRTSourceFactory::~FRTSourceFactory() = default; + std::unique_ptr FRTSourceFactory::createSource(std::shared_ptr holder, const ConfigKey & key) const { diff --git a/config/src/vespa/config/frt/frtsourcefactory.h b/config/src/vespa/config/frt/frtsourcefactory.h index 4331c6411dc..19cf6241481 100644 --- a/config/src/vespa/config/frt/frtsourcefactory.h +++ b/config/src/vespa/config/frt/frtsourcefactory.h @@ -1,28 +1,32 @@ // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. #pragma once -#include "connectionfactory.h" #include "frtconfigrequestfactory.h" #include #include namespace config { +class ConnectionFactory; + /** * Class for sending and receiving config requests via FRT. */ class FRTSourceFactory : public SourceFactory { public: - FRTSourceFactory(ConnectionFactory::UP connectionFactory, const TimingValues & timingValues, int traceLevel, const VespaVersion & vespaVersion, const CompressionType & compressionType); - + FRTSourceFactory(const FRTSourceFactory &) = delete; + FRTSourceFactory & operator =(const FRTSourceFactory &) = delete; + FRTSourceFactory(std::unique_ptr connectionFactory, const TimingValues & timingValues, + int traceLevel, const VespaVersion & vespaVersion, const CompressionType & compressionType); + ~FRTSourceFactory() override; /** * Create source handling config described by key. */ std::unique_ptr createSource(std::shared_ptr holder, const ConfigKey & key) const override; private: - ConnectionFactory::SP _connectionFactory; + std::shared_ptr _connectionFactory; FRTConfigRequestFactory _requestFactory; const TimingValues _timingValues; }; diff --git a/config/src/vespa/config/subscription/configsubscriptionset.cpp b/config/src/vespa/config/subscription/configsubscriptionset.cpp index 0b3944acc4e..40dc40d11ef 100644 --- a/config/src/vespa/config/subscription/configsubscriptionset.cpp +++ b/config/src/vespa/config/subscription/configsubscriptionset.cpp @@ -112,7 +112,7 @@ ConfigSubscriptionSet::close() { _state = CLOSED; for (const auto & subscription : _subscriptionList) { - _mgr.unsubscribe(subscription); + _mgr.unsubscribe(*subscription); subscription->close(); } } diff --git a/config/src/vespa/config/subscription/sourcespec.cpp b/config/src/vespa/config/subscription/sourcespec.cpp index 6af981807de..ec70f921179 100644 --- a/config/src/vespa/config/subscription/sourcespec.cpp +++ b/config/src/vespa/config/subscription/sourcespec.cpp @@ -11,6 +11,9 @@ #include #include #include +#include +#include +#include #include namespace config { @@ -25,7 +28,7 @@ RawSpec::RawSpec(const vespalib::string & config) { } -SourceFactory::UP +std::unique_ptr RawSpec::createSourceFactory(const TimingValues &) const { return std::make_unique(_config); @@ -49,7 +52,7 @@ FileSpec::verifyName(const vespalib::string & fileName) } } -SourceFactory::UP +std::unique_ptr FileSpec::createSourceFactory(const TimingValues & ) const { return std::make_unique(*this); @@ -60,7 +63,7 @@ DirSpec::DirSpec(const vespalib::string & dirName) { } -SourceFactory::UP +std::unique_ptr DirSpec::createSourceFactory(const TimingValues & ) const { return std::make_unique(*this); @@ -117,12 +120,15 @@ ServerSpec::ServerSpec(const vespalib::string & hostSpec) initialize(hostSpec); } -SourceFactory::UP +std::unique_ptr ServerSpec::createSourceFactory(const TimingValues & timingValues) const { const auto vespaVersion = VespaVersion::getCurrentVersion(); - return std::make_unique(std::make_unique(*this, timingValues), timingValues, - _traceLevel, vespaVersion, _compressionType); + return std::make_unique( + std::make_unique(std::make_unique(64_Ki), + std::make_unique(), + *this, timingValues), + timingValues, _traceLevel, vespaVersion, _compressionType); } @@ -131,7 +137,7 @@ ConfigSet::ConfigSet() { } -SourceFactory::UP +std::unique_ptr ConfigSet::createSourceFactory(const TimingValues & ) const { return std::make_unique(_builderMap); diff --git a/config/src/vespa/config/subscription/sourcespec.h b/config/src/vespa/config/subscription/sourcespec.h index a57b22ca322..3c23233e3ab 100644 --- a/config/src/vespa/config/subscription/sourcespec.h +++ b/config/src/vespa/config/subscription/sourcespec.h @@ -156,13 +156,6 @@ public: SourceFactorySP createSourceFactory(const TimingValues & timingValues) const override; - /** - * Add another host to this source spec, allowing failover. - * - * @param host hostname formatted as tcp/hostname:port - */ - void addHost(const vespalib::string & host) { _hostList.push_back(host); } - /** * Inspect how many hosts this source refers to. * @@ -193,9 +186,9 @@ public: CompressionType compressionType() const { return _compressionType; } private: void initialize(const vespalib::string & hostSpec); - HostSpecList _hostList; - const int _protocolVersion; - const int _traceLevel; + HostSpecList _hostList; + const int _protocolVersion; + const int _traceLevel; const CompressionType _compressionType; const static int DEFAULT_PROXY_PORT = 19090; }; -- cgit v1.2.3