-
Notifications
You must be signed in to change notification settings - Fork 593
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make (some) Ice properties opt-in (C++ only) #2983
Changes from 10 commits
feaa7da
884292e
5544078
ca57c54
f0ba1d2
79ce92c
5065e0b
22b772c
8a575a3
a50f850
cf53d1c
5a33ad2
eac6ffb
bbe4193
7b669ca
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,47 +38,115 @@ namespace DataStorm | |
/** | ||
* Construct a DataStorm node. | ||
* | ||
* A node is the main DataStorm object. It is required to construct topics. The constructor initializes | ||
* the Ice communicator using the given arguments. If the communicator creation fails, an Ice exception is | ||
* raised. | ||
* A node is the main DataStorm object. It is required to construct topics. The node uses the given Ice | ||
* communicator. | ||
* | ||
* @param argc The number of arguments in argv. | ||
* @param argv The configuration arguments. | ||
* @param configFile The path to an optional Ice configuration file. | ||
* @param customExecutor An optional executor used to execute user callbacks, if no callback executor is | ||
* provided the Node will use the default callback executor that executes callback in a dedicated thread. | ||
* | ||
* @param iceArgs Arguments which are passed to the Ice::initialize function. | ||
*/ | ||
template<class... T> Node(T&&... iceArgs) : _ownsCommunicator(true) | ||
Node( | ||
int& argc, | ||
const char* argv[], | ||
std::optional<std::string_view> configFile = std::nullopt, | ||
std::function<void(std::function<void()> call)> customExecutor = nullptr); | ||
|
||
/** | ||
* Construct a DataStorm node. | ||
* | ||
* A node is the main DataStorm object. It is required to construct topics. The node uses the given Ice | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This constructor (like most Node constructors) does not accept a communicator. So:
is not correct. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed. |
||
* communicator. | ||
* | ||
* @param argc The number of arguments in argv. | ||
* @param argv The configuration arguments. | ||
* @param configFile The path to an optional Ice configuration file. | ||
* @param customExecutor An optional executor used to execute user callbacks, if no callback executor is | ||
* provided the Node will use the default callback executor that executes callback in a dedicated thread. | ||
* | ||
*/ | ||
Node( | ||
int& argc, | ||
char* argv[], | ||
std::optional<std::string_view> configFile = std::nullopt, | ||
std::function<void(std::function<void()> call)> customExecutor = nullptr) | ||
: Node(argc, const_cast<const char**>(argv), configFile, customExecutor) | ||
{ | ||
init(Ice::initialize(std::forward<T>(iceArgs)...), nullptr); | ||
} | ||
|
||
#ifdef _WIN32 | ||
/** | ||
* Construct a DataStorm node. | ||
* | ||
* A node is the main DataStorm object. It is required to construct topics. The node uses the given Ice | ||
* communicator. | ||
* | ||
* @param communicator The Ice communicator used by the topic factory for its configuration and communications. | ||
* @param argc The number of arguments in argv. | ||
* @param argv The configuration arguments. | ||
* @param configFile The path to an optional Ice configuration file. | ||
* @param customExecutor An optional executor used to execute user callbacks, if no callback executor is | ||
* provided the Node will use the default callback executor that executes callback in a dedicated thread. | ||
* | ||
*/ | ||
Node( | ||
Ice::CommunicatorPtr communicator, | ||
int& argc, | ||
const wchar_t* argv[], | ||
std::optional<std::string_view> configFile = std::nullopt, | ||
std::function<void(std::function<void()> call)> customExecutor = nullptr); | ||
|
||
/** | ||
* Construct a DataStorm node. | ||
* | ||
* A node is the main DataStorm object. It is required to construct topics. The constructor initializes | ||
* the Ice communicator using the given arguments. If the communicator creation fails, an Ice exception is | ||
* raised. | ||
* A node is the main DataStorm object. It is required to construct topics. The node uses the given Ice | ||
* communicator. | ||
* | ||
* @param argc The number of arguments in argv. | ||
* @param argv The configuration arguments. | ||
* @param configFile The path to an optional Ice configuration file. | ||
* @param customExecutor An optional executor used to execute user callbacks, if no callback executor is | ||
* provided the Node will use the default callback executor that executes callback in a dedicated thread. | ||
* @param iceArgs Arguments which are passed to the Ice::initialize function. | ||
* | ||
*/ | ||
template<class... T> | ||
Node(std::function<void(std::function<void()> call)> customExecutor, T&&... iceArgs) : _ownsCommunicator(true) | ||
Node( | ||
int& argc, | ||
wchar_t* argv[], | ||
std::optional<std::string_view> configFile = std::nullopt, | ||
std::function<void(std::function<void()> call)> customExecutor = nullptr) | ||
: Node(argc, const_cast<const wchar_t**>(argv), configFile, customExecutor) | ||
{ | ||
init(Ice::initialize(std::forward<T>(iceArgs)...), std::move(customExecutor)); | ||
} | ||
#endif | ||
|
||
/** | ||
* Construct a DataStorm node. | ||
* | ||
* A node is the main DataStorm object. It is required to construct topics. The node uses the given Ice | ||
* communicator. | ||
* | ||
* @param configFile The path to an optional Ice configuration file. | ||
* @param customExecutor An optional executor used to execute user callbacks, if no callback executor is | ||
* provided the Node will use the default callback executor that executes callback in a dedicated thread. | ||
*/ | ||
Node( | ||
std::optional<std::string_view> configFile = std::nullopt, | ||
std::function<void(std::function<void()> call)> customExecutor = nullptr); | ||
|
||
/** | ||
* Construct a DataStorm node. | ||
* | ||
* A node is the main DataStorm object. It is required to construct topics. The node uses the given Ice | ||
* communicator. | ||
* | ||
* @param communicator The Ice communicator used by the topic factory for its configuration and communications. | ||
* This communicator must be initialized with a property set to use the "DataStorm" service prefix. | ||
* @param customExecutor An optional executor used to execute user callbacks, if no callback executor is | ||
* provided the Node will use the default callback executor that executes callback in a dedicated thread. | ||
*/ | ||
Node( | ||
Ice::CommunicatorPtr communicator, | ||
std::function<void(std::function<void()> call)> customExecutor = nullptr); | ||
|
||
/** | ||
* Construct a new Node by taking ownership of the given node. | ||
|
@@ -133,7 +201,10 @@ namespace DataStorm | |
Ice::ConnectionPtr getSessionConnection(const std::string& ident) const noexcept; | ||
|
||
private: | ||
void init(const Ice::CommunicatorPtr&, std::function<void(std::function<void()> call)> customExecutor); | ||
Node( | ||
const Ice::CommunicatorPtr&, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should be an |
||
std::function<void(std::function<void()> call)> customExecutor, | ||
bool ownsCommunicator); | ||
|
||
std::shared_ptr<DataStormI::Instance> _instance; | ||
std::shared_ptr<DataStormI::TopicFactory> _factory; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We needed to be able to intercept the construction of the Node's communicator to create a Property set with the "DataStorm" service prefix. So we can no longer just forward everything. @bernardnormier suggested we just offer a few constructors. This also means we can now always have the custom executor as a trailing parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The alternative is to not consider DataStorm a service, which is not really one. It is a user library given that you have to programmatically construct the node.
In theory any servers communicator can be shared with the node and pass to its constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer making DataStorm an opt-in reserved prefix, as opposed to allowing it all the time like the plugins.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this the same as IceDiscovery and IceLocatorDiscovery which are marked with service-prefix=false.?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the plugins (IceDiscovery etc.) are treated like Ice - always enabled.