From aa4380331ba01642599c8c6c8ec10c4f5795475a Mon Sep 17 00:00:00 2001 From: Henning Baldersheim Date: Tue, 22 Feb 2022 19:44:30 +0100 Subject: 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, 65 insertions(+), 105 deletions(-) (limited to 'config/src') diff --git a/config/src/tests/file_subscription/file_subscription.cpp b/config/src/tests/file_subscription/file_subscription.cpp index 6b900876bda..b4a2b890bb5 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"); - auto factory = spec.createSourceFactory(TimingValues()); + SourceFactory::UP 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 46e1f698091..c2ed60370e0 100644 --- a/config/src/tests/frtconnectionpool/frtconnectionpool.cpp +++ b/config/src/tests/frtconnectionpool/frtconnectionpool.cpp @@ -3,9 +3,6 @@ #include #include #include -#include -#include -#include #include #include #include @@ -15,12 +12,8 @@ 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(); @@ -32,18 +25,6 @@ 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; @@ -98,7 +79,7 @@ void Test::verifyAllSourcesInRotation(FRTConnectionPool& sourcePool) { */ void Test::testBasicRoundRobin() { const ServerSpec spec(_sources); - FRTConnectionPool sourcePool(_transport, spec, timingValues); + FRTConnectionPool sourcePool(spec, timingValues); for (int i = 0; i < 9; i++) { int j = i % _sources.size(); std::stringstream s; @@ -112,7 +93,7 @@ void Test::testBasicRoundRobin() { */ void Test::testBasicHashBasedSelection() { const ServerSpec spec(_sources); - FRTConnectionPool sourcePool(_transport, spec, timingValues); + FRTConnectionPool sourcePool(spec, timingValues); sourcePool.setHostname("a.b.com"); for (int i = 0; i < 9; i++) { EXPECT_EQUAL("host1", sourcePool.getNextHashBased()->getAddress()); @@ -127,7 +108,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(_transport, spec2, timingValues); + FRTConnectionPool sourcePool2(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"); @@ -142,7 +123,7 @@ void Test::testBasicHashBasedSelection() { */ void Test::testSetErrorRoundRobin() { const ServerSpec spec(_sources); - FRTConnectionPool sourcePool(_transport, spec, timingValues); + FRTConnectionPool sourcePool(spec, timingValues); FRTConnection* source = sourcePool.getNextRoundRobin(); source->setError(FRTE_RPC_CONNECTION); for (int i = 0; i < 9; i++) { @@ -157,7 +138,7 @@ void Test::testSetErrorRoundRobin() { */ void Test::testSetErrorAllRoundRobin() { const ServerSpec spec(_sources); - FRTConnectionPool sourcePool(_transport, spec, timingValues); + FRTConnectionPool sourcePool(spec, timingValues); for (int i = 0; i < (int)_sources.size(); i++) { FRTConnection* source = sourcePool.getNextRoundRobin(); source->setError(FRTE_RPC_CONNECTION); @@ -171,7 +152,7 @@ void Test::testSetErrorAllRoundRobin() { */ void Test::testSetErrorHashBased() { const ServerSpec spec(_sources); - FRTConnectionPool sourcePool(_transport, spec, timingValues); + FRTConnectionPool sourcePool(spec, timingValues); FRTConnection* source = sourcePool.getNextHashBased(); source->setError(FRTE_RPC_CONNECTION); for (int i = 0; i < (int)_sources.size(); i++) { @@ -186,7 +167,7 @@ void Test::testSetErrorHashBased() { */ void Test::testSetErrorAllHashBased() { const ServerSpec spec(_sources); - FRTConnectionPool sourcePool(_transport, spec, timingValues); + FRTConnectionPool sourcePool(spec, timingValues); FRTConnection* firstSource = sourcePool.getNextHashBased(); auto readySources = sourcePool.getReadySources(); for (int i = 0; i < (int)readySources.size(); i++) { @@ -218,7 +199,7 @@ void Test::testSetErrorAllHashBased() { */ void Test::testSuspensionTimeout() { const ServerSpec spec(_sources); - FRTConnectionPool sourcePool(_transport, spec, timingValues); + FRTConnectionPool sourcePool(spec, timingValues); Connection* source = sourcePool.getCurrent(); source->setTransientDelay(1s); source->setError(FRTE_RPC_CONNECTION); @@ -246,7 +227,7 @@ void Test::testManySources() { twoSources.push_back("host1"); const ServerSpec spec(twoSources); - FRTConnectionPool sourcePool(_transport, spec, timingValues); + FRTConnectionPool sourcePool(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 d7320bb6a2f..548e54e1d18 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 & subscription) override { + void unsubscribe(const ConfigSubscription::SP & subscription) override { (void) subscription; numCancel++; } diff --git a/config/src/vespa/config/common/cancelhandler.h b/config/src/vespa/config/common/cancelhandler.h index 694f0185e7c..8dde83ba7d9 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 ConfigSubscription & subscription) = 0; + virtual void unsubscribe(const std::shared_ptr & subscription) = 0; virtual ~CancelHandler() = default; }; diff --git a/config/src/vespa/config/common/configcontext.cpp b/config/src/vespa/config/common/configcontext.cpp index eb56202bf06..2c8c981862d 100644 --- a/config/src/vespa/config/common/configcontext.cpp +++ b/config/src/vespa/config/common/configcontext.cpp @@ -6,7 +6,9 @@ namespace config { ConfigContext::ConfigContext(const SourceSpec & spec) - : ConfigContext(TimingValues(), spec) + : _timingValues(), + _generation(1), + _manager(std::make_unique(spec.createSourceFactory(_timingValues), _generation)) { } 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 1417aed79ae..983cccb0b0b 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(std::unique_ptr sourceFactory, int64_t initialGeneration) +ConfigManager::ConfigManager(SourceFactory::UP 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 & subscription) +ConfigManager::unsubscribe(const ConfigSubscription::SP & 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 a622bce469e..09d263d9c25 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(std::unique_ptr sourceFactory, int64_t initialGeneration); + ConfigManager(SourceFactory::UP sourceFactory, int64_t initialGeneration); ~ConfigManager() override; ConfigSubscription::SP subscribe(const ConfigKey & key, vespalib::duration timeout) override; - void unsubscribe(const ConfigSubscription & subscription) override; + void unsubscribe(const ConfigSubscription::SP & subscription) override; void reload(int64_t generation) override; private: - std::atomic _idGenerator; - std::unique_ptr _sourceFactory; - int64_t _generation; + std::atomic _idGenerator; + SourceFactory::UP _sourceFactory; + int64_t _generation; - using SubscriptionMap = std::map; + typedef std::map SubscriptionMap; SubscriptionMap _subscriptionMap; std::mutex _lock; }; diff --git a/config/src/vespa/config/common/reloadhandler.h b/config/src/vespa/config/common/reloadhandler.h index cd329e8d525..73016a2f883 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() = default; + virtual ~ReloadHandler() { } }; } diff --git a/config/src/vespa/config/common/sourcefactory.h b/config/src/vespa/config/common/sourcefactory.h index f11a070fd95..0236bea2802 100644 --- a/config/src/vespa/config/common/sourcefactory.h +++ b/config/src/vespa/config/common/sourcefactory.h @@ -13,6 +13,7 @@ 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 eb7ca10fa0d..dc1eadf3cf6 100644 --- a/config/src/vespa/config/frt/connectionfactory.h +++ b/config/src/vespa/config/frt/connectionfactory.h @@ -12,10 +12,12 @@ 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() = default; + virtual ~ConnectionFactory() { } }; } diff --git a/config/src/vespa/config/frt/frtconnectionpool.cpp b/config/src/vespa/config/frt/frtconnectionpool.cpp index 15ba8472af0..629baf67cf8 100644 --- a/config/src/vespa/config/frt/frtconnectionpool.cpp +++ b/config/src/vespa/config/frt/frtconnectionpool.cpp @@ -27,8 +27,10 @@ FRTConnectionPool::FRTConnectionKey::operator==(const FRTConnectionKey& right) c return _hostname == right._hostname; } -FRTConnectionPool::FRTConnectionPool(FNET_Transport & transport, const ServerSpec & spec, const TimingValues & timingValues) - : _supervisor(std::make_unique(& transport)), +FRTConnectionPool::FRTConnectionPool(const ServerSpec & spec, const TimingValues & timingValues) + : _threadPool(std::make_unique(60_Ki)), + _transport(std::make_unique()), + _supervisor(std::make_unique(_transport.get())), _selectIdx(0), _hostname("") { @@ -37,9 +39,13 @@ FRTConnectionPool::FRTConnectionPool(FNET_Transport & transport, const ServerSpe _connections[key] = std::make_shared(spec.getHost(i), *_supervisor, timingValues); } setHostname(); + _transport->Start(_threadPool.get()); } -FRTConnectionPool::~FRTConnectionPool() = default; +FRTConnectionPool::~FRTConnectionPool() +{ + _transport->ShutDown(true); +} void FRTConnectionPool::syncTransport() @@ -144,19 +150,4 @@ 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 b7530f8536e..6b387fbf67f 100644 --- a/config/src/vespa/config/frt/frtconnectionpool.h +++ b/config/src/vespa/config/frt/frtconnectionpool.h @@ -13,6 +13,7 @@ class FastOS_ThreadPool; namespace config { class FRTConnectionPool : public ConnectionFactory { + private: /** @@ -31,6 +32,8 @@ private: int operator==(const FRTConnectionKey& right) const; }; + std::unique_ptr _threadPool; + std::unique_ptr _transport; std::unique_ptr _supervisor; int _selectIdx; vespalib::string _hostname; @@ -38,7 +41,7 @@ private: ConnectionMap _connections; public: - FRTConnectionPool(FNET_Transport & transport, const ServerSpec & spec, const TimingValues & timingValues); + FRTConnectionPool(const ServerSpec & spec, const TimingValues & timingValues); FRTConnectionPool(const FRTConnectionPool&) = delete; FRTConnectionPool& operator=(const FRTConnectionPool&) = delete; ~FRTConnectionPool() override; @@ -127,18 +130,5 @@ 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 1c740534730..9706e3bbd5e 100644 --- a/config/src/vespa/config/frt/frtsourcefactory.cpp +++ b/config/src/vespa/config/frt/frtsourcefactory.cpp @@ -1,21 +1,17 @@ // 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(std::unique_ptr connectionFactory, const TimingValues & timingValues, int traceLevel, const VespaVersion & vespaVersion, const CompressionType & compressionType) +FRTSourceFactory::FRTSourceFactory(ConnectionFactory::UP 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 19cf6241481..4331c6411dc 100644 --- a/config/src/vespa/config/frt/frtsourcefactory.h +++ b/config/src/vespa/config/frt/frtsourcefactory.h @@ -1,32 +1,28 @@ // 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(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; + FRTSourceFactory(ConnectionFactory::UP connectionFactory, const TimingValues & timingValues, int traceLevel, const VespaVersion & vespaVersion, const CompressionType & compressionType); + /** * Create source handling config described by key. */ std::unique_ptr createSource(std::shared_ptr holder, const ConfigKey & key) const override; private: - std::shared_ptr _connectionFactory; + ConnectionFactory::SP _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 40dc40d11ef..0b3944acc4e 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 ec70f921179..6af981807de 100644 --- a/config/src/vespa/config/subscription/sourcespec.cpp +++ b/config/src/vespa/config/subscription/sourcespec.cpp @@ -11,9 +11,6 @@ #include #include #include -#include -#include -#include #include namespace config { @@ -28,7 +25,7 @@ RawSpec::RawSpec(const vespalib::string & config) { } -std::unique_ptr +SourceFactory::UP RawSpec::createSourceFactory(const TimingValues &) const { return std::make_unique(_config); @@ -52,7 +49,7 @@ FileSpec::verifyName(const vespalib::string & fileName) } } -std::unique_ptr +SourceFactory::UP FileSpec::createSourceFactory(const TimingValues & ) const { return std::make_unique(*this); @@ -63,7 +60,7 @@ DirSpec::DirSpec(const vespalib::string & dirName) { } -std::unique_ptr +SourceFactory::UP DirSpec::createSourceFactory(const TimingValues & ) const { return std::make_unique(*this); @@ -120,15 +117,12 @@ ServerSpec::ServerSpec(const vespalib::string & hostSpec) initialize(hostSpec); } -std::unique_ptr +SourceFactory::UP ServerSpec::createSourceFactory(const TimingValues & timingValues) const { const auto vespaVersion = VespaVersion::getCurrentVersion(); - return std::make_unique( - std::make_unique(std::make_unique(64_Ki), - std::make_unique(), - *this, timingValues), - timingValues, _traceLevel, vespaVersion, _compressionType); + return std::make_unique(std::make_unique(*this, timingValues), timingValues, + _traceLevel, vespaVersion, _compressionType); } @@ -137,7 +131,7 @@ ConfigSet::ConfigSet() { } -std::unique_ptr +SourceFactory::UP 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 3c23233e3ab..a57b22ca322 100644 --- a/config/src/vespa/config/subscription/sourcespec.h +++ b/config/src/vespa/config/subscription/sourcespec.h @@ -156,6 +156,13 @@ 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. * @@ -186,9 +193,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