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

Add polling parameters to ConfigManagingActor #1082

Merged

Conversation

daniel-zullo-frequenz
Copy link
Contributor

The ConfigManagingActor now allows to force file polling and set the polling interval.

The ConfigManagingActor now allows to force file polling
and set the polling interval.

Signed-off-by: Daniel Zullo <daniel.zullo@frequenz.com>
@github-actions github-actions bot added part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests part:tooling Affects the development tooling (CI, deployment, dependency management, etc.) part:config Affects the configuration management labels Oct 1, 2024
@daniel-zullo-frequenz daniel-zullo-frequenz marked this pull request as ready for review October 1, 2024 08:04
@daniel-zullo-frequenz daniel-zullo-frequenz requested a review from a team as a code owner October 1, 2024 08:04
@daniel-zullo-frequenz daniel-zullo-frequenz requested review from Marenz and removed request for a team October 1, 2024 08:04
Marenz
Marenz previously approved these changes Oct 1, 2024
@llucax llucax added this pull request to the merge queue Oct 3, 2024
@llucax llucax added this to the v1.0.0-rc1000 milestone Oct 3, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 3, 2024
@llucax llucax added this pull request to the merge queue Oct 3, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 3, 2024
@llucax
Copy link
Contributor

llucax commented Oct 3, 2024

Damn, cross-arch tests are consistently failing for pytest_max when adding this to the merge queue.

FAILED tests/timeseries/_battery_pool/test_battery_pool.py::test_battery_pool_power_bounds - assert 0.855949 < 0.22
 +  where 0.855949 = <built-in method total_seconds of datetime.timedelta object at 0x5512d00630>()
 +    where <built-in method total_seconds of datetime.timedelta object at 0x5512d00630> = datetime.timedelta(microseconds=855949).total_seconds

Super weird, I can't see ow this change could be related to this.

@shsms shsms added this pull request to the merge queue Oct 3, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 3, 2024
@shsms shsms added this pull request to the merge queue Oct 6, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 6, 2024
This test was failing in arm64 runs, because of a large time
difference.  This fixes the issue.

Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
@shsms
Copy link
Contributor

shsms commented Oct 6, 2024

I fixed it, in the latest commit. Here's a successful run: https://github.com/shsms/frequenz-sdk-python/actions/runs/11201255580/job/31135960350

It is too complicated to explain how this fixes it, so I won't.

@llucax
Copy link
Contributor

llucax commented Oct 7, 2024

It is too complicated to explain how this fixes it, so I won't.

This is not great 😟

I see in the commit with the fix you at least explain something. Can you maybe mention at least what do you think it triggers this issue, as it seems completely unrelated to the update to of the channels version? I wonder if there is something we can learn from here about expecting similar issues when upgrading other projects.

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.

So the changes LGTM, I would like to know more about the root cause of the new breakage, in case we bump into similar issues when upgrading to channels 1.2 in other projects, but we can merge anyways.

@shsms
Copy link
Contributor

shsms commented Oct 7, 2024

I see in the commit with the fix you at least explain something. Can you maybe mention at least what do you think it triggers this issue, as it seems completely unrelated to the update to of the channels version? I wonder if there is something we can learn from here about expecting similar issues when upgrading other projects.

I actually have no idea. The commit reduces the delay by 0.1 seconds, so technically it couldn't have made up for the 0.6s lag that we were seeing in the failing test. I think it is just software emulated VM flakiness.

@shsms shsms added this pull request to the merge queue Oct 7, 2024
@shsms
Copy link
Contributor

shsms commented Oct 7, 2024

It is too complicated to explain how this fixes it, so I won't.

That was actually a joke, I guess it wasn't obvious. :D

Merged via the queue into frequenz-floss:v1.x.x with commit 8eabc21 Oct 7, 2024
18 checks passed
@llucax
Copy link
Contributor

llucax commented Oct 7, 2024

That was actually a joke, I guess it wasn't obvious. :D

No, I'm too scared of this kind of failure/flakiness, I guess my jokes detector doesn't perform well in these kind of extreme conditions :P

@daniel-zullo-frequenz daniel-zullo-frequenz deleted the feature/file-polling branch October 21, 2024 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
part:config Affects the configuration management part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests part:tooling Affects the development tooling (CI, deployment, dependency management, etc.)
Projects
Development

Successfully merging this pull request may close these issues.

4 participants