-
Notifications
You must be signed in to change notification settings - Fork 7
feat!: Make the eventing provider specific instead of being singleton #75
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
Conversation
e828a5a to
69379d2
Compare
fabriziodemaria
left a comment
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 <vahidlazio@gmail.com>
8a4414e to
5a99cc3
Compare
Signed-off-by: vahid torkaman <vahidlazio@gmail.com>
5a99cc3 to
cd34e5e
Compare
| 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.events.OpenFeatureEvents | ||
| 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.
toddbaert
left a comment
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