diff options
author | Arne Juul <arnej@yahoo-inc.com> | 2018-10-08 09:32:51 +0000 |
---|---|---|
committer | Arne Juul <arnej@yahoo-inc.com> | 2018-10-08 09:40:14 +0000 |
commit | 1bd250dd19a2d4dcae1494736fd8fc7c73645784 (patch) | |
tree | 39db5cf63d7d00df118b68ba70e5fedd73210dce /slobrok/src | |
parent | 59a11d316b9465b1fa8b19604958696e8a955328 (diff) |
Modernize slobrok internals more
* simplify / clarify some code
* modernize slightly
* reduce data multiplication
* rename command class to ScriptCommand
* use "localhost" in test
Diffstat (limited to 'slobrok/src')
-rwxr-xr-x | slobrok/src/tests/startsome/startsome.sh | 12 | ||||
-rw-r--r-- | slobrok/src/vespa/slobrok/server/cmd.cpp | 117 | ||||
-rw-r--r-- | slobrok/src/vespa/slobrok/server/cmd.h | 36 | ||||
-rw-r--r-- | slobrok/src/vespa/slobrok/server/exchange_manager.cpp | 39 | ||||
-rw-r--r-- | slobrok/src/vespa/slobrok/server/exchange_manager.h | 19 | ||||
-rw-r--r-- | slobrok/src/vespa/slobrok/server/history.cpp | 13 | ||||
-rw-r--r-- | slobrok/src/vespa/slobrok/server/history.h | 2 | ||||
-rw-r--r-- | slobrok/src/vespa/slobrok/server/rpc_server_manager.cpp | 4 | ||||
-rw-r--r-- | slobrok/src/vespa/slobrok/server/rpc_server_manager.h | 6 | ||||
-rw-r--r-- | slobrok/src/vespa/slobrok/server/rpchooks.cpp | 4 |
10 files changed, 128 insertions, 124 deletions
diff --git a/slobrok/src/tests/startsome/startsome.sh b/slobrok/src/tests/startsome/startsome.sh index 35049813206..ed3d2b29537 100755 --- a/slobrok/src/tests/startsome/startsome.sh +++ b/slobrok/src/tests/startsome/startsome.sh @@ -32,7 +32,7 @@ ${SLOBROK} -p 18484 & sleep 1 ./slobrok_rpc_info_app \ - tcp/`hostname`:18481 verbose > rpc-method-list + tcp/localhost:18481 verbose > rpc-method-list echo port 18481: ${SBCMD} 18481 slobrok.callback.listNamesServed @@ -52,13 +52,13 @@ listall listall -add=tcp/`hostname`:18482 +add=tcp/localhost:18482 ${SBCMD} 18481 slobrok.admin.addPeer $add $add -add=tcp/`hostname`:18483 +add=tcp/localhost:18483 ${SBCMD} 18481 slobrok.admin.addPeer $add $add -add=tcp/`hostname`:18484 +add=tcp/localhost:18484 ${SBCMD} 18481 slobrok.admin.addPeer $add $add -add=tcp/`hostname`:18481 +add=tcp/localhost:18481 ${SBCMD} 18484 slobrok.admin.addPeer $add $add listall @@ -67,7 +67,7 @@ listall listall -rem=tcp/`hostname`:18482 +rem=tcp/localhost:18482 ${SBCMD} 18481 slobrok.admin.removePeer $rem $rem ./slobrok_tstdst_app -s 18482 -p 18490 -n testrpcsrv/19 & diff --git a/slobrok/src/vespa/slobrok/server/cmd.cpp b/slobrok/src/vespa/slobrok/server/cmd.cpp index e728c8f80ad..99e86814852 100644 --- a/slobrok/src/vespa/slobrok/server/cmd.cpp +++ b/slobrok/src/vespa/slobrok/server/cmd.cpp @@ -14,61 +14,63 @@ namespace slobrok { //----------------------------------------------------------------------------- -struct RegRpcSrvData -{ -private: - RegRpcSrvData(const RegRpcSrvData &); - RegRpcSrvData &operator=(const RegRpcSrvData &); +struct ScriptData { + SBEnv &env; + const std::string name; + const std::string spec; + FRT_RPCRequest * const registerRequest; -public: enum { RDC_INIT, XCH_WANTADD, CHK_RPCSRV, XCH_DOADD, XCH_IGNORE, RDC_INVAL } _state; - SBEnv &env; - const std::string name; - const std::string spec; - FRT_RPCRequest * const registerRequest; - - RegRpcSrvData(SBEnv &e, const std::string &n, const std::string &s, FRT_RPCRequest *r) - : _state(RDC_INIT), env(e), name(n), spec(s), registerRequest(r) + ScriptData(SBEnv &e, const std::string &n, const std::string &s, FRT_RPCRequest *r) + : env(e), name(n), spec(s), registerRequest(r), _state(RDC_INIT) {} }; -RegRpcSrvCommand::RegRpcSrvCommand(std::unique_ptr<RegRpcSrvData> data) - : _data(std::move(data)) +//----------------------------------------------------------------------------- + +const std::string & +ScriptCommand::name() { return _data->name; } + +const std::string & +ScriptCommand::spec() { return _data->spec; } + +ScriptCommand::ScriptCommand(std::unique_ptr<ScriptData> data) + : _data(std::move(data)) {} -RegRpcSrvCommand::RegRpcSrvCommand(RegRpcSrvCommand &&) = default; -RegRpcSrvCommand & RegRpcSrvCommand::operator =(RegRpcSrvCommand &&) = default; -RegRpcSrvCommand::~RegRpcSrvCommand() = default; +ScriptCommand::ScriptCommand(ScriptCommand &&) = default; +ScriptCommand& +ScriptCommand::operator= (ScriptCommand &&) = default; +ScriptCommand::~ScriptCommand() = default; -RegRpcSrvCommand -RegRpcSrvCommand::makeRegRpcSrvCmd(SBEnv &env, - const std::string &name, const std::string &spec, - FRT_RPCRequest *req) +ScriptCommand +ScriptCommand::makeRegRpcSrvCmd(SBEnv &env, + const std::string &name, const std::string &spec, + FRT_RPCRequest *req) { - return RegRpcSrvCommand(std::make_unique<RegRpcSrvData>(env, name, spec, req)); + return ScriptCommand(std::make_unique<ScriptData>(env, name, spec, req)); } -RegRpcSrvCommand -RegRpcSrvCommand::makeRemRemCmd(SBEnv &env, const std::string & name, const std::string &spec) +ScriptCommand +ScriptCommand::makeRemRemCmd(SBEnv &env, const std::string & name, const std::string &spec) { - auto data = std::make_unique<RegRpcSrvData>(env, name, spec, nullptr); - data->_state = RegRpcSrvData::XCH_IGNORE; - return RegRpcSrvCommand(std::move(data)); + auto data = std::make_unique<ScriptData>(env, name, spec, nullptr); + data->_state = ScriptData::XCH_IGNORE; + return ScriptCommand(std::move(data)); } void -RegRpcSrvCommand::doRequest() +ScriptCommand::doRequest() { - LOG_ASSERT(_data->_state == RegRpcSrvData::RDC_INIT); + LOG_ASSERT(_data->_state == ScriptData::RDC_INIT); doneHandler(OkState()); } -void -RegRpcSrvCommand::cleanupReservation(RegRpcSrvData & data) +void cleanupReservation(ScriptData & data) { RpcServerMap &map = data.env._rpcsrvmap; const ReservedName *rsvp = map.getReservation(data.name.c_str()); @@ -78,13 +80,20 @@ RegRpcSrvCommand::cleanupReservation(RegRpcSrvData & data) } void -RegRpcSrvCommand::doneHandler(OkState result) +ScriptCommand::doneHandler(OkState result) { - LOG_ASSERT(_data != nullptr); - std::unique_ptr<RegRpcSrvData> dataUP = std::move(_data); - RegRpcSrvData & data = *dataUP; + LOG_ASSERT(_data); + std::unique_ptr<ScriptData> dataUP = std::move(_data); + LOG_ASSERT(! _data); + ScriptData & data = *dataUP; + const char *name_p = data.name.c_str(); + const char *spec_p = data.spec.c_str(); + ExchangeManager &xch = data.env._exchanger; + RpcServerManager &rsm = data.env._rpcsrvmanager; + if (result.failed()) { - LOG(warning, "failed in state %d: %s", data._state, result.errorMsg.c_str()); + LOG(warning, "failed [%s->%s] in state %d: %s", + name_p, spec_p, data._state, result.errorMsg.c_str()); cleanupReservation(data); // XXX should handle different state errors differently? if (data.registerRequest != nullptr) { @@ -93,30 +102,30 @@ RegRpcSrvCommand::doneHandler(OkState result) } else { LOG(warning, "ignored: %s", result.errorMsg.c_str()); } - goto alldone; + return; } - if (data._state == RegRpcSrvData::RDC_INIT) { - LOG(spam, "phase wantAdd(%s,%s)", data.name.c_str(), data.spec.c_str()); - data._state = RegRpcSrvData::XCH_WANTADD; - data.env._exchanger.wantAdd(data.name.c_str(), data.spec.c_str(), std::move(dataUP)); + if (data._state == ScriptData::RDC_INIT) { + LOG(spam, "phase wantAdd(%s,%s)", name_p, spec_p); + data._state = ScriptData::XCH_WANTADD; + xch.wantAdd(std::move(dataUP)); return; - } else if (data._state == RegRpcSrvData::XCH_WANTADD) { - LOG(spam, "phase addManaged(%s,%s)", data.name.c_str(), data.spec.c_str()); - data._state = RegRpcSrvData::CHK_RPCSRV; - data.env._rpcsrvmanager.addManaged(data.name, data.spec.c_str(), std::move(dataUP)); + } else if (data._state == ScriptData::XCH_WANTADD) { + LOG(spam, "phase addManaged(%s,%s)", name_p, spec_p); + data._state = ScriptData::CHK_RPCSRV; + rsm.addManaged(std::move(dataUP)); return; - } else if (data._state == RegRpcSrvData::CHK_RPCSRV) { - LOG(spam, "phase doAdd(%s,%s)", data.name.c_str(), data.spec.c_str()); - data._state = RegRpcSrvData::XCH_DOADD; - data.env._exchanger.doAdd(data.name.c_str(), data.spec.c_str(), std::move(dataUP)); + } else if (data._state == ScriptData::CHK_RPCSRV) { + LOG(spam, "phase doAdd(%s,%s)", name_p, spec_p); + data._state = ScriptData::XCH_DOADD; + xch.doAdd(std::move(dataUP)); return; - } else if (data._state == RegRpcSrvData::XCH_DOADD) { - LOG(debug, "done doAdd(%s,%s)", data.name.c_str(), data.spec.c_str()); - data._state = RegRpcSrvData::RDC_INVAL; + } else if (data._state == ScriptData::XCH_DOADD) { + LOG(debug, "done doAdd(%s,%s)", name_p, spec_p); + data._state = ScriptData::RDC_INVAL; // all OK data.registerRequest->Return(); goto alldone; - } else if (data._state == RegRpcSrvData::XCH_IGNORE) { + } else if (data._state == ScriptData::XCH_IGNORE) { goto alldone; } // no other state should be possible diff --git a/slobrok/src/vespa/slobrok/server/cmd.h b/slobrok/src/vespa/slobrok/server/cmd.h index 07b3d3ee0ba..02ae16e457b 100644 --- a/slobrok/src/vespa/slobrok/server/cmd.h +++ b/slobrok/src/vespa/slobrok/server/cmd.h @@ -9,34 +9,26 @@ class FRT_RPCRequest; namespace slobrok { class SBEnv; +struct ScriptData; -struct RegRpcSrvData; - -/** - * @class RegRpcSrvCommand - * @brief Small "script" to handle the various stages of a registration - * - * XXX should change name, also used for other tasks - **/ -class RegRpcSrvCommand +class ScriptCommand { private: - std::unique_ptr<RegRpcSrvData> _data; - static void cleanupReservation(RegRpcSrvData &); - RegRpcSrvCommand(std::unique_ptr<RegRpcSrvData> data); + std::unique_ptr<ScriptData> _data; + ScriptCommand(std::unique_ptr<ScriptData> data); public: - virtual void doneHandler(OkState result); - void doRequest(); + const std::string &name(); + const std::string &spec(); - RegRpcSrvCommand(RegRpcSrvCommand &&); - RegRpcSrvCommand & operator=(RegRpcSrvCommand &&); - virtual ~RegRpcSrvCommand(); + ScriptCommand(ScriptCommand &&); + ScriptCommand& operator= (ScriptCommand &&); + ~ScriptCommand(); - static RegRpcSrvCommand makeRegRpcSrvCmd(SBEnv &env, const std::string &name, const std::string &spec, FRT_RPCRequest *req); - static RegRpcSrvCommand makeRemRemCmd(SBEnv &env, const std::string &name, const std::string &spec); -}; + static ScriptCommand makeRegRpcSrvCmd(SBEnv &env, const std::string &name, const std::string &spec, FRT_RPCRequest *req); + static ScriptCommand makeRemRemCmd(SBEnv &env, const std::string &name, const std::string &spec); -//----------------------------------------------------------------------------- + void doneHandler(OkState result); + void doRequest(); +}; } // namespace slobrok - diff --git a/slobrok/src/vespa/slobrok/server/exchange_manager.cpp b/slobrok/src/vespa/slobrok/server/exchange_manager.cpp index e74724803e8..6636e832b61 100644 --- a/slobrok/src/vespa/slobrok/server/exchange_manager.cpp +++ b/slobrok/src/vespa/slobrok/server/exchange_manager.cpp @@ -69,8 +69,8 @@ ExchangeManager::getPartnerList() void ExchangeManager::forwardRemove(const std::string & name, const std::string & spec) { - WorkPackage *package = new WorkPackage(WorkPackage::OP_REMOVE, name, spec, *this, - RegRpcSrvCommand::makeRemRemCmd(_env, name, spec)); + WorkPackage *package = new WorkPackage(WorkPackage::OP_REMOVE, *this, + ScriptCommand::makeRemRemCmd(_env, name, spec)); for (const auto & entry : _partners) { package->addItem(entry.second.get()); } @@ -78,9 +78,9 @@ ExchangeManager::forwardRemove(const std::string & name, const std::string & spe } void -ExchangeManager::doAdd(const std::string &name, const std::string &spec, RegRpcSrvCommand rdc) +ExchangeManager::doAdd(ScriptCommand rdc) { - WorkPackage *package = new WorkPackage(WorkPackage::OP_DOADD, name, spec, *this, std::move(rdc)); + WorkPackage *package = new WorkPackage(WorkPackage::OP_DOADD, *this, std::move(rdc)); for (const auto & entry : _partners) { package->addItem(entry.second.get()); @@ -90,9 +90,9 @@ ExchangeManager::doAdd(const std::string &name, const std::string &spec, RegRpcS void -ExchangeManager::wantAdd(const std::string &name, const std::string &spec, RegRpcSrvCommand rdc) +ExchangeManager::wantAdd(ScriptCommand rdc) { - WorkPackage *package = new WorkPackage(WorkPackage::OP_WANTADD, name, spec, *this, std::move(rdc)); + WorkPackage *package = new WorkPackage(WorkPackage::OP_WANTADD, *this, std::move(rdc)); for (const auto & entry : _partners) { package->addItem(entry.second.get()); } @@ -157,21 +157,21 @@ ExchangeManager::WorkPackage::WorkItem::~WorkItem() { if (_pendingReq != nullptr) { _pendingReq->Abort(); + // _pendingReq cleared by RequestDone Method LOG_ASSERT(_pendingReq == nullptr); } } -ExchangeManager::WorkPackage::WorkPackage(op_type op, const std::string & name, const std::string & spec, - ExchangeManager &exchanger, RegRpcSrvCommand donehandler) +ExchangeManager::WorkPackage::WorkPackage(op_type op, + ExchangeManager &exchanger, + ScriptCommand script) : _work(), _doneCnt(0), _numDenied(0), - _donehandler(std::move(donehandler)), + _script(std::move(script)), _exchanger(exchanger), - _optype(op), - _name(name), - _spec(spec) + _optype(op) { } @@ -188,9 +188,9 @@ ExchangeManager::WorkPackage::doneItem(bool denied) (int)_doneCnt, (int)_work.size(), (int)_numDenied); if (_doneCnt == _work.size()) { if (_numDenied > 0) { - _donehandler.doneHandler(OkState(_numDenied, "denied by remote")); + _script.doneHandler(OkState(_numDenied, "denied by remote")); } else { - _donehandler.doneHandler(OkState()); + _script.doneHandler(OkState()); } delete this; } @@ -203,6 +203,9 @@ ExchangeManager::WorkPackage::addItem(RemoteSlobrok *partner) if (! partner->isConnected()) { return; } + const char *name_p = _script.name().c_str(); + const char *spec_p = _script.spec().c_str(); + FRT_RPCRequest *r = _exchanger._env.getSupervisor()->AllocRPCRequest(); // XXX should recheck rpcsrvmap again if (_optype == OP_REMOVE) { @@ -213,13 +216,13 @@ ExchangeManager::WorkPackage::addItem(RemoteSlobrok *partner) r->SetMethodName("slobrok.internal.doAdd"); } r->GetParams()->AddString(_exchanger._env.mySpec().c_str()); - r->GetParams()->AddString(_name.c_str()); - r->GetParams()->AddString(_spec.c_str()); + r->GetParams()->AddString(name_p); + r->GetParams()->AddString(spec_p); _work.push_back(std::make_unique<WorkItem>(*this, partner, r)); LOG(spam, "added %s(%s,%s,%s) for %s to workpackage", r->GetMethodName(), _exchanger._env.mySpec().c_str(), - _name.c_str(), _spec.c_str(), partner->getName().c_str()); + name_p, spec_p, partner->getName().c_str()); } @@ -229,7 +232,7 @@ ExchangeManager::WorkPackage::expedite() size_t sz = _work.size(); if (sz == 0) { // no remotes need doing. - _donehandler.doneHandler(OkState()); + _script.doneHandler(OkState()); delete this; return; } diff --git a/slobrok/src/vespa/slobrok/server/exchange_manager.h b/slobrok/src/vespa/slobrok/server/exchange_manager.h index ea335cbb3c0..a7ff60d9ecc 100644 --- a/slobrok/src/vespa/slobrok/server/exchange_manager.h +++ b/slobrok/src/vespa/slobrok/server/exchange_manager.h @@ -59,22 +59,21 @@ private: ~WorkItem(); }; std::vector<std::unique_ptr<WorkItem>> _work; - size_t _doneCnt; - size_t _numDenied; - RegRpcSrvCommand _donehandler; + size_t _doneCnt; + size_t _numDenied; + ScriptCommand _script; public: - ExchangeManager &_exchanger; + ExchangeManager &_exchanger; enum op_type { OP_NOP, OP_WANTADD, OP_DOADD, OP_REMOVE }; op_type _optype; - const std::string _name; - const std::string _spec; void addItem(RemoteSlobrok *partner); void doneItem(bool denied); void expedite(); WorkPackage(const WorkPackage&) = delete; WorkPackage& operator= (const WorkPackage&) = delete; - WorkPackage(op_type op, const std::string & name, const std::string & spec, - ExchangeManager &exchanger, RegRpcSrvCommand donehandler); + WorkPackage(op_type op, + ExchangeManager &exchanger, + ScriptCommand donehandler); ~WorkPackage(); }; @@ -94,8 +93,8 @@ public: void forwardRemove(const std::string & name, const std::string & spec); - void wantAdd(const std::string & name, const std::string & spec, RegRpcSrvCommand rdc); - void doAdd(const std::string & name, const std::string & spec, RegRpcSrvCommand rdc); + void wantAdd(ScriptCommand rdc); + void doAdd(ScriptCommand rdc); RemoteSlobrok *lookupPartner(const std::string & name) const; void healthCheck(); diff --git a/slobrok/src/vespa/slobrok/server/history.cpp b/slobrok/src/vespa/slobrok/server/history.cpp index f23b0cbb757..059f0ff4b89 100644 --- a/slobrok/src/vespa/slobrok/server/history.cpp +++ b/slobrok/src/vespa/slobrok/server/history.cpp @@ -11,10 +11,10 @@ void History::verify() const { if (_entries.size() > 0) { - citer_t i = _entries.begin(); + citer_t i = _entries.cbegin(); vespalib::GenCnt gen = i->gen; - while (++i != _entries.end()) { + while (++i != _entries.cend()) { gen.add(); LOG_ASSERT(gen == i->gen); } @@ -24,10 +24,7 @@ History::verify() const void History::add(const std::string &name, vespalib::GenCnt gen) { - HistoryEntry h; - _entries.push_back(h); - _entries.back().name = name; - _entries.back().gen = gen; + _entries.emplace_back(name, gen); if (_entries.size() > 1500) { _entries.erase(_entries.begin(), _entries.begin() + 500); @@ -54,8 +51,8 @@ History::has(vespalib::GenCnt gen) const std::set<std::string> History::since(vespalib::GenCnt gen) const { - citer_t i = _entries.begin(); - citer_t end = _entries.end(); + citer_t i = _entries.cbegin(); + citer_t end = _entries.cend(); while (i != end) { if (i->gen == gen) break; ++i; diff --git a/slobrok/src/vespa/slobrok/server/history.h b/slobrok/src/vespa/slobrok/server/history.h index ca90c927dc0..d88b1348c6d 100644 --- a/slobrok/src/vespa/slobrok/server/history.h +++ b/slobrok/src/vespa/slobrok/server/history.h @@ -14,6 +14,8 @@ private: struct HistoryEntry { std::string name; vespalib::GenCnt gen; + HistoryEntry(const std::string &n, vespalib::GenCnt g) + : name(n), gen(g) {} }; std::vector<HistoryEntry> _entries; diff --git a/slobrok/src/vespa/slobrok/server/rpc_server_manager.cpp b/slobrok/src/vespa/slobrok/server/rpc_server_manager.cpp index e63efd8089a..e3d7800f236 100644 --- a/slobrok/src/vespa/slobrok/server/rpc_server_manager.cpp +++ b/slobrok/src/vespa/slobrok/server/rpc_server_manager.cpp @@ -239,8 +239,10 @@ RpcServerManager::removeLocal(const std::string & name, const std::string &spec) void -RpcServerManager::addManaged(const std::string &name, const std::string &spec, RegRpcSrvCommand rdc) +RpcServerManager::addManaged(ScriptCommand rdc) { + const std::string &name = rdc.name(); + const std::string &spec = rdc.spec(); auto newRpcServer = std::make_unique<ManagedRpcServer>(name, spec, *this); ManagedRpcServer & rpcsrv = *newRpcServer; _rpcsrvmap.addNew(std::move(newRpcServer)); diff --git a/slobrok/src/vespa/slobrok/server/rpc_server_manager.h b/slobrok/src/vespa/slobrok/server/rpc_server_manager.h index 70ca10e480d..174afc1adcc 100644 --- a/slobrok/src/vespa/slobrok/server/rpc_server_manager.h +++ b/slobrok/src/vespa/slobrok/server/rpc_server_manager.h @@ -38,8 +38,8 @@ private: struct MRSandRRSC { ManagedRpcServer *rpcsrv; - RegRpcSrvCommand handler; - MRSandRRSC(ManagedRpcServer *d, RegRpcSrvCommand h) + ScriptCommand handler; + MRSandRRSC(ManagedRpcServer *d, ScriptCommand h) : rpcsrv(d), handler(std::move(h)) {} }; std::vector<MRSandRRSC> _addManageds; @@ -55,7 +55,7 @@ public: OkState addMyReservation(const std::string & name, const std::string & spec); bool alreadyManaged(const std::string & name, const std::string & spec); - void addManaged(const std::string & name, const std::string & spec, RegRpcSrvCommand rdc); + void addManaged(ScriptCommand rdc); OkState remove(ManagedRpcServer *rpcsrv); diff --git a/slobrok/src/vespa/slobrok/server/rpchooks.cpp b/slobrok/src/vespa/slobrok/server/rpchooks.cpp index 460b060cb82..ff1ce514084 100644 --- a/slobrok/src/vespa/slobrok/server/rpchooks.cpp +++ b/slobrok/src/vespa/slobrok/server/rpchooks.cpp @@ -269,8 +269,8 @@ RPCHooks::rpc_registerRpcServer(FRT_RPCRequest *req) } // need to actually setup management and decide result later req->Detach(); - RegRpcSrvCommand completer - = RegRpcSrvCommand::makeRegRpcSrvCmd(_env, dName, dSpec, req); + ScriptCommand completer + = ScriptCommand::makeRegRpcSrvCmd(_env, dName, dSpec, req); completer.doRequest(); } |