Skip to content

Conversation

@shsms
Copy link
Contributor

@shsms shsms commented Jul 8, 2024

Before the power manager was introduced, the request_timeout parameter was used to specify a different timeout for the set_power calls made to the api service.

The propose_power method is much more high level and it no longer makes sense to provide a request_timeout parameter, because it could conflict with other proposals and it would be hard to figure out which timeout to use.

From now on, it may be specified at application startup, through the new optional api_power_request_timeout parameter in the microgrid.initialize() method.

@shsms shsms requested a review from a team as a code owner July 8, 2024 08:48
@github-actions github-actions bot added part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests part:data-pipeline Affects the data pipeline part:actor Affects an actor ot the actors utilities (decorator, etc.) part:microgrid Affects the interactions with the microgrid labels Jul 8, 2024
@shsms shsms changed the title # Remove request_timeout parameter from propose_power methods Remove request_timeout parameter from propose_power methods Jul 8, 2024
@shsms shsms added this to the v1.0.0-rc800 milestone Jul 8, 2024
Copy link
Contributor

@llucax llucax left a comment

Choose a reason for hiding this comment

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

LGTM, just a suggestion to improve documentation, as I'm not sure what would be the effect of changing the timeout or what happens when the timeout is reached.

Args:
resampler_config: Config to pass on to the resampler.
api_power_request_timeout: Timeout to use when making power requests to
the microgrid API.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it would be good to document what happens if it times out? As a user I'm not sure what to do with this. Why should I increase or decrease the timeout?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a few more lines. The only thing is that I've not been specific about the exact duration for which the components get blocked, because currently it is hard-coded to exponentially increase from 1s to a max of 30s when there are subsequent timeouts.

I think we don't have to provide a way to change the duration until we can do so from the UI.

Copy link
Contributor

@daniel-zullo-frequenz daniel-zullo-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 too, only the suggestion from Luca needs to be checked/addressed

@shsms shsms force-pushed the remove-req-timeout branch from 62afe72 to e756de5 Compare July 9, 2024 10:25
@shsms shsms added this pull request to the merge queue Jul 9, 2024
Merged via the queue into frequenz-floss:v1.x.x with commit cdede2e Jul 9, 2024
@shsms shsms deleted the remove-req-timeout branch July 9, 2024 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

part:actor Affects an actor ot the actors utilities (decorator, etc.) part:data-pipeline Affects the data pipeline part:docs Affects the documentation part:microgrid Affects the interactions with the microgrid part:tests Affects the unit, integration and performance (benchmarks) tests

Projects

Development

Successfully merging this pull request may close these issues.

3 participants