[19942] Make remove unused entities compatible with the discovery trigger #83

Tempate · 2024-01-17T07:44:56Z

No description provided.

codecov-commenter · 2024-01-17T07:53:19Z

Codecov Report

Attention: Patch coverage is 46.42857% with 60 lines in your changes are missing coverage. Please review.

Project coverage is 34.12%. Comparing base (8c96a74) to head (4428faa).

Files	Patch %	Lines
...spipe_core/src/cpp/communication/dds/DdsBridge.cpp	31.34%	36 Missing and 10 partials ⚠️
...core/src/cpp/configuration/RoutesConfiguration.cpp	70.96%	3 Missing and 6 partials ⚠️
ddspipe_core/src/cpp/core/DdsPipe.cpp	62.50%	2 Missing and 1 partial ⚠️
ddspipe_core/src/cpp/communication/dds/Track.cpp	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #83      +/-   ##
==========================================
+ Coverage   34.10%   34.12%   +0.01%     
==========================================
  Files         154      155       +1     
  Lines        7861     7881      +20     
  Branches     3515     3518       +3     
==========================================
+ Hits         2681     2689       +8     
- Misses       3267     3287      +20     
+ Partials     1913     1905       -8

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ddspipe_core/src/cpp/configuration/RoutesConfiguration.cpp

ddspipe_core/include/ddspipe_core/configuration/RoutesConfiguration.hpp

ddspipe_core/src/cpp/communication/dds/DdsBridge.cpp

ddspipe_core/include/ddspipe_core/core/DdsPipe.hpp

ddspipe_core/include/ddspipe_core/communication/dds/DdsBridge.hpp

ddspipe_core/src/cpp/communication/dds/DdsBridge.cpp

juanlofer-eprosima · 2024-07-15T07:22:09Z

ddspipe_core/src/cpp/communication/dds/Track.cpp

-        {
-            writer_it.second->disable();
-        }
+        // Don't disable writers, as they may be used by other tracks


Although I agree this might make more sense, note that I believe this would imply a delayed abortion when stopping the pipe or blocking a topic (would keep looping through writers map until done). Also, I don't like the fact that this would make enable-disable asymmetrical.

Not disabling the writers wasn't an aesthetic choice. If you disable the writers there are many scenarios that don't work or segfault with discovery-trigger and remove-unused-entities. If you propose another solution I may consider it.

My comment was wrong, not disabling writers has no effect in how soon Track::transmit_ is aborted. After thinking about it, I prefer to leave it as it is: having "atomic" write operations for every taken data.

However, I still don't like the asymmetry. I propose:

Leave it as it is but with a nice comment for future developers to consider.

Do not enable writers in tracks' enable, create them always enabled and never disable, or handle enable/disable somewhere else (in DdsBridge for example, as long as we decide to keep the writers_ attribute).

ddspipe_core/src/cpp/communication/dds/DdsBridge.cpp

ddspipe_core/src/cpp/core/DdsPipe.cpp

juanlofer-eprosima · 2024-07-16T06:59:52Z

ddspipe_core/include/ddspipe_core/communication/dds/DdsBridge.hpp

    std::map<types::ParticipantId, std::unique_ptr<Track>> tracks_;

+    //! The writers of the bridge index by their participant_id.
+    std::map<types::ParticipantId, std::shared_ptr<IWriter>> writers_;


Design comment: up to now bridges contain tracks, and tracks the only ones holding reference to endpoints. Analyze implications (such as object lifespan) and try to maintain this principle if makes sense.

juanlofer-eprosima · 2024-07-16T07:03:40Z

ddspipe_core/src/cpp/core/DdsPipe.cpp

        }
    }
    else if (configuration_.remove_unused_entities && topic->topic_discoverer() != DEFAULT_PARTICIPANT_ID)
    {
-        // The bridge already exists. Create a writer in the participant who discovered it.
-        it_bridge->second->create_writer(topic->topic_discoverer());
+        it_bridge->second->create_endpoint(topic->topic_discoverer(), endpoint_kind);


Design comment: call to endpoint creation method is done both from pipe and bridge. Analyze if it would be possible to further isolate their functionality so it's only done from one of them.

Related: scope of remove_unused_endpoint (used both in pipe and bridge).

juanlofer-eprosima · 2024-07-16T07:50:01Z

ddspipe_core/src/cpp/core/DdsPipe.cpp

Piggyback: please add a comment explaining that comparison with DEFAULT_PARTICIPANT_ID is used to determine whether topics are builtin.

juanlofer-eprosima · 2024-07-16T07:54:22Z

ddspipe_core/src/cpp/communication/dds/Track.cpp

-        {
-            writer_it.second->disable();
-        }
+        // Don't disable writers, as they may be used by other tracks


My comment was wrong, not disabling writers has no effect in how soon Track::transmit_ is aborted. After thinking about it, I prefer to leave it as it is: having "atomic" write operations for every taken data.

However, I still don't like the asymmetry. I propose:

Leave it as it is but with a nice comment for future developers to consider.

Do not enable writers in tracks' enable, create them always enabled and never disable, or handle enable/disable somewhere else (in DdsBridge for example, as long as we decide to keep the writers_ attribute).

juanlofer-eprosima

Piggyback request:

Please add to the participant's discovery traces the participant id (missing in endpoint removed/ignored/changed qos cases), I believe this is quite helpful for debugging.

juanlofer-eprosima · 2024-07-16T07:58:00Z

ddspipe_core/include/ddspipe_core/communication/dds/DdsBridge.hpp

+     * @param routes_config: Configuration of the routes of the Bridge
+     * @param remove_unused_entities: Whether the Bridge should remove unused entities
+     * @param manual_topics: Topics that explicitally set a QoS attribute for this participant
+     * @param endpoint_kind: Kind of the endpoint that discovered the topic


NIT: I believe DdsBridge should be agnostic (or at least has been until now) to discovery events. So instead of passing the kind of the endpoint that discovered the topic or was discovered, I would pass the kind of the endpoint that should be created.

juanlofer-eprosima · 2024-07-16T08:01:12Z

ddspipe_core/src/cpp/core/DdsPipe.cpp

@@ -527,7 +531,8 @@ void DdsPipe::create_new_service_nts_(
 }

 void DdsPipe::activate_topic_nts_(
-        const utils::Heritable<DistributedTopic>& topic) noexcept
+        const utils::Heritable<DistributedTopic>& topic,
+        const EndpointKind& endpoint_kind /*= EndpointKind::reader*/) noexcept


Be careful with the following case: an endpoint is discovered while its topic is blocked, then it is only added to current_topics_ hence losing the kind information. So if the topic is then allowed, a bridge is wrongly created with the default kind value.

Related scenario: subsequent relevant discovered endpoints would not be taken into account, but only the first one (used in DdsBridge constructor).

Potential solution: create an EndpointsManager that would keep track of all created endpoints, and also the ones that should be created once enabled (so DdsBridges will always be created even if the topic is disabled). This new object would then contain all references of entities, so the creation of a track would require passing a reference to this object as well as a map of ids.

juanlofer-eprosima · 2024-07-16T09:54:27Z

ddspipe_core/src/cpp/communication/dds/DdsBridge.cpp

-    // Add the writer to the tracks it has routes for.
-    add_writer_to_tracks_nts_(participant_id, writer);
+    // Create the track
+    create_track_nts_(participant_id, reader, writers_);


Shouldn't you be passing only the IWriters in the route?

juanlofer-eprosima · 2024-07-16T10:31:34Z

ddspipe_core/src/cpp/communication/dds/DdsBridge.cpp

+    assert(participant_id != DEFAULT_PARTICIPANT_ID);
+
+    // Create the writer
+    auto writer = create_writer_nts_(participant_id);


Another corner case: shouldn't we avoid creating writers if not present in any route?

Related: should we also avoid creating a reader if there is a "blockage" route? I'd say yes.

Signed-off-by: tempate <danieldiaz@eprosima.com>

juanlofer-eprosima · 2024-09-20T10:56:52Z

ddspipe_core/src/cpp/configuration/RoutesConfiguration.cpp

+RoutesConfiguration::RoutesMap RoutesConfiguration::routes_of_readers(
+        const std::map<types::ParticipantId, bool>& participant_ids) const noexcept
+{
+    static RoutesConfiguration::RoutesMap readers_routes;


Remove static.

Tempate temporarily deployed to codecov January 17, 2024 07:45 — with GitHub Actions Inactive

Tempate temporarily deployed to codecov January 17, 2024 09:07 — with GitHub Actions Inactive

Tempate changed the title ~~Make remove unused entities compatible with the discovery trigger~~ [19942] Make remove unused entities compatible with the discovery trigger Jan 29, 2024

LuciaEchevarria99 requested changes Apr 10, 2024

View reviewed changes

Tempate temporarily deployed to codecov April 10, 2024 14:03 — with GitHub Actions Inactive

Tempate force-pushed the feature/dynamic_tracks_discovery_trigger branch from fd5c1bb to 4428faa Compare April 10, 2024 14:09

Tempate temporarily deployed to codecov April 10, 2024 14:09 — with GitHub Actions Inactive

Tempate mentioned this pull request Jul 8, 2024

[19942] Make remove unused entities compatible with the discovery trigger eProsima/DDS-Router#460

Open

Tempate force-pushed the feature/dynamic_tracks_discovery_trigger branch from 4428faa to 6abb4d6 Compare July 8, 2024 07:37

Tempate had a problem deploying to codecov July 8, 2024 07:37 — with GitHub Actions Failure

juanlofer-eprosima requested changes Jul 15, 2024

View reviewed changes

ddspipe_core/src/cpp/core/DdsPipe.cpp Show resolved Hide resolved

Tempate force-pushed the feature/dynamic_tracks_discovery_trigger branch from 6abb4d6 to 1cf2453 Compare July 15, 2024 14:27

Tempate had a problem deploying to codecov July 15, 2024 14:27 — with GitHub Actions Failure

juanlofer-eprosima reviewed Jul 16, 2024

View reviewed changes

juanlofer-eprosima requested changes Jul 16, 2024

View reviewed changes

Tempate added 5 commits September 18, 2024 12:17

Make remove unused entities compatible with the discovery trigger

01b592e

Signed-off-by: tempate <danieldiaz@eprosima.com>

Generate the routes outside of the DdsBridge

ddae53b

Signed-off-by: tempate <danieldiaz@eprosima.com>

Uncrustify

e77bab7

Signed-off-by: tempate <danieldiaz@eprosima.com>

Apply suggestions

e00db76

Signed-off-by: tempate <danieldiaz@eprosima.com>

Apply suggestions

0789d95

Signed-off-by: tempate <danieldiaz@eprosima.com>

juanlofer-eprosima force-pushed the feature/dynamic_tracks_discovery_trigger branch from 1cf2453 to 0789d95 Compare September 18, 2024 10:40

juanlofer-eprosima temporarily deployed to codecov September 18, 2024 10:40 — with GitHub Actions Inactive

juanlofer-eprosima requested changes Sep 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[19942] Make remove unused entities compatible with the discovery trigger #83

[19942] Make remove unused entities compatible with the discovery trigger #83

Tempate commented Jan 17, 2024

codecov-commenter commented Jan 17, 2024 •

edited

Loading

juanlofer-eprosima Jul 15, 2024

Tempate Jul 15, 2024

juanlofer-eprosima Jul 16, 2024

juanlofer-eprosima Jul 16, 2024

juanlofer-eprosima Jul 16, 2024

juanlofer-eprosima Jul 16, 2024

juanlofer-eprosima Jul 16, 2024

juanlofer-eprosima Jul 16, 2024

juanlofer-eprosima left a comment

juanlofer-eprosima Jul 16, 2024

juanlofer-eprosima Jul 16, 2024 •

edited

Loading

juanlofer-eprosima Jul 16, 2024

juanlofer-eprosima Jul 16, 2024 •

edited

Loading

juanlofer-eprosima Jul 16, 2024 •

edited

Loading

juanlofer-eprosima Jul 16, 2024

juanlofer-eprosima Jul 16, 2024 •

edited

Loading

juanlofer-eprosima Sep 20, 2024

[19942] Make remove unused entities compatible with the discovery trigger #83

Are you sure you want to change the base?

[19942] Make remove unused entities compatible with the discovery trigger #83

Conversation

Tempate commented Jan 17, 2024

codecov-commenter commented Jan 17, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juanlofer-eprosima left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juanlofer-eprosima Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juanlofer-eprosima Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

juanlofer-eprosima Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juanlofer-eprosima Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jan 17, 2024 •

edited

Loading

juanlofer-eprosima Jul 16, 2024 •

edited

Loading

juanlofer-eprosima Jul 16, 2024 •

edited

Loading

juanlofer-eprosima Jul 16, 2024 •

edited

Loading

juanlofer-eprosima Jul 16, 2024 •

edited

Loading