-
Notifications
You must be signed in to change notification settings - Fork 216
DataStorm demos build fixes #611
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
Conversation
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.
Pull Request Overview
This PR updates DataStorm demo applications to use the new constructor pattern from Ice framework pull request #4462. The changes remove configuration files that contained only optional properties and update the code to use direct constructor calls.
- Removes config files with only optional/commented properties from most demos
- Updates DataStorm::Node constructors to use argc/argv overload instead of config file parameter
- Adds explicit property configuration in the node demo which required specific connection settings
Reviewed Changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| cpp/DataStorm/stock/config.* | Removes unnecessary config files with only optional settings |
| cpp/DataStorm/stock/Writer.cpp | Updates constructor to use argc/argv instead of config file |
| cpp/DataStorm/stock/Reader.cpp | Updates constructor to use argc/argv instead of config file |
| cpp/DataStorm/sampleFilter/config.* | Removes unnecessary config files with only optional settings |
| cpp/DataStorm/sampleFilter/Writer.cpp | Updates constructor to use argc/argv instead of config file |
| cpp/DataStorm/sampleFilter/Reader.cpp | Updates constructor to use argc/argv instead of config file |
| cpp/DataStorm/node/config.* | Removes config files and moves required properties to code |
| cpp/DataStorm/node/Writer.cpp | Adds explicit property configuration and uses communicator-based constructor |
| cpp/DataStorm/node/Reader.cpp | Adds explicit property configuration and uses communicator-based constructor |
| cpp/DataStorm/keyFilter/config.* | Removes unnecessary config files with only optional settings |
| cpp/DataStorm/keyFilter/Writer.cpp | Updates constructor to use argc/argv instead of config file |
| cpp/DataStorm/keyFilter/Reader.cpp | Updates constructor to use argc/argv instead of config file |
| cpp/DataStorm/clock/config.* | Removes unnecessary config files with only optional settings |
| cpp/DataStorm/clock/Writer.cpp | Updates constructor to use argc/argv instead of config file |
| cpp/DataStorm/clock/Reader.cpp | Updates constructor to use argc/argv instead of config file |
| random_device rd; | ||
| mt19937 generator(rd()); | ||
| uniform_int_distribution<> id(1); | ||
| // Default properties. Communicators used by DataStorm must have a property set that can use the "DataStorm" |
Copilot
AI
Sep 25, 2025
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.
There are two spaces after 'properties.' in the comment on line 21. Should be a single space.
| // Default properties. Communicators used by DataStorm must have a property set that can use the "DataStorm" | |
| // Default properties. Communicators used by DataStorm must have a property set that can use the "DataStorm" |
| // CtrlCHandler must be called before the node is created or any other threads are started. | ||
| Ice::CtrlCHandler ctrlCHandler; | ||
|
|
||
| // Default properties. Communicators used by DataStorm must have a property set that can use the "DataStorm" |
Copilot
AI
Sep 25, 2025
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.
There are two spaces after 'properties.' in the comment on line 18. Should be a single space.
| // Default properties. Communicators used by DataStorm must have a property set that can use the "DataStorm" | |
| // Default properties. Communicators used by DataStorm must have a property set that can use the "DataStorm" |
|
|
||
| // Default properties. Communicators used by DataStorm must have a property set that can use the "DataStorm" | ||
| // opt-in prefix. | ||
| auto defaultProperties = make_shared<Ice::Properties>(vector<string>{"DataStorm"}); |
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 should improve this "reserved property" name syntax. It's not elegant.
| Ice::CommunicatorPtr communicator = Ice::initialize(initData); | ||
|
|
||
| // Make sure the communicator is destroyed at the end of this scope. | ||
| Ice::CommunicatorHolder ich{communicator}; |
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 should rework this code to be consistent with Greeter and the other demos. For a follow-up PR.
| { | ||
| // Instantiates DataStorm node. | ||
| DataStorm::Node node(argc, argv, "config.reader"); | ||
| DataStorm::Node node(argc, argv); |
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 should use {...} for constructors. For a follow-up PR.
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 will review this demos in a follow up PR, we haven't done much updates on them.
This PR fixes the DataStorm demos to use the updated constructors from zeroc-ice/ice#4462
For most demos they use a config file, but none of the properties in the config file was required, for those I just remove the config files and updated the code to use the argc/argv overload.
For the DataStorm/node demo I move the required properties to the Reader and Writer, which requires a bit more code.