213 : ExtractionModule & ExtractionChain implementation and integrations#633
Open
213 : ExtractionModule & ExtractionChain implementation and integrations#633
Conversation
…ialization - Added a destructor to ConfigurationManager to ensure proper cleanup of JSON configuration objects. - Refactored the constructor to initialize all configuration pointers to nullptr and load configurations from a JSON file. - Updated condition checks in LoadConfFromJSONFile to use logical AND instead of OR for better validation of JSON object types.
- Updated the return values in the parseJsonConf method across multiple configuration files (ChronoGrapher, ChronoKeeper, ChronoPlayer, ChronoVisor) to use the chronolog::CL_SUCCESS constant instead of a hardcoded value of 1 for improved clarity and consistency in success indication. - Adjusted the deployment script to reflect changes in configuration paths for better maintainability.
- Replaced assert statements with conditional checks in the parseJsonConf methods across ChronoGrapher, ChronoKeeper, ChronoPlayer, and ChronoVisor configuration files to improve error handling. - Added error messages to standard output for invalid configuration types, ensuring clearer feedback for configuration issues. - Updated return values to use chl::CL_ERR_INVALID_CONF for consistency in error reporting.
- Added conditional checks in the parseJsonConf methods for CLOCK_CONF and AUTH_CONF to handle parsing errors more gracefully. - Introduced error messages to standard output for invalid configurations, enhancing feedback for users. - Ensured consistent exit behavior on parsing failures by utilizing chronolog::CL_ERR_INVALID_CONF.
… & test/unit/extraction_chain_config
…, ExtractionChain is using variant array instead of argument pack; new KeeperExtractionConfiguration; new KeeperExtractionChain implementation; new GrapherExtractionConfiguration & new GrapherExtractionChain implementation
… and Grapher; added tests/functional/chrono-grapher
This comment was marked as resolved.
This comment was marked as resolved.
This was
linked to
issues
May 6, 2026
iameneko
requested changes
May 7, 2026
Contributor
iameneko
left a comment
iameneko
left a comment
There was a problem hiding this comment.
let me know your thoughts on my comments
iameneko
approved these changes
May 7, 2026
fkengun
reviewed
May 7, 2026
fkengun
reviewed
May 7, 2026
| bool is_active() const { return (nullptr != rdma_sender); } | ||
|
|
||
| private: | ||
| tl::engine& sender_tl_engine; // local tl::engine |
Contributor
There was a problem hiding this comment.
tl::engine& cannot be rebound, so the copy-assignment at ChunkExtractorRDMA.cpp:75
sender_tl_engine = other.get_sender_engine();calls tl::engine::operator= on the externally-owned engine itself, not on the local member. Thallium's copy-assign explicitly runs ~engine() on the LHS before swapping m_impl (thallium/engine.hpp:276-281) — if that's the last reference, finalize() runs on the live process engine; otherwise the upstream engine silently points at a different impl. Either way the "borrowed reference" contract is broken.
Recommend changing the member to tl::engine* sender_tl_engine; so copy-assign rebinds the pointer instead of mutating the engine. Same fix needed in DualEndpointChunkExtractorRDMA.h:40 / DualEndpointChunkExtractorRDMA.cpp:157.
Collaborator
Author
There was a problem hiding this comment.
I see . so the engine is silently finalized .. nice...
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
this PR contains the code changes for the suite of issues grouped under issue #213
Please, note that there's a follow-up #635 that will have a separate PR