Extend bidirectional provider by introducing sample interval filtering #141
Conversation
rafaeling
changed the title
proto/kuksa/val/v2/types.proto
Outdated
| // Duration of the active call. If it is not set, call will least for ever. | ||
| int32 duration_ms = 1; | ||
| // Max desired sample update interval. | ||
| SampleInterval max_sample_interval = 2; |
There was a problem hiding this comment.
I would use int32 inline
There was a problem hiding this comment.
Since it will be also used in Metadata I decided to create its own message to keep the same semantic. What do you think?
| int32 max_interval_ms = 1; | ||
| } | ||
|
|
||
| enum FilterError { |
There was a problem hiding this comment.
this shall be aligned with grpc errors? because we encountered difficulties with own error codes and stuff like that right?
There was a problem hiding this comment.
The purpose of this error enum was to transmit Provider's context errors, like can network error, provider overloaded, etc
proto/kuksa/val/v2/val.proto
Outdated
| @@ -44,6 +45,7 @@ service VAL { | |||
| // PERMISSION_DENIED if access is denied for any of the requested signals. | |||
| // INVALID_ARGUMENT if the request is empty or provided path is too long | |||
| // - MAX_REQUEST_PATH_LENGTH: usize = 1000; | |||
| // UNAVAILABLE if there is no provider currently providing the signals | |||
| // | |||
There was a problem hiding this comment.
Using UNAVAILABBLE for this purpose has some drawbacks:
- Clients cannot easily distinguish, if just the signal has no provider or if the whole data broker is unavailable
- I would strongly prefer to get the state of the other available signals even if some of them are "not provided" at the moment
There was a problem hiding this comment.
Decided after meeting to remove it and keep the non availability of provider as None in datapoint
| @@ -298,6 +309,28 @@ message BatchActuateStreamResponse { | |||
| Error error = 2; | |||
| } | |||
|
|
|||
| message UpdateFilterRequest { | |||
| // Databroker sends filters to provider. | |||
There was a problem hiding this comment.
A request id isn't required here?
Can we make sure, that the data broker send only one UpdateFilterRequest to a provider and waits for the response before sending another UpdateFilterRequest?
| FILTER_ERROR_CODE_OK = 1; | ||
| FILTER_ERROR_CODE_NETWORK_ERROR = 2; | ||
| FILTER_ERROR_CODE_OVERLOAD = 3; | ||
| } |
There was a problem hiding this comment.
Couldn't there be more errors like
- Unknown signal id
| @@ -47,6 +47,24 @@ message Value { | |||
| } | |||
| } | |||
|
|
|||
| message SampleInterval { | |||
There was a problem hiding this comment.
Why SampleInterval? Why not SampleRate, or maybe Frequency, maybe as mentioned in some of the other findings it can be inlined because it's just an object with a single property.
There was a problem hiding this comment.
It is used in Metadata and Filter messages, wanted to keep same semantic and meaning between both messages.
| @@ -131,6 +131,7 @@ service VAL { | |||
| // NOT_FOUND if the specified root branch does not exist. | |||
| // UNAUTHENTICATED if no credentials provided or credentials has expired | |||
| // INVALID_ARGUMENT if the provided path or wildcard is wrong. | |||
| // UNAVAILABLE if there is no provider currently providing the signal -> do we want this error for list metadata? | |||
There was a problem hiding this comment.
I wouldn't do this: This prevents clients - including providers - from getting the metadata of a signal if a provider for it is not (yet) available.
| message PublishValueResponse { | ||
| } | ||
|
|
| map<int32, Datapoint> data_points = 2; | ||
| } | ||
|
|
||
| message PublishValuesResponse { | ||
| int32 request_id = 1; | ||
| uint64 request_id = 1; /// Unique request id for the stream that can be used to match the corresponding request. |
There was a problem hiding this comment.
Missed that yesterday: Do we really need a 64 bit request id? This would mean, we expect that request out of 4 billion could potentially overlap ...
BTW: Wouldn't that be an incompatible interface change - requiring to proceed to v3?
| @@ -131,6 +131,7 @@ service VAL { | |||
| // NOT_FOUND if the specified root branch does not exist. | |||
| // UNAUTHENTICATED if no credentials provided or credentials has expired | |||
| // INVALID_ARGUMENT if the provided path or wildcard is wrong. | |||
| // UNAVAILABLE if there is no provider currently providing the signal -> do we want this error for list metadata? | |||
| // | |||
| rpc ListMetadata(ListMetadataRequest) returns (ListMetadataResponse); | |||
|
|
|||
There was a problem hiding this comment.
Should we add an error to PublishValue for the case that the addressed signal is already "claimed" by another provider (via OpenProviderStream -> ProvideSignalRequest). E.g.
// RESOURCE_EXHAUSTED if the addressed signal is already occupied by another provider
Or what is the current behavior of the broker in those cases?
| message ProvideActuationResponse { | ||
| } | ||
|
|
||
| message ProvideSignalRequest { | ||
| map<int32, SampleInterval> signals_sample_intervals = 1; | ||
| } | ||
|
|
||
| message ProvideSignalResponse { | ||
| } | ||
|
|
There was a problem hiding this comment.
Sorry, I didn't recognize till now that the ProvideSignalRequest/Response is also new.
We should specify the behavior when a provider freshly claims a signal: Does he immediately start to send updates or does he wait until a UpdateFilterRequest comes in? Or should we send back a filter map in the response (like in the UpdateFilterRequest) which is defining the initial filter? What about "legacy" providers not knowing the filter concept?
And BTW: We did not specify the behavior if already another provider has claimed signals for actuation or provisioning. Maybe there should be some respective "error signalling" in the responses.
No description provided.