Skip to content

Bidirectional provider implementation #149

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

Conversation

rafaeling
Copy link
Contributor

@rafaeling rafaeling commented Mar 26, 2025

Introduced the FilterManager structure to manage filter subscriptions based on the lowest time interval.

Added new fields to the Subscriptions struct to manage ChangeSubscription and Providers by their UUID.

Modified sender: broadcast::Sender in ChangeSubscription to sender: broadcast::Sender<Option> to allow receiving None when the sender is gone. This enables returning an error (VISS case) or None to the consumer.

Implemented logic to propagate time-based filters to providers, notify subscriptions according to the time interval, and claim signals by providers during startup.

Added VISS subscription integration tests.

@rafaeling rafaeling requested a review from lukasmittag March 26, 2025 12:56
Copy link

codecov bot commented Mar 28, 2025

Codecov Report

Attention: Patch coverage is 44.79532% with 472 lines in your changes missing coverage. Please review.

Project coverage is 62.69%. Comparing base (0985941) to head (956e338).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
databroker/src/broker.rs 32.20% 221 Missing ⚠️
databroker/src/grpc/kuksa_val_v2/val.rs 35.05% 189 Missing ⚠️
databroker/src/viss/v2/server.rs 0.00% 23 Missing ⚠️
databroker/src/grpc/kuksa_val_v1/val.rs 0.00% 17 Missing ⚠️
databroker/src/grpc/kuksa_val_v2/conversions.rs 20.00% 8 Missing ⚠️
databroker/src/viss/v2/types.rs 0.00% 5 Missing ⚠️
databroker/src/filter/filter_manager.rs 97.61% 4 Missing ⚠️
databroker/src/types.rs 75.00% 3 Missing ⚠️
...tabroker/src/grpc/sdv_databroker_v1/conversions.rs 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #149      +/-   ##
==========================================
- Coverage   63.77%   62.69%   -1.08%     
==========================================
  Files          36       37       +1     
  Lines       17637    18325     +688     
==========================================
+ Hits        11248    11489     +241     
- Misses       6389     6836     +447     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@rafaeling rafaeling force-pushed the bidirectional-provider-implementation branch 3 times, most recently from 4117bb6 to e06d186 Compare March 31, 2025 07:22
@rafaeling rafaeling marked this pull request as ready for review March 31, 2025 07:23
@rafaeling rafaeling force-pushed the bidirectional-provider-implementation branch from a9ab8a7 to 26d129c Compare March 31, 2025 07:47
@rafaeling rafaeling requested a review from wba2hi March 31, 2025 09:05
@terbshaeusser-itk
Copy link

I also noticed during some testing that no UpdateFilter request is sent to the provider if no timebased filter is given in the VISS subscription message. Is this intentional? How would the provider in this case know, when to send new values?

@rafaeling
Copy link
Contributor Author

I also noticed during some testing that no UpdateFilter request is sent to the provider if no timebased filter is given in the VISS subscription message. Is this intentional? How would the provider in this case know, when to send new values?

Yes, it is intentional, my interpretation is that TimeBased filter is kind of mandatory -> (Arguments, of which path and filter are mandatory) https://www.w3.org/TR/viss2-core/#subscribe

I cannot find anywhere in the specification what should be the expected behaviour if the TimeBased filter is not provided in the subscribe request https://www.w3.org/TR/viss2-core/#timebased-filter-operation

@lukasmittag
Copy link
Contributor

lukasmittag commented Apr 11, 2025

In general I agree with @rafaeling, it seems not to be stated as clear as we would want:

The client may send a [subscribeRequest](https://www.w3.org/TR/viss2-transport/#dfn-subscriberequest) message to request a subscription to one or more signals, thereby requesting the server to repeatedly return subscription event messages, as specified by the [filter request](https://www.w3.org/TR/viss2-core/#filter-request) described in the [[viss2-core](https://www.w3.org/TR/viss2-transport/#bib-viss2-core)]. The server MAY reduce the number of [subcriptionEvent](https://www.w3.org/TR/viss2-transport/#dfn-subscriptionevent) messages sent to the client in order to reduce processing demands.

You can understand it in two ways:

  • repeatedly return subscription event messages -> if no timebased filter is set sent continously at the rate you want
  • as specified by the filter request described in the [[viss2-core] -> if timebased filter is set accordingly to it

but since it is in the same sentence it leaves doubts if timebased filter is mandatory.

@terbshaeusser-itk
Copy link

You can understand it in two ways:

* repeatedly return subscription event messages -> if no timebased filter is set sent continously at the rate you want
* as specified by the [filter request](https://www.w3.org/TR/viss2-core/#filter-request) described in the [[viss2-core] -> if timebased filter is set accordingly to it

but since it is in the same sentence it leaves doubts if timebased filter is mandatory.

I think there is a misunderstanding here. Our goal was not only to allow the client to basically throttle the sending of values but also to allow subscription based systems as a provider. It should be possible to have a provider that only sends values as long as at least one client is subscribed. This would help to reduce system load and unnecessary computations that might be necessary to transform "raw" signals to VSS signals before they are published to the databroker.

If an UpdateFilterRequest is now only sent when a subscription filter is present, the provider can never be sure if a client (without filter) is subscribed or not. Thus, the provider would need to send values all the time, just like in the current implementation.

@rafaeling
Copy link
Contributor Author

rafaeling commented Apr 15, 2025

You can understand it in two ways:

* repeatedly return subscription event messages -> if no timebased filter is set sent continously at the rate you want
* as specified by the [filter request](https://www.w3.org/TR/viss2-core/#filter-request) described in the [[viss2-core] -> if timebased filter is set accordingly to it

but since it is in the same sentence it leaves doubts if timebased filter is mandatory.

I think there is a misunderstanding here. Our goal was not only to allow the client to basically throttle the sending of values but also to allow subscription based systems as a provider. It should be possible to have a provider that only sends values as long as at least one client is subscribed. This would help to reduce system load and unnecessary computations that might be necessary to transform "raw" signals to VSS signals before they are published to the databroker.

If an UpdateFilterRequest is now only sent when a subscription filter is present, the provider can never be sure if a client (without filter) is subscribed or not. Thus, the provider would need to send values all the time, just like in the current implementation.

Fixed: If the subscribe method doesn’t specify a TimeBased filter, the default time_interval value used will now match the value provided when the signal is claimed from the databroker.

@rafaeling rafaeling force-pushed the bidirectional-provider-implementation branch from 62602b7 to 1010e8f Compare April 15, 2025 09:39
Copy link
Contributor

@lukasmittag lukasmittag left a comment

Choose a reason for hiding this comment

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

LGTM

@lukasmittag
Copy link
Contributor

https://gitlab.eclipse.org/eclipsefdn/helpdesk/-/issues/5893 since it is an eclipse wide issue and all checks beside the one are not failing its good to merge.

@rafaeling rafaeling force-pushed the bidirectional-provider-implementation branch from f2097f8 to 956e338 Compare April 30, 2025 10:10
@rafaeling rafaeling merged commit 821b926 into eclipse-kuksa:main Apr 30, 2025
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants