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

feat: Initialisation strategy #58

Merged
merged 5 commits into from
Sep 6, 2023
Merged

feat: Initialisation strategy #58

merged 5 commits into from
Sep 6, 2023

Conversation

vahidlazio
Copy link
Contributor

  • Add the option to choose an initialisation strategy
  • ActivateAndFetchAsync or FetchAndActivate are the different strategies that can be selected in the initialisation phase

@vahidlazio vahidlazio changed the title Initialisation strategy feat: Initialisation strategy Sep 4, 2023
@vahidlazio vahidlazio marked this pull request as ready for review September 4, 2023 20:26
@@ -54,14 +58,43 @@ class ConfidenceFeatureProvider private constructor(
eventsPublisher.publish(OpenFeatureEvents.ProviderReady)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should to send ProviderError if the cache is empty at this point? Or ProviderStale?

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 exception happens when the network request has failed. especially when the initialisation strategy is activate and fetch IMO it doesn't make sense to have provider not ready. for fetch and activate, maybe .. but is it what we want? considering that if the developer decides to have the strategy as "try to refresh first" or "force refresh first"? in the latter case they can't use the properties since the provider is never ready. they will be stuck. but if the definition of FetchAndActivate is try to fetch, then the failure in the network request should be treated as ready? we have to add this to the documentation anyways.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, with ActivateAndFetch, the provider should be ready, since the developer would most likely apply the flags that were fetched later in the lifecycle / next session.
For fetchAndActivate - would we then handle an error as "nothing new to fetch" and wait for another opportunity to fetch freshest flags? In that case, I also agree that it can be a ProviderReady. My only concern (which might come from me not fully understanding Confidence/data science) is whether we report these events correctly.

@vahidlazio vahidlazio force-pushed the initialisation-strategy branch 3 times, most recently from d27221f to 4c9ff2f Compare September 6, 2023 14:13
@vahidlazio vahidlazio merged commit 5b3e011 into main Sep 6, 2023
1 check passed
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