-
Notifications
You must be signed in to change notification settings - Fork 4
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!: Make the eventing provider specific instead of being singleton #75
Conversation
e828a5a
to
69379d2
Compare
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.
Overall, I think there is an opportunity to take inspiration from the Java SDK setup (which should also adhere very well to the Specs): https://github.com/open-feature/java-sdk/tree/main
OpenFeature/src/main/java/dev/openfeature/sdk/FeatureProvider.kt
Outdated
Show resolved
Hide resolved
OpenFeature/src/main/java/dev/openfeature/sdk/async/Extensions.kt
Outdated
Show resolved
Hide resolved
OpenFeature/src/main/java/dev/openfeature/sdk/async/Extensions.kt
Outdated
Show resolved
Hide resolved
OpenFeature/src/main/java/dev/openfeature/sdk/FeatureProvider.kt
Outdated
Show resolved
Hide resolved
OpenFeature/src/main/java/dev/openfeature/sdk/events/EventHandler.kt
Outdated
Show resolved
Hide resolved
OpenFeature/src/main/java/dev/openfeature/sdk/FeatureProvider.kt
Outdated
Show resolved
Hide resolved
OpenFeature/src/main/java/dev/openfeature/sdk/events/EventHandler.kt
Outdated
Show resolved
Hide resolved
6292faa
to
2d3d26c
Compare
dispatcher: CoroutineDispatcher = Dispatchers.IO | ||
) { | ||
val provider = getProvider() | ||
requireNotNull(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.
Let's remove this too. I guess this function would just return right away if no provider is set?
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.
Hm... If no provider is set then getProvider()
returns null
and I assume requireNotNull
will throw an exception. This is not great IMO.
Ideally this suspending function would just hang until a provider is set on OpenFeatureAPI
and become ready. However I don't know how to solve that. Ideas?
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.
Is it completely crazy to setup a flow
where we continously poll getProvider()
until one becomes available?
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 thought about polling .. but isn't it too much maybe?
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 5 options :
1- not have this function at all.
2- never return from this suspending if no provider set (bad because you won't know why you don't get out of the await function)
3- exception with correct message so you know you ask for this function early
4- poll until provider is set
5- return if provider is null (bad because returning implies the provider is ready which is not in this case)
IMO, we need to choose between option 4 or 3. I chose the option 3 but if that's violating the not throwing exception rule. I can try the polling thing.
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 may be missing some of the context here but would it be possible to return he no-op 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.
@beeme1mr we updated the extension to be like following instead of waiting for provider in a different line and we are adding the setProviderAndAwaitReady
.
suspend fun OpenFeatureAPI.setProviderAndAwaitReady(
provider: FeatureProvider,
dispatcher: CoroutineDispatcher
) {
OpenFeatureAPI.setProvider(provider)
provider.awaitReady(dispatcher)
}
} | ||
|
||
interface EventObserver { | ||
fun <T : OpenFeatureEvents> observe(kClass: KClass<T>): Flow<T> | ||
interface ProviderStatus { |
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.
nit: I would expect an interface such as this to have a getStatus(): Status
function.
6cdcbdf
to
8a4414e
Compare
Signed-off-by: vahid torkaman <[email protected]>
8a4414e
to
5a99cc3
Compare
Signed-off-by: vahid torkaman <[email protected]>
5a99cc3
to
cd34e5e
Compare
import dev.openfeature.sdk.events.OpenFeatureEvents | ||
import dev.openfeature.sdk.events.observe | ||
import kotlinx.coroutines.CoroutineDispatcher | ||
import kotlinx.coroutines.CoroutineScope | ||
import kotlinx.coroutines.Dispatchers | ||
import kotlinx.coroutines.cancel | ||
import kotlinx.coroutines.flow.Flow | ||
import kotlinx.coroutines.flow.onStart | ||
import kotlinx.coroutines.flow.take | ||
import kotlinx.coroutines.launch | ||
import kotlinx.coroutines.suspendCancellableCoroutine | ||
|
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.
Need to put some proper effort in documenting the API's in this file but I think this can go in a separate PR.
import dev.openfeature.sdk.events.OpenFeatureEvents | ||
import kotlinx.coroutines.CoroutineDispatcher | ||
|
||
class TestFeatureProvider( |
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 this be the in-memory provider? We started adding those in to other SDKs. It can be used in tests or for basic use cases.
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.
In most SDKs there's a testing provider (or a few) that have more testing features related to testing the SDK itself (such as throwing on init to test init logic, etc). That seems more like what this is.
The in-memory provider is mostly for outsiders to test things. I think that's a separate issue.
import dev.openfeature.sdk.exceptions.OpenFeatureError.FlagNotFoundError | ||
import kotlinx.coroutines.flow.Flow | ||
import kotlinx.coroutines.flow.flow |
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 imported twice.
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.
will fix in the next 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'm not much of a Kotlin dev, so I could be missing something, but this looks right to me.
I think this will make implementing named providers
more simple.
This PR