-
Notifications
You must be signed in to change notification settings - Fork 0
Conversation
bc412cf
to
55cf607
Compare
OpenFeature/src/main/java/dev/openfeature/sdk/events/EventPublisher.kt
Outdated
Show resolved
Hide resolved
b909c14
to
35f4e8c
Compare
… to the async client
35f4e8c
to
564461e
Compare
97af885
to
027939d
Compare
027939d
to
2dc402e
Compare
…xt set and initialize for setting provider
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still unfamiliar with the details of Flow
, but added some generic comments
internal class AsyncClientImpl( | ||
private val client: OpenFeatureClient | ||
) : AsyncClient { | ||
private fun <T> observeEvents(callback: () -> T) = observeProviderReady() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have a "observeProviderState" that listens to any Provider State event?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have 2 functions one observeProviderReady
and another one a suspending function awaitProviderReady()
. added to the description of the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i removed the secondary way and now we have one way as helper functions to listen for providerReady
CoroutineScope(Dispatchers.IO).launch {
awaitProviderReady()
// now provider is ready, read the properties
}
@@ -0,0 +1,9 @@ | |||
package dev.openfeature.sdk.events | |||
|
|||
sealed class OpenFeatureEvents { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we name this to something "state" specific, e.g. ProviderStateEvents
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i would argue this can be used to any sort of event from the provider and IMO it's not always about the provider state.
return AsyncClientImpl(this) | ||
} | ||
|
||
fun observeProviderReady() = EventHandler.eventsObserver() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar comment above: let's extend this to support any ProviderStateEvent type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is just a helper function for provider ready. the design already allows listening for all the events like:
EventHandler.eventsObserver()
.observe<OpenFeatureEvents>()
.collect {
// do something with the event
}
@@ -5,10 +5,14 @@ interface FeatureProvider { | |||
val metadata: ProviderMetadata | |||
|
|||
// Called by OpenFeatureAPI whenever the new Provider is registered | |||
suspend fun initialize(initialContext: EvaluationContext?) | |||
fun initialize(initialContext: EvaluationContext?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should keep "suspend" in the interface towards the Provider, and only remove "suspend" from the OpenFeature user-facing API setProvider
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think it makes sense that now we have the shutdown function we move the responsibility of having scopes and suspending in the provider side. but we can further discuss this.
is OpenFeatureEvents.ProviderReady -> isProviderReady.value = true | ||
is OpenFeatureEvents.ProviderShutDown -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder is the provider is not ready after ProviderConfigurationChanged
as well, as I think that's when a new EvaluationContext is set and the Provider starts fetching the new values from backend
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
like ShutDownCompleted
? i believe the signal of shutdown is coming from the Provider and it's signaling that the provider is shut down.
Would be great to curate the PR description a bit more, since this is probably the place where all reviews will happen.
|
f48ded0
to
b314889
Compare
b314889
to
7031488
Compare
To do some actions after provider is ready:
The usage will be something like:
in the compose world will be:
we can update the implementation to limit the value emissions only for when the provider is initialised and ready to not emit the default value if the provider is not ready.