Support loading add-on configurations by SebSparrowHawk · Pull Request #930 · eclipse-score/communication

SebSparrowHawk · 2026-08-14T12:20:18Z

Enables users to load add-on mw::com configurations after loading the actual application's configuration.
Only service definitions of these add-on configurations will be considered and merged into the existing configuration if there is no conflict with the already existing configuration.
Restructured the Configuration's internal data structures for service types and service instances so that the use of mutex/locks to protect the configuration can be limited to a minimum.

Review Checklist Evidence Notice - Merge Queue

This pull request was modified after the review checklist evidence was recorded.
The review checklist evidence visible here does no longer reflect the evidence that will be recorded at merge.
Please rely on the evidence in the git history once the pull request was merged.

The git history shows the evidence state at the time of merge queue entry.
A pull request may only enter the merge queue when all necessary review checklist acknowledgements are in place.
Changes made after this pull request enters the merge queue may update the evidence here,
but they do not affect the evidence recorded in the git history.

castler

As discussed, we have to rethink the "add" logik.

castler · 2026-08-17T12:40:24Z

+     *
+     * Pointer to Configuration is an atomic pointer because SetConfiguration() might be called from different threads,
+     * while it is read from Create() concurrently. Using std::atomic avoids the data race that could occur in this
+     * situation.


This does not change with the addon configuration feature - does it?

castler · 2026-08-17T12:50:25Z

+    {
+        return merge_result;
+    }
+    InstanceIdentifier::SetConfiguration(&configuration_);


Why should we do that? We are just changing the content of the original configuration object, its not that we create a new one. Thus, this change is not needed! You can then remove also the changes in the InstanceIdentifier no?

Yes, removed that.

castler · 2026-08-17T12:55:58Z

+    // Suppress "AUTOSAR C++14 A11-3-1", The rule declares: "Friend declarations shall not be used".
+    // Test only use to check internal state of configuration after (merge) operations.
+    // coverity[autosar_cpp14_a11_3_1_violation]
+    friend class ConfigurationFixture;


What internal state is that? Should not every change be publicly visible?

At the moment there are methods "ServiceTypeDeployments GetServiceTypes() const noexcept" and "ServiceInstanceDeployments GetServiceInstances() const noexcept" that are defined as private methods.
Because they should only be used internally and are useful for some test checks.

castler · 2026-08-18T05:19:38Z


 #include "score/result/result.h"

+#include <atomic>


This include should not be needed.

Right, removed

castler · 2026-08-18T05:31:09Z


 #include "score/result/result.h"

+#include "score/mw/log/logging.h"


You are not performing any logging in the header.

If you add this here, this will pollute the dependencies, thus better remove it.

Removed it.

castler · 2026-08-18T05:53:22Z

+    ///         over the persisted storage.
+    ServiceTypeDeployments GetServiceTypes() const noexcept
+    {
+        auto result = ServiceTypeDeployments();


you are initializing here the variable no?

Then easiest would be:

ServiceTypeDeployments result{};

otherwise its not clear if you are invoking a constructor in 160 or a method or ....

castler · 2026-08-18T05:55:07Z

+    ///         directly over the persisted storage.
+    ServiceInstanceDeployments GetServiceInstances() const noexcept
+    {
+        auto result = ServiceInstanceDeployments();


Same statement as above, prefer {}-Initialization.

castler · 2026-08-18T05:55:29Z

+            for (auto& element : *current_list.get())
+            {
+                for (auto& entry : *element.get())
+                {


Same question above, should those be moved - if no, make it const

Won't be moved, I'll make them const

castler · 2026-08-18T05:56:48Z

+
+    /// \brief Helper function to check if entry for this service_identifier is already stored in list of service type
+    /// deployments
+    static bool CheckServiceTypeExists(


Why do we make this static and the pass the member variable? Same question for the function below

I wanted to avoid to call std:.atomic_load_explicit(...) in a loop and therefore decided to load it just once and pass it by reference.
If that should be avoided, I could think about restructuring the whole check logic.

castler · 2026-08-18T05:57:45Z

    GlobalConfiguration global_configuration_;
    TracingConfiguration tracing_configuration_;
+
+    std::mutex merge_mutex_;


Please give an explanation in a comment what this mutex is used for.

Only used to avoid parallel merges...or something like that. Not used in "read" path, due to atomic list construct...yada yada

github-actions · 2026-08-20T14:36:43Z

Review Checklist Evidence Notice - Merge Queue

This pull request was modified after the review checklist evidence was recorded.
The review checklist evidence visible here does no longer reflect the evidence that will be recorded at merge.
Please rely on the evidence in the git history once the pull request was merged.

The git history shows the evidence state at the time of merge queue entry.
A pull request may only enter the merge queue when all necessary review checklist acknowledgements are in place.
Changes made after this pull request enters the merge queue may update the evidence here,
but they do not affect the evidence recorded in the git history.

Enables users to load additional mw::com configurations. Introduced a new public API to load these configurations. It is expected that a complete configuration has been loaded before. The data structures of a Configuration to hold service type and service instance information has been changed in order to limit the required usage of mutexes to a minimum.

github-project-automation Bot added this to COM - Communication FT Aug 14, 2026

github-project-automation Bot moved this to Backlog in COM - Communication FT Aug 14, 2026

github-advanced-security Bot found potential problems Aug 14, 2026

View reviewed changes

Comment thread score/mw/com/impl/configuration/configuration.cpp Fixed

Comment thread score/mw/com/impl/configuration/configuration.cpp Fixed

Comment thread score/mw/com/impl/runtime.cpp Fixed

Comment thread score/mw/com/impl/runtime.cpp Fixed

SebSparrowHawk force-pushed the ssp_merging_addon_configs branch 2 times, most recently from 4569701 to a7c0120 Compare August 17, 2026 14:03

castler reviewed Aug 18, 2026

View reviewed changes

SebSparrowHawk force-pushed the ssp_merging_addon_configs branch 2 times, most recently from f00263b to a990e5c Compare August 20, 2026 08:18

castler previously approved these changes Aug 20, 2026

View reviewed changes

SebSparrowHawk marked this pull request as ready for review August 20, 2026 08:57

SebSparrowHawk requested review from LittleHuba, bemerybmw, crimson11, hoe-jo and limdor as code owners August 20, 2026 08:58

castler added this pull request to the merge queue Aug 20, 2026

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Aug 20, 2026

SebSparrowHawk dismissed castler’s stale review via b60b823 August 20, 2026 11:45

SebSparrowHawk force-pushed the ssp_merging_addon_configs branch from a990e5c to b60b823 Compare August 20, 2026 11:45

castler previously approved these changes Aug 20, 2026

View reviewed changes

castler added this pull request to the merge queue Aug 20, 2026

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Aug 20, 2026

castler added this pull request to the merge queue Aug 20, 2026

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Aug 20, 2026

SebSparrowHawk dismissed castler’s stale review via 93efafd August 21, 2026 06:21

SebSparrowHawk force-pushed the ssp_merging_addon_configs branch from b60b823 to 93efafd Compare August 21, 2026 06:21

castler approved these changes Aug 21, 2026

View reviewed changes

castler added this pull request to the merge queue Aug 21, 2026


		#include "score/result/result.h"

		#include "score/mw/log/logging.h"

Uh oh!

Conversation

SebSparrowHawk commented Aug 14, 2026 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review Checklist Evidence Notice - Merge Queue

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

castler left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions Bot commented Aug 20, 2026

Review Checklist Evidence Notice - Merge Queue

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

SebSparrowHawk commented Aug 14, 2026 •

edited by github-actions Bot

Loading