On demand OS connection manager #1645
Conversation
| previous_consumer_ = factory_->create(*peers.previous_consumer); | ||
| })) {} | ||
|
|
||
| // OdOsNotification |
There was a problem hiding this comment.
This comment exists in both header and cpp, maybe you should expand it a bit, because I don't understand, what it's supposed to mean
| RoundType round; | ||
| boost::optional<OnDemandConnectionManager::ProposalType> oproposal = | ||
| OnDemandConnectionManager::ProposalType{}; | ||
| auto proposal = oproposal.value().get(); |
There was a problem hiding this comment.
Won't it be better to firstly check optional for ASSERT_TRUE(..)?
There was a problem hiding this comment.
No, because it is not created from component being tested, but explicitly in two lines above by initializing optional with unique pointer.
| // shared lock | ||
| std::shared_lock<std::shared_timed_mutex> lock(mutex_); | ||
|
|
||
| current_consumer_->onTransactions(transactions); |
There was a problem hiding this comment.
Why there is a guarantee that pointer is not a null? Maybe initiate connections with some stub in ctor?
There was a problem hiding this comment.
Or wrap pointers with the optionals, or apply bind to a pointer.
| class OnDemandConnectionManager : public transport::OdOsNotification { | ||
| public: | ||
| struct CurrentPeers { | ||
| std::shared_ptr<shared_model::interface::Peer> issuer, current_consumer, |
There was a problem hiding this comment.
Add descriptions to the peers.
| std::shared_ptr<transport::OdOsNotificationFactory> factory_; | ||
| rxcpp::composite_subscription subscription_; | ||
|
|
||
| std::unique_ptr<transport::OdOsNotification> issuer_, current_consumer_, |
There was a problem hiding this comment.
Maybe create a structure similar to CurrentPeers with current connections.
It will lead to the following benefits: * increase testability of the code and explicit initialization in ctor, which will prevent the previous problem with pointers.
lebdron
force-pushed
the
feature/bft-os-transport
branch
from
06899b7
to
f3bf2c2
Compare
lebdron
force-pushed
the
feature/bft-os-conn-mgr
branch
from
6c4f1ad
to
78be2ae
Compare
lebdron
force-pushed
the
feature/bft-os-transport
branch
from
f3bf2c2
to
0e639e6
Compare
Signed-off-by: Andrei Lebedev <lebdron@gmail.com>
Signed-off-by: Andrei Lebedev <lebdron@gmail.com>
Signed-off-by: Andrei Lebedev <lebdron@gmail.com>
lebdron
force-pushed
the
feature/bft-os-conn-mgr
branch
from
78be2ae
to
d960af2
Compare
Signed-off-by: Andrei Lebedev <lebdron@gmail.com>
Description of the Change
Implementation of connection manager component for on demand OS. Manager receives new peers and forwards the requests. Methods are thread safe.
It uses connection factory each time new peers are received, so that required peers are used with each update.
Shared lock is used to allow concurrent method calls and updates of peers.