Skip to content
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

[core] new-pub-sub-matching #1615

Closed
wants to merge 7 commits into from
Closed

Conversation

rex-schilasky
Copy link
Contributor

Description

  • layer states (enabling) exchanged between pub/sub
  • layer activation based on layer priority lists for local and remote connections

layer activation based on layer priority lists for local and remote connections
@rex-schilasky rex-schilasky added the cherry-pick-to-NONE Don't cherry-pick these changes label May 24, 2024
@@ -95,6 +95,7 @@ namespace eCAL
TLayer layer;
layer.type = static_cast<eTLayerType>(rand() % (tl_all + 1));
layer.version = rand() % 100;
layer.enabled = rand() % 2 == 1;
layer.confirmed = rand() % 2 == 1;
return layer;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😨 Tests should be reproducible!
if you are generating numbers for a tests, you should do so with a seed, so they are always generating the same.
And in this case, combinations would maked more sense instead of just random numbers.

Copy link
Contributor

@KerstinKeller KerstinKeller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a remark: I think we really need to discuss about starting layers even though no subscribers are there. What is the current behavior? If noone is subscribed, nothing happens.
If someone is subscribed on udp, is the publisher also always sending on shm, even though noone is listening on SHM?

What are the latencies, when a Publisher is present, a subscriber wants to subscriber, until the subscriber receives the first message, assuming e.g. the publisher sends at 10 Hz frequency. How is it in eCAL5?


eTLayerType type = 1; // transport layer type
int32 version = 2; // transport layer version
bool confirmed = 3; // transport layer used ?
bool enabled = 6; // transport layer enabled ?
bool confirmed = 3; // transport layer usage confirmed ?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain the logic again?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

enabled == enabled by the (user) publisher/subscriber logic
confirmed == data were successful send over the layer finally (used by the monitoring mainly)

@rex-schilasky
Copy link
Contributor Author

rex-schilasky commented May 24, 2024

Just a remark: I think we really need to discuss about starting layers even though no subscribers are there. What is the current behavior? If noone is subscribed, nothing happens. If someone is subscribed on udp, is the publisher also always sending on shm, even though noone is listening on SHM?

What are the latencies, when a Publisher is present, a subscriber wants to subscriber, until the subscriber receives the first message, assuming e.g. the publisher sends at 10 Hz frequency. How is it in eCAL5?

The old behavior is that write layers are enabled (instantiated) at the construction phase of the CDataWriter if their mode is configured as off or auto. This leads to the fact that a memory file is created, a socket is opened .. even there is no local/external subscription.

The new behavior is to not instantiate any of theses layers if their is no matching subscription for this kind of transport. The connection timing is still insides 2 registration loops as guaranteed in eCAL5.

@KerstinKeller
Copy link
Contributor

KerstinKeller commented May 24, 2024

I was thinking about it more. What do you think about the following logic:

The publisher provides which layers it supports. In the config this may be activated.
The subscriber provides a list of priorities, on which layer it would prefer to receive data.
eg [shm, udp, tcp] or [tcp, udp]. The first entry has highest priority.

The publisher will then send on the "first" layer which is activated, which the subscriber wants.

With this combination (and the knowledge, if a layer is supported for the combi / location of publisher & subscriber), for both publisher and subscriber, it's clear on which layer the data will flow.

e.g.

Publisher 1: 
Host: host1
shm: true
udp: true
tcp: true
Subscriber 1:
Host: host1
layers: [shm, udp, tcp]

Publisher 1 and subscriber 1 will communicate on SHM

Subscriber 2:
Host: host1
layers: [tcp, udp]

Publisher 1 and subscriber 1 will communicate on TCP

Subscriber 3:
Host: host2
layers: [shm, udp, tcp]

Publisher 1 and subscriber 3 will communicate on UDP, because they are on different hosts but shm does not support inter_host communication

Publisher 2: 
Host: host1
shm: false
udp: true
tcp: true

Publisher 2 and Subscriber 1 will communicate on udp
Publisher 2 and Subscriber 2 will communicate on tcp
Publisher 2 and Subscriber 3 will communicate on udp

I think it's pretty straighforward, and there is not forward - backward communication necessary.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

TLayer::eTransportLayer CDataWriter::DetermineTransportLayer2Start(const std::vector<eTLayerType>& enabled_pub_layer_, const std::vector<eTLayerType>& enabled_sub_layer_, bool same_host_)
{
// determine the priority list to use
Publisher::Configuration::LayerPriorityVector& layer_priority_vector = same_host_ ? m_config.layer_priority_local : m_config.layer_priority_remote;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: variable 'layer_priority_vector' of type 'Publisher::Configuration::LayerPriorityVector &' (aka 'vectorTLayer::eTransportLayer &') can be declared 'const' [misc-const-correctness]

Suggested change
Publisher::Configuration::LayerPriorityVector& layer_priority_vector = same_host_ ? m_config.layer_priority_local : m_config.layer_priority_remote;
Publisher::Configuration::LayerPriorityVector const& layer_priority_vector = same_host_ ? m_config.layer_priority_local : m_config.layer_priority_remote;

@rex-schilasky
Copy link
Contributor Author

replaced by new PR new-pub-sub-matching-2

@rex-schilasky rex-schilasky deleted the feature/new-pub-sub-matching branch July 17, 2024 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick-to-NONE Don't cherry-pick these changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants