Upgrade DataStorm to use a Default OA for OutgoingConnections #3355
Conversation
| @@ -210,15 +210,15 @@ namespace Ice | |||
| * @return The object adapter associated by default with new outgoing connections. | |||
| * @see Connection::getAdapter | |||
| */ | |||
| [[nodiscard]] ObjectAdapterPtr getDefaultObjectAdapter() const noexcept; | |||
| [[nodiscard]] ObjectAdapterPtr getDefaultObjectAdapter() const; | |||
There was a problem hiding this comment.
Removed because it can throw CommunicatorDestroyedExcetion
cpp/src/DataStorm/Instance.cpp
Outdated
| @@ -19,6 +19,12 @@ using namespace Ice; | |||
| Instance::Instance(CommunicatorPtr communicator, function<void(function<void()> call)> customExecutor) | |||
| : _communicator(std::move(communicator)) | |||
| { | |||
| if (_communicator->getDefaultObjectAdapter() != nullptr) | |||
There was a problem hiding this comment.
This wont catch some cases where the application set a default OA after starting the Node. For those we need to relay in the documentation.
| Node n2(Ice::initialize(initData)); | ||
| n2.getCommunicator()->destroy(); | ||
| Ice::CommunicatorHolder communicatorHolder(Ice::initialize(initData)); | ||
| Node n2(communicatorHolder.communicator()); | ||
| } |
There was a problem hiding this comment.
The previous code would destroy the communicator before destroying the node, which now results in CommunicatorDestroyerException thrown from DataStormI::Instance::destroy.
| @@ -179,6 +186,7 @@ Instance::destroy(bool ownsCommunicator) | |||
| timer->destroy(); | |||
| } | |||
|
|
|||
| _communicator->setDefaultObjectAdapter(nullptr); | |||
There was a problem hiding this comment.
This would throw CommunicatorDestroyedException exception if the application destroys the communicator before destroying the node. I believe this is acceptable, as the application should not destroy the communicator passed to a DataStorm node before destroying the node itself.
| cout << "testing send time discard policy... " << flush; | ||
| { | ||
| writers.update(false); // Not ready | ||
| vector<tuple<Ice::CommunicatorHolder, Node, Topic<string, int>, SingleKeyWriter<string, int>>> w; | ||
| vector<WriterHolder> w; |
There was a problem hiding this comment.
This is to avoid the undefined destruction order of tuples, we don't want the communicator to be destroyed before than the node.
cpp/src/DataStorm/Instance.cpp
Outdated
| @@ -19,6 +19,12 @@ using namespace Ice; | |||
| Instance::Instance(CommunicatorPtr communicator, function<void(function<void()> call)> customExecutor) | |||
| : _communicator(std::move(communicator)) | |||
| { | |||
| if (_communicator->getDefaultObjectAdapter() != nullptr) | |||
There was a problem hiding this comment.
Our usual style is if (_communicator->getDefaultObjectAdapter()) ...
cpp/test/DataStorm/api/Writer.cpp
Outdated
| @@ -36,8 +36,8 @@ void ::Writer::run(int argc, char* argv[]) | |||
| // Communicators shared with DataStorm must have a property set that can use the "DataStorm" opt-in prefix. | |||
| Ice::InitializationData initData; | |||
| initData.properties = make_shared<Ice::Properties>(vector<string>{"DataStorm"}); | |||
| Node n2(Ice::initialize(initData)); | |||
| n2.getCommunicator()->destroy(); | |||
| Ice::CommunicatorHolder communicatorHolder(Ice::initialize(initData)); | |||
This PR upgrades DataStorm implementation to use setDefaultObjectAdapter to set a default object adapter for outgoing connections. See #3337