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: allow to fail if unavailable #225

Merged
merged 5 commits into from
Nov 22, 2023
Merged

Conversation

gastonfournier
Copy link
Contributor

@gastonfournier gastonfournier commented Nov 21, 2023

About the changes

On synchronous initialization, we were not failing in case of an invalid response code but still marking the client as ready. With this change, we're making sure we have a valid features collection before marking the client as ready and we're not going to throttle in case of asynchronous initialization.

@gastonfournier gastonfournier self-assigned this Nov 21, 2023
Copy link
Member

@chriswk chriswk left a comment

Choose a reason for hiding this comment

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

Should we make exception handler configurable here?

@chriswk
Copy link
Member

chriswk commented Nov 21, 2023

I see our tests that test that we increment failure count when we fail to connect is failing.

Comment on lines -111 to -113
private Integer calculateMaxSkips(int fetchTogglesInterval) {
return Integer.max(20, 300 / Integer.max(fetchTogglesInterval, 1));
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was unused

Copy link
Member

@sighphyre sighphyre left a comment

Choose a reason for hiding this comment

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

G

@gastonfournier gastonfournier merged commit b55056d into main Nov 22, 2023
4 checks passed
@gastonfournier gastonfournier deleted the fail-on-unavailable branch November 22, 2023 09:24
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.

3 participants