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

chore: timeouts on version discovery #4476

Merged
merged 1 commit into from
Mar 7, 2024
Merged

Conversation

dpc
Copy link
Contributor

@dpc dpc commented Mar 6, 2024

When working locally I've noticed test failing due to too short api discovery timeout.

Parametrize timeout of api version discovery using env variable. Set it to low value only when testing offline nodes.

Parametrize timeout of api version discovery using env variable.
Set it to low value only when testing offline nodes.
@dpc dpc requested review from a team as code owners March 6, 2024 08:06
@dpc dpc enabled auto-merge March 6, 2024 08:18
Copy link
Member

@maan2003 maan2003 left a comment

Choose a reason for hiding this comment

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

fedimint-client should not use environment variables.
libraries using env variables is super weird.
we should move env variables checking to fedimint-cli

@elsirion
Copy link
Contributor

elsirion commented Mar 6, 2024

The problem with that is getting the setting deep into the client. Eventually we'll need a general solution for things like this to also override the default esplora server etc., but for now this seems like the only reasonable-effort way to adjust the timeout in tests without breaking clients on slow connections.

@dpc
Copy link
Contributor Author

dpc commented Mar 6, 2024

fedimint-client should not use environment variables.
libraries using env variables is super weird.
we should move env variables checking to fedimint-cli

I agree, but it isn't a big crime and that can be a follow-up. We'll have some builder pattern on Client where we can set this value, and read it from variable in fedimint-cli. But this PR is better than hardcoding a super-short timeout, which will break production cases, and all in all it isn't too bad and seems perfectly landable. @maan2003

Copy link
Member

@bradleystachurski bradleystachurski left a comment

Choose a reason for hiding this comment

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

@dpc Thanks for tackling this. I think we can close #4398, but feel free to keep open if you want to refactor the misdemeanor in a followup.

@maan2003 I share your discomfort with using env vars to trigger different logic in prod vs tests. I'm approving since I prefer the tradeoff to fix the broken behavior in prod first, which can be refactored in a fast follow.

@dpc dpc added this pull request to the merge queue Mar 7, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 7, 2024
@bradleystachurski bradleystachurski added this pull request to the merge queue Mar 7, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 7, 2024
@dpc dpc added this pull request to the merge queue Mar 7, 2024
@dpc
Copy link
Contributor Author

dpc commented Mar 7, 2024

copying path '/nix/store/mx1mwi6bfzjsblhw1m95qs1zwb0l382c-ndk-bundle-23.0.7344513-rc4' from 'https://fedimint.cachix.org/'...
Error: The operation was canceled.

@bradleystachurski Seems like the nix stale timeout you landed recently also didn't help? :(

https://github.com/fedimint/fedimint/actions/runs/8181905886/job/22372424769

It's always the ndk-bundle (which is probably relatively large), so maybe it's not completely stuck but just sometimes get very slow?

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 7, 2024
@bradleystachurski
Copy link
Member

bradleystachurski commented Mar 7, 2024

@dpc Should we try temporarily bumping the cross compile timeout?

@dpc dpc added this pull request to the merge queue Mar 7, 2024
@dpc
Copy link
Contributor Author

dpc commented Mar 7, 2024

@dpc Should we try temporarily bumping the cross compile timeout?

Worth giving a shot, I guess. Can you make a PR?

Merged via the queue into fedimint:master with commit 3599ce3 Mar 7, 2024
20 checks passed
@dpc dpc deleted the 24-03-06-timeouts branch March 7, 2024 04:37
@bradleystachurski
Copy link
Member

#4487

dpc added a commit to dpc/fedimint that referenced this pull request Mar 8, 2024
We started getting slow test and timeouts in the CI:

https://github.com/fedimint/fedimint/actions/runs/8198894922/job/22423145415

caused by fedimint#4476.

Turns out there are other tests just degraded federation tests that
run with degraded federations.

Generally in dev shell, tests and CI, we can just default to 10 seconds
timeout, should be enough even under heavy load.
dpc added a commit to dpc/fedimint that referenced this pull request Mar 8, 2024
We started getting slow test and timeouts in the CI:

https://github.com/fedimint/fedimint/actions/runs/8198894922/job/22423145415

caused by fedimint#4476.

Turns out there are other tests just degraded federation tests that
run with degraded federations.

Generally in dev shell, tests and CI, we can just default to 10 seconds
timeout, should be enough even under heavy load.
isaacknjama pushed a commit to isaacknjama/fedimint that referenced this pull request Mar 12, 2024
We started getting slow test and timeouts in the CI:

https://github.com/fedimint/fedimint/actions/runs/8198894922/job/22423145415

caused by fedimint#4476.

Turns out there are other tests just degraded federation tests that
run with degraded federations.

Generally in dev shell, tests and CI, we can just default to 10 seconds
timeout, should be enough even under heavy load.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants