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

Feature: continous subscription #16

Conversation

lukasmittag
Copy link
Contributor

@lukasmittag lukasmittag commented Apr 18, 2024

eclipse/kuksa.val#734

Behaviour:

  • frequency present in subscrbe request -> continousSubscription
  • frequency None in subscribe request -> changeSubscription

@lukasmittag lukasmittag force-pushed the feature/continous_subscription branch from fec8f18 to 5ed8372 Compare April 23, 2024 08:05
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this new line here?

Copy link
Contributor

@argerus argerus left a comment

Choose a reason for hiding this comment

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

So my initial impression is that this doesn't really solve the issue.

As currently implemented, this will notify the subscriber every interval millisecond, regardless of whether any new value was received from any provider.

That would seem to make it useless for "heartbeat signals".

It seems to make more sense to notify exactly when a new value arrives, but to limit this to a certain number per second / certain interval.

@lukasmittag
Copy link
Contributor Author

Okay, I see. I can understand. I wasn't aware of this since the PR of @rafaeling did the same.

@rafaeling
Copy link
Contributor

rafaeling commented Apr 25, 2024

Yes I did the same, I guess I forgot the goal of the PR and the issue context after some time.
When I started working on this I remember to read this here eclipse/kuksa.val#652 (comment) since signal_type was not an inherent property I started with the approach of adding signal_type to VSS but after Comminity meeting it was decided to use better an optional parameter on the request to "decide implicitly" the type of signal/subscription (Change or Continuous).
This PR I think implements a good feature but agree with that it does not really resolve the issue.

Copy link
Contributor

@BjoernAtBosch BjoernAtBosch left a comment

Choose a reason for hiding this comment

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

Just looked at the proto changes. (My Rust knowledge is too poor to rate the rest, currently)

@@ -98,6 +98,7 @@ message SubscribeEntry {
// Subscribe to changes in datapoints.
message SubscribeRequest {
repeated SubscribeEntry entries = 1;
optional uint64 interval_ms = 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

Findings:

  • What is the meaning of "interval_ms"? It should be already clear from the parameter name. Currently, I cannot see (without reading the documentation) if it is minimum, typical, or maximum interval.
  • Don't we add detailed documentation about the behaviour in the proto files? I agree with Erik (in the old PR): We should also describe the semantics in the interface definition. Imho, the "outside visible behaviour" always belongs to the contract description of an interface definition.
  • In the current definition this parameter is related to the overall subscription, but not to certain signals - which is fine for me if the parameter defines the max update frequency. But the behaviour should be clearly defined if the the subscription comprises signals with different update frequencies.
  • What is the behaviour, if signals are updated faster than the max frequency requested by the client:
    • Does it get "just" some sample value at the moment the update interval is reached,
    • do we send some mean value of the last samples (how would that be calculated?),
    • do we accumulated values and send them as a batch,
    • ...?

If we are unsure about the behaviour to be achieved by this PR, we should rethink merging it ...

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, if this comment is not "fully" matching - I initially commented the corresponding PR in the old repo.
I also added a comment in the issue: eclipse/kuksa.val#652

@erikbosch
Copy link
Contributor

@lukasmittag - what is the status of this one. Still relevant?

@erikbosch
Copy link
Contributor

Agreed in discussion to close this one for now

@erikbosch erikbosch closed this Oct 22, 2024
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.

5 participants