-
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 26 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 |
---|---|---|
|
@@ -2,6 +2,7 @@ | |
|
||
import dev.openfeature.sdk.internal.TriConsumer; | ||
|
||
|
||
/** | ||
* Abstract EventProvider. Providers must extend this class to support events. | ||
* Emit events with {@link #emit(ProviderEvent, ProviderEventDetails)}. Please | ||
|
@@ -15,22 +16,20 @@ | |
* @see FeatureProvider | ||
*/ | ||
public abstract class EventProvider implements FeatureProvider { | ||
private EventProviderListener eventProviderListener; | ||
|
||
/** | ||
* {@inheritDoc} | ||
*/ | ||
@Override | ||
public abstract ProviderState getState(); | ||
void setEventProviderListener(EventProviderListener eventProviderListener) { | ||
this.eventProviderListener = eventProviderListener; | ||
} | ||
|
||
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 +49,14 @@ 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) { | ||
if (eventProviderListener != null) { | ||
eventProviderListener.onEmit(event, details); | ||
} | ||
if (this.onEmit != null) { | ||
this.onEmit.accept(this, event, details); | ||
} | ||
|
@@ -63,7 +65,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 +76,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 +86,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 +96,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 |
---|---|---|
@@ -0,0 +1,6 @@ | ||
package dev.openfeature.sdk; | ||
|
||
@FunctionalInterface | ||
interface EventProviderListener { | ||
void onEmit(ProviderEvent event, ProviderEventDetails details); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,82 @@ | ||
package dev.openfeature.sdk; | ||
|
||
import dev.openfeature.sdk.exceptions.OpenFeatureError; | ||
import lombok.Getter; | ||
import lombok.experimental.Delegate; | ||
|
||
import java.util.concurrent.atomic.AtomicBoolean; | ||
|
||
class FeatureProviderStateManager implements EventProviderListener { | ||
|
||
private interface ExcludeFromDelegate { | ||
void initialize(EvaluationContext evaluationContext) throws Exception; | ||
|
||
void shutdown(); | ||
|
||
ProviderState getState(); | ||
} | ||
|
||
@Delegate(excludes = ExcludeFromDelegate.class) | ||
toddbaert marked this conversation as resolved.
Show resolved
Hide resolved
|
||
private final FeatureProvider delegate; | ||
private final AtomicBoolean isInitialized = new AtomicBoolean(); | ||
@Getter | ||
private ProviderState state = ProviderState.NOT_READY; | ||
|
||
public FeatureProviderStateManager(FeatureProvider delegate) { | ||
this.delegate = delegate; | ||
if (delegate instanceof EventProvider) { | ||
((EventProvider) delegate).setEventProviderListener(this); | ||
} | ||
} | ||
|
||
public void initialize(EvaluationContext evaluationContext) throws Exception { | ||
if (isInitialized.getAndSet(true)) { | ||
return; | ||
} | ||
try { | ||
delegate.initialize(evaluationContext); | ||
state = ProviderState.READY; | ||
} catch (OpenFeatureError openFeatureError) { | ||
if (ErrorCode.PROVIDER_FATAL.equals(openFeatureError.getErrorCode())) { | ||
state = ProviderState.FATAL; | ||
} else { | ||
state = ProviderState.ERROR; | ||
} | ||
isInitialized.set(false); | ||
throw openFeatureError; | ||
} catch (Exception e) { | ||
state = ProviderState.ERROR; | ||
isInitialized.set(false); | ||
throw e; | ||
} | ||
} | ||
|
||
public void shutdown() { | ||
delegate.shutdown(); | ||
state = ProviderState.NOT_READY; | ||
isInitialized.set(false); | ||
} | ||
|
||
@Override | ||
public void onEmit(ProviderEvent event, ProviderEventDetails details) { | ||
if (ProviderEvent.PROVIDER_ERROR.equals(event)) { | ||
if (details != null && details.getErrorCode() == ErrorCode.PROVIDER_FATAL) { | ||
state = ProviderState.FATAL; | ||
} else { | ||
state = ProviderState.ERROR; | ||
} | ||
} else if (ProviderEvent.PROVIDER_STALE.equals(event)) { | ||
state = ProviderState.STALE; | ||
} else if (ProviderEvent.PROVIDER_READY.equals(event)) { | ||
state = ProviderState.READY; | ||
} | ||
} | ||
|
||
FeatureProvider getProvider() { | ||
return delegate; | ||
} | ||
|
||
public boolean hasSameProvider(FeatureProvider featureProvider) { | ||
return this.delegate.equals(featureProvider); | ||
} | ||
} |
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.
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 comment
The 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 comment
The 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.