Adding GetValue support #4
Conversation
| @@ -46,7 +46,12 @@ impl From<&proto::Datapoint> for broker::Datapoint { | |||
| impl From<broker::Datapoint> for Option<proto::Datapoint> { | |||
| fn from(from: broker::Datapoint) -> Self { | |||
| match from.value { | |||
| broker::DataValue::NotAvailable => None, | |||
| broker::DataValue::NotAvailable => Some(proto::Datapoint { | |||
There was a problem hiding this comment.
This a discussion point. For GetValue we expect a ValueState:.Failure to be returned if a value not yet exist, but not for subscribe. One solution is to map here but filter higher up (in subscribe)
|
@lukasmittag @rafaeling @argerus My first PR with substantial Rust changes. Seems to do what it shall but we could discuss how we want subscribe to react if the is no valid value - should you get a value first when there actually is a value, i.e. never receive notAvailable? As I am a Rust beginner feel free to propose changes that makes the rust look more compact or improves reuse. |
| @@ -147,18 +159,20 @@ message ValueRestrictionString { | |||
| repeated string allowed_values = 1; | |||
| } | |||
|
|
|||
| // Used to indicate status of a signal in Databroker | |||
| // This is given as an alternative to Value, so if there is a valid value | |||
| // ValueFailure shall not be specified. | |||
There was a problem hiding this comment.
Just a thought - would it be better to name this enum ValueInfo? At least getting NOT_PROVIDED is not necessarily a failure, and could theoretically be used by some provider in PublishValueRequest to indicate that the previous value should be deleted.
There was a problem hiding this comment.
I agree that calling it failure is a bit misleading. I also think having the value inside a value_state is unnecessary as it adds one more level of nesting to get to the actual value.
I think something like this would be more intuitive:
message DataPoint {
google.protobuf.Timestamp timestamp = 1;
Value value = 3;
Status status = 10;
}
enum Status {
STATUS_UNSPECIFIED = 0;
STATUS_OK = 1;
STATUS_VALUE_NOT_AVAILBLE = 2;
}If there is no value, a provider (or databroker) can set the value to None and the corresponding reason for it in the status field.
One "downside" of this is that it's possible to set a value while at the same time setting the status to something other than STATUS_OK (and vice versa setting STATUS_OK while not setting any value).
But doing that can simply be disallowed by databroker in the same way that it doesn't allow setting a value of the wrong data type today.
There was a problem hiding this comment.
I like the proposal.
And if we document (in *.proto) how various combinations/situations should be handled by client and databroker it should not be a problem.
| Scenario | value | status |
|---|---|---|
| Value available for signal | <value> |
STATUS_UNSPECIFIED or STATUS_OK |
| Value NOT available for signal | None |
STATUS_VALUE_NOT_AVAILABLE |
Anyway - will not have time for any update next week, but @rafaeling or @lukasmittag are free to change if they like and gree
| // The signal is known, but no value is provided currently | ||
| // Can theoretically also be used in write request to "delete" a value |
There was a problem hiding this comment.
Should we possible for each one indicate in which direction they can/shall be used? I can see a use-case for sending NOT_PROVIDED towards databroker, but I see no reason to send INTERNAL_ERROR to Databroker.
We can discuss if we want to keep INTERNAL_ERROR. For single calls I see no need as we can return a GRPC error code. For calls that shall return multiple signals (like GetValues) it could make sense to be able to return other signals. For streaming it makes sense if we want to continue to keep the stream open upon an internal error to be able to continue subscription for other signals. But internal error should never happen right?
Work in progress