Skip to content

Allow clients to specify validity durations for bounds#231

Merged
tiyash-basu-frequenz merged 1 commit intofrequenz-floss:v0.x.xfrom
tiyash-basu-frequenz:specify_bounds_lifetime
Apr 2, 2024
Merged

Allow clients to specify validity durations for bounds#231
tiyash-basu-frequenz merged 1 commit intofrequenz-floss:v0.x.xfrom
tiyash-basu-frequenz:specify_bounds_lifetime

Conversation

@tiyash-basu-frequenz
Copy link
Copy Markdown
Contributor

This commit adds a field named validity_duration to the AddComponentBoundsRequest message, which can be used by clients to specify a custom default duration to bounds while adding them. If not specified, a default validity duration of 5 seconds is applied.

@tiyash-basu-frequenz tiyash-basu-frequenz added this to the v0.17.0 milestone Mar 28, 2024
@tiyash-basu-frequenz tiyash-basu-frequenz self-assigned this Mar 28, 2024
@tiyash-basu-frequenz tiyash-basu-frequenz requested a review from a team as a code owner March 28, 2024 10:05
@github-actions github-actions Bot added part:docs Affects the documentation part:protobuf Affects the protocol buffer definition files labels Mar 28, 2024
@tiyash-basu-frequenz
Copy link
Copy Markdown
Contributor Author

The context being that we do not want clients to set bounds for arbitrarily long durations:
An alternative approach to the one in this PR would be to allow the client to ask for an arbitrary time, but limit it internally to 15 minutes (and of course, document it). I did not go for that approach because that approach requires users to go through the docs, whereas this approach enforces such limits directly in the API.

Would be interested in hearing your opinions.

@llucax
Copy link
Copy Markdown
Contributor

llucax commented Mar 28, 2024

An alternative approach to the one in this PR would be to allow the client to ask for an arbitrary time, but limit it internally to 15 minutes (and of course, document it). I did not go for that approach because that approach requires users to go through the docs, whereas this approach enforces such limits directly in the API.

I agree it is better to fail if the requested time can't be fulfilled, otherwise it could lead to unexpected behaviour. But any reason not to use an arbitrary number, document a maximum and minimum allowed value and fail the request if the value is out of the valid range? For me it looks a bit weird to have these times as an enum.

@tiyash-basu-frequenz
Copy link
Copy Markdown
Contributor Author

But any reason not to use an arbitrary number, document a maximum and minimum allowed value and fail the request if the value is out of the valid range? For me it looks a bit weird to have these times as an enum.

If it is an enum, users will get a compilation error if they choose an invalid timestamp. If itis a number, then they will get a runtime error, which could mean more mistakes and more error-handling (so more code). Hence my first choice was an enum.

But TBH, I do not have a strong opinion either way. I figured I could just create the PR and discuss it here.

This commit adds a field named `validity_duration` to the
`AddComponentBoundsRequest` message, which can be used by clients to
specify a custom default duration to bounds while adding them. If not
specified, a default validity duration of 5 seconds is applied.

The decision to use an enumerated list of allowed durations has been
impacted by our intention to not allow users to set arbitrarily long
validity durations on bounds. Using an enum makes it clear while using the
API.

Signed-off-by: Tiyash Basu <tiyash.basu@frequenz.com>
@llucax
Copy link
Copy Markdown
Contributor

llucax commented Mar 28, 2024

I'm not completely against the enum either, which I guess it is a bad thing, as it is harder to decide :P

I just find it a bit weird, but I never used it either, so maybe I'm just concerned that it might be a bit inflexible but it is an unjustified concern and it might be more than enough flexibility. So probably it is better to get someone that really use bounds to talk. Maybe @shsms @daniel-zullo-frequenz @Marenz ?

@shsms
Copy link
Copy Markdown
Contributor

shsms commented Mar 28, 2024

The SDK won't be sending bounds once we upgrade to this version, just setting power. For power, the interval is not customizable right? If it is customizable, we'd have to get an option from the actors, so that the SDK doesn't have to make generalized decisions.

Also, I'm curious about the motivation for asking the users to decide. Why/when would I as a user choose 15 minutes over the default 5 seconds, and repeat commands every 3 seconds? Are there any specific usecases/requirements that's driving this?

@tiyash-basu-frequenz tiyash-basu-frequenz linked an issue Mar 28, 2024 that may be closed by this pull request
@tiyash-basu-frequenz
Copy link
Copy Markdown
Contributor Author

The SDK won't be sending bounds once we upgrade to this version, just setting power. For power, the interval is not customizable right? If it is customizable, we'd have to get an option from the actors, so that the SDK doesn't have to make generalized decisions.

The idea is to make it customizable.

Also, I'm curious about the motivation for asking the users to decide. Why/when would I as a user choose 15 minutes over the default 5 seconds, and repeat commands every 3 seconds? Are there any specific usecases/requirements that's driving this?

Good point, I forgot to link the issue. It is linked now, and has the use-case mentioned there.

@tiyash-basu-frequenz
Copy link
Copy Markdown
Contributor Author

ping

Copy link
Copy Markdown

@thomas-nicolai-frequenz thomas-nicolai-frequenz left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown

@stefan-brus-frequenz stefan-brus-frequenz left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown
Contributor

@shsms shsms left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown

@thea-leake thea-leake left a comment

Choose a reason for hiding this comment

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

LGMT!

@tiyash-basu-frequenz tiyash-basu-frequenz merged commit 4a0e53c into frequenz-floss:v0.x.x Apr 2, 2024
@tiyash-basu-frequenz tiyash-basu-frequenz deleted the specify_bounds_lifetime branch April 2, 2024 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

part:docs Affects the documentation part:protobuf Affects the protocol buffer definition files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow clients to specify durations for provided bounds

7 participants