-
Notifications
You must be signed in to change notification settings - Fork 38
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 provider interface "stateless"; SDK maintains provider state #1096
Changes from 6 commits
f2e9097
d2b15fe
16f57e8
b993ee2
625a404
98c620f
d0a2027
11a0ccb
56e66a5
7245963
b90f393
2a2184b
7d89d52
e3c1d30
4bc6f66
8bd726d
37326d9
b0d66a5
6bdd7ac
124be36
8ca0d2d
53dae87
e4ea124
28130ba
975d3ad
01538c3
ca8fc16
cd56f25
45e9ba2
5816732
6d199d9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,7 @@ | ||
package dev.openfeature.sdk; | ||
|
||
import dev.openfeature.sdk.exceptions.GeneralError; | ||
import dev.openfeature.sdk.exceptions.OpenFeatureError; | ||
import dev.openfeature.sdk.internal.TriConsumer; | ||
|
||
/** | ||
|
@@ -16,21 +18,56 @@ | |
*/ | ||
public abstract class EventProvider implements FeatureProvider { | ||
|
||
private ProviderState providerState = ProviderState.NOT_READY; | ||
|
||
/** | ||
* {@inheritDoc} | ||
*/ | ||
@Override | ||
public abstract ProviderState getState(); | ||
public final ProviderState getState() { | ||
return providerState; | ||
} | ||
|
||
@Override | ||
public final void initialize(EvaluationContext evaluationContext) throws Exception { | ||
try { | ||
doInitialization(evaluationContext); | ||
providerState = ProviderState.READY; | ||
} catch (OpenFeatureError openFeatureError) { | ||
if (ErrorCode.PROVIDER_FATAL.equals(openFeatureError.getErrorCode())) { | ||
providerState = ProviderState.FATAL; | ||
} else { | ||
providerState = ProviderState.ERROR; | ||
} | ||
throw openFeatureError; | ||
} catch (Exception e) { | ||
providerState = ProviderState.ERROR; | ||
throw new GeneralError(e); | ||
} | ||
} | ||
|
||
protected void doInitialization(EvaluationContext evaluationContext) throws Exception { | ||
// Intentionally left blank, to be implemented by inheritors | ||
} | ||
|
||
@Override | ||
public final void shutdown() { | ||
providerState = ProviderState.NOT_READY; | ||
doShutdown(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why this split is needed ? |
||
} | ||
|
||
protected void doShutdown() { | ||
// Intentionally left blank, to be implemented by inheritors | ||
} | ||
|
||
private TriConsumer<EventProvider, ProviderEvent, ProviderEventDetails> onEmit = null; | ||
|
||
/** | ||
* "Attach" this EventProvider to an SDK, which allows events to propagate from this provider. | ||
* No-op if the same onEmit is already attached. | ||
* No-op if the same onEmit is already attached. | ||
* | ||
* @param onEmit the function to run when a provider emits events. | ||
* @throws IllegalStateException if attempted to bind a new emitter for already bound provider | ||
* | ||
*/ | ||
void attach(TriConsumer<EventProvider, ProviderEvent, ProviderEventDetails> onEmit) { | ||
if (this.onEmit != null && this.onEmit != onEmit) { | ||
|
@@ -50,11 +87,18 @@ void detach() { | |
|
||
/** | ||
* Emit the specified {@link ProviderEvent}. | ||
* | ||
* | ||
* @param event The event type | ||
* @param details The details of the event | ||
*/ | ||
public void emit(ProviderEvent event, ProviderEventDetails details) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unrelated to this PR, but I'm wondering if public visibility of emit methods is too generous. Is there a use-case where anyone outside the provider itself would want to emit a provider state change directly? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is a good point, and we can probably reduce this visibility without considering this a breaking change since events are still labeled "experimental" so that would break our stability guarantee. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @chrfwow did this but I had to revert it. We had multiple providers in the contribs who used an encapsulation strategy and used this publicly instead of in a subclass. |
||
if (ProviderEvent.PROVIDER_ERROR.equals(event)) { | ||
providerState = ProviderState.ERROR; | ||
} else if (ProviderEvent.PROVIDER_STALE.equals(event)) { | ||
providerState = ProviderState.STALE; | ||
} else if (ProviderEvent.PROVIDER_READY.equals(event)) { | ||
providerState = ProviderState.READY; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add a specific test for this? It relates to specification requirement 5.3.5, so you can use our |
||
if (this.onEmit != null) { | ||
this.onEmit.accept(this, event, details); | ||
} | ||
|
@@ -63,7 +107,7 @@ public void emit(ProviderEvent event, ProviderEventDetails details) { | |
/** | ||
* Emit a {@link ProviderEvent#PROVIDER_READY} event. | ||
* Shorthand for {@link #emit(ProviderEvent, ProviderEventDetails)} | ||
* | ||
* | ||
* @param details The details of the event | ||
*/ | ||
public void emitProviderReady(ProviderEventDetails details) { | ||
|
@@ -74,7 +118,7 @@ public void emitProviderReady(ProviderEventDetails details) { | |
* Emit a | ||
* {@link ProviderEvent#PROVIDER_CONFIGURATION_CHANGED} | ||
* event. Shorthand for {@link #emit(ProviderEvent, ProviderEventDetails)} | ||
* | ||
* | ||
* @param details The details of the event | ||
*/ | ||
public void emitProviderConfigurationChanged(ProviderEventDetails details) { | ||
|
@@ -84,7 +128,7 @@ public void emitProviderConfigurationChanged(ProviderEventDetails details) { | |
/** | ||
* Emit a {@link ProviderEvent#PROVIDER_STALE} event. | ||
* Shorthand for {@link #emit(ProviderEvent, ProviderEventDetails)} | ||
* | ||
* | ||
* @param details The details of the event | ||
*/ | ||
public void emitProviderStale(ProviderEventDetails details) { | ||
|
@@ -94,7 +138,7 @@ public void emitProviderStale(ProviderEventDetails details) { | |
/** | ||
* Emit a {@link ProviderEvent#PROVIDER_ERROR} event. | ||
* Shorthand for {@link #emit(ProviderEvent, ProviderEventDetails)} | ||
* | ||
* | ||
* @param details The details of the event | ||
*/ | ||
public void emitProviderError(ProviderEventDetails details) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should there also be a method like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would take the approach of just checking if the error is a Fatal error. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How can I do that? The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. I think that's another thing we need to add. I might have missed it in the issue, but it's in the spec: https://openfeature.dev/specification/types#event-details There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it queried like this?
I can't see how this should work from the documentation There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm saying the event details should have a first class member representing the error code (not in the event data), if it's an error event. |
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,11 +5,7 @@ | |
import dev.openfeature.sdk.internal.AutoCloseableReentrantReadWriteLock; | ||
import lombok.extern.slf4j.Slf4j; | ||
|
||
import java.util.ArrayList; | ||
import java.util.Arrays; | ||
import java.util.List; | ||
import java.util.Optional; | ||
import java.util.Set; | ||
import java.util.*; | ||
import java.util.function.Consumer; | ||
|
||
/** | ||
|
@@ -86,7 +82,7 @@ public Client getClient() { | |
* Multiple clients can be used to segment feature flag configuration. | ||
* If there is already a provider bound to this domain, this provider will be used. | ||
* Otherwise, the default provider is used until a provider is assigned to that domain. | ||
* | ||
* | ||
* @param domain an identifier which logically binds clients with providers | ||
* @return a new client instance | ||
*/ | ||
|
@@ -100,15 +96,18 @@ public Client getClient(String domain) { | |
* Multiple clients can be used to segment feature flag configuration. | ||
* If there is already a provider bound to this domain, this provider will be used. | ||
* Otherwise, the default provider is used until a provider is assigned to that domain. | ||
* | ||
* @param domain a identifier which logically binds clients with providers | ||
* | ||
* @param domain a identifier which logically binds clients with providers | ||
* @param version a version identifier | ||
* @return a new client instance | ||
*/ | ||
public Client getClient(String domain, String version) { | ||
return new OpenFeatureClient(this, | ||
return new OpenFeatureClient( | ||
() -> providerRepository.getProvider(domain), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Inside the |
||
this, | ||
domain, | ||
version); | ||
version | ||
); | ||
} | ||
|
||
/** | ||
|
@@ -193,8 +192,8 @@ public void setProvider(FeatureProvider provider) { | |
/** | ||
* Add a provider for a domain. | ||
* | ||
* @param domain The domain to bind the provider to. | ||
* @param provider The provider to set. | ||
* @param domain The domain to bind the provider to. | ||
* @param provider The provider to set. | ||
*/ | ||
public void setProvider(String domain, FeatureProvider provider) { | ||
try (AutoCloseableLock __ = lock.writeLockAutoCloseable()) { | ||
|
@@ -226,8 +225,8 @@ public void setProviderAndWait(FeatureProvider provider) throws OpenFeatureError | |
/** | ||
* Add a provider for a domain and wait for initialization to finish. | ||
* | ||
* @param domain The domain to bind the provider to. | ||
* @param provider The provider to set. | ||
* @param domain The domain to bind the provider to. | ||
* @param provider The provider to set. | ||
*/ | ||
public void setProviderAndWait(String domain, FeatureProvider provider) throws OpenFeatureError { | ||
try (AutoCloseableLock __ = lock.writeLockAutoCloseable()) { | ||
|
@@ -300,6 +299,7 @@ public void addHooks(Hook... hooks) { | |
|
||
/** | ||
* Fetch the hooks associated to this client. | ||
* | ||
* @return A list of {@link Hook}s. | ||
*/ | ||
public List<Hook> getHooks() { | ||
|
@@ -404,7 +404,7 @@ void addHandler(String domain, ProviderEvent event, Consumer<EventDetails> handl | |
|
||
/** | ||
* Runs the handlers associated with a particular provider. | ||
* | ||
* | ||
* @param provider the provider from where this event originated | ||
* @param event the event type | ||
* @param details the event details | ||
|
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 implementation is logically correct, but this won't work retroactively with existing implementations (many of which can be found here, though there's others maintained by vendors, etc).
We need to do this in a non-breaking way, without breaking any existing contracts or behvaiors. In C#, I accomplished this by creating a new package-private ProviderState field on the provider which the SDK updates after it has successfully (or unsuccessfully) run the
initialize
method, and basically ignoring the previous provider state field.This allows existing providers to work without breaking changes.
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.
@toddbaert I wonder if there can be some additional automated step to see if the sdk-contrib repository providers are not breaking, possibly as warning / non-mandatory. There is the InMemoryProvider on this repository which is a good sanity, maybe checking the other providers as well can help. Or is too much? What do you think ?