Conversation
This is the full migration to integrations, swapping out the base extractor and cleaning up the now unused components that have been replaced by tasks. The tests need to change because enabling history now introduces some requirements, due to the way the extractor is initialized. This means that we need to enable history _before_ constructing the extractor. Which is what would usually happen, but not all tests do this correctly.
einarmo
force-pushed
the
use-integrations-full
branch
from
28bdb54 to
417a313
Compare
einarmo
force-pushed
the
use-integrations-full
branch
from
417a313 to
67d26bb
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #964 +/- ##
==========================================
+ Coverage 80.05% 80.13% +0.07%
==========================================
Files 130 128 -2
Lines 17015 16504 -511
Branches 2503 2432 -71
==========================================
- Hits 13622 13225 -397
+ Misses 2625 2551 -74
+ Partials 768 728 -40
🚀 New features to boost your workflow:
|
|
/gemini review |
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the OPC-UA Extractor's internal task management by replacing the custom Looper and HistoryReader classes with a new ExtractorTaskScheduler and dedicated task implementations for browsing, pushing data, handling history, and managing subscriptions. The UAExtractor class has been updated to inherit from BaseExtractor<FullConfig> and now orchestrates these new tasks, streamlining the extractor's lifecycle and improving its robustness. Key changes include updating configuration handling, modifying how historical data and events are processed, and adapting various test cases to the new asynchronous task-based architecture. Review comments highlight the need for a try...finally block in ConfigurationTool/Checks/History.cs to ensure configuration consistency and address potential deadlocks in test disposal methods by recommending await DisposeAsync() instead of synchronous Wait() calls.
Toshad
added
the
waiting-for-risk-review
kornelione
added
risk-review-ongoing
3 participants
This is the full migration to integrations, swapping out the base extractor and cleaning up the now unused components that have been replaced by tasks.
The tests need to change because enabling history now introduces some requirements, due to the way the extractor is initialized. This means that we need to enable history before constructing the extractor. Which is what would usually happen, but not all tests do this correctly.
It may be possible to get this below 500 lines by front-loading some of the test changes, but I'm not sure. This is a very breaking change, and it's hard to get it lower.