-
Notifications
You must be signed in to change notification settings - Fork 3
feat: add oneShot parameter for initialization configuration change listener #233
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
base: main
Are you sure you want to change the base?
Conversation
Add ability to auto-unsubscribe from configuration changes after first
invocation by adding an optional oneShot boolean parameter to the
Builder's onConfigurationChange() method.
When oneShot=true, the callback is automatically unsubscribed after
the first configuration change, which is useful for initialization
scenarios where you only want to be notified once.
The implementation uses an OneShotConfigCallback wrapper class that
delegates to the user's callback and then executes the unsubscribe
Runnable returned by BaseEppoClient.onConfigurationChange(). The
wrapping happens immediately in the Builder method, keeping the logic
clean and avoiding the need to store state about oneShot mode.
Fixes FFL-1683
aarsilv
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.
Makes sense as a starting point to have the config change be one-time, auto-unsubscribed
dd-oleksii
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.
Shall we just add an unsubscribe to BaseEppoClient, so we don't need one-shot special handling?
| delegate.accept(config); | ||
| } finally { | ||
| if (unsubscriber != null) { | ||
| unsubscriber.run(); |
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.
minor: shall we unsubscribe before running accept on delegate? (could matter in edge cases when accept() is running for too long and we receive another configuration?)
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 was actually thinking the same 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.
good catch here. The base callbackmanager doesn't have any locking 🫠
|
Eppo Internal:
🎟️ Fixes FFL-1683
Motivation and Context
Previously, when registering a configuration change callback via the Builder's
onConfigurationChange()method, there was no way to unsubscribe from these notifications. The callback would continue to fire for every configuration change (initialization, manualloadConfiguration()calls, and polling updates) for the lifetime of the EppoClient instance.This is problematic for initialization scenarios where developers only want to be notified once when the initial configuration is loaded, not on subsequent updates. Without an unsubscribe mechanism, developers would receive unwanted callbacks during polling or manual reloads.
While
BaseEppoClient.onConfigurationChange()already returns an unsubscribeRunnable, the Android SDK wrapper wasn't capturing or exposing this functionality, making it impossible to unsubscribe from the Builder-registered callback.Because we are building the Android client with a
Builder, we don't have a clean vector to return the unsubscribe runnable without breaking the Builder convention or introducing a breaking change to the API. In addition to this change, we can add anunsubscribe(listener)to the BaseEppoClient to allow unsubscribing.Description
This PR adds an optional
oneShotboolean parameter to the Builder'sonConfigurationChange()method. WhenoneShot=true, the callback automatically unsubscribes itself after the first invocation, making it perfect for initialization scenarios.Implementation Details
OneShotConfigCallback Wrapper Class (
EppoClient.java:160-185):Builder Method Overload (
EppoClient.java:291-302):Initialization Logic (
EppoClient.java:354-360):How has this been documented?
Code Documentation
TODO: Customer Comms
How has this been tested?
New Tesst