-
Notifications
You must be signed in to change notification settings - Fork 1
[Draft] Android api 21 support #109
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?
Changes from all commits
79f4c28
ba28f26
792a35c
89e33ad
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 |
---|---|---|
|
@@ -6,6 +6,12 @@ | |
|
||
import cloud.eppo.api.*; | ||
import cloud.eppo.cache.AssignmentCacheEntry; | ||
import cloud.eppo.json.JacksonMapper; | ||
import cloud.eppo.json.JacksonMapperNode; | ||
import cloud.eppo.json.Mapper; | ||
import cloud.eppo.json.MapperException; | ||
import cloud.eppo.json.MapperJsonProcessingException; | ||
import cloud.eppo.json.MapperNode; | ||
import cloud.eppo.logging.Assignment; | ||
import cloud.eppo.logging.AssignmentLogger; | ||
import cloud.eppo.logging.BanditAssignment; | ||
|
@@ -27,9 +33,7 @@ | |
|
||
public class BaseEppoClient { | ||
private static final Logger log = LoggerFactory.getLogger(BaseEppoClient.class); | ||
private final ObjectMapper mapper = | ||
new ObjectMapper() | ||
.registerModule(EppoModule.eppoModule()); // TODO: is this the best place for this? | ||
private final Mapper mapper; | ||
|
||
protected final ConfigurationRequestor requestor; | ||
|
||
|
@@ -73,6 +77,46 @@ protected BaseEppoClient( | |
@Nullable CompletableFuture<Configuration> initialConfiguration, | ||
@Nullable IAssignmentCache assignmentCache, | ||
@Nullable IAssignmentCache banditAssignmentCache) { | ||
this( | ||
new JacksonMapper(), | ||
apiKey, | ||
sdkName, | ||
sdkVersion, | ||
host, | ||
apiBaseUrl, | ||
assignmentLogger, | ||
banditLogger, | ||
configurationStore, | ||
isGracefulMode, | ||
expectObfuscatedConfig, | ||
supportBandits, | ||
initialConfiguration, | ||
assignmentCache, | ||
banditAssignmentCache | ||
); | ||
} | ||
|
||
// It is important that the bandit assignment cache expire with a short-enough TTL to last about | ||
// one user session. | ||
// The recommended is 10 minutes (per @Sven) | ||
/** @param host To be removed in v4. use `apiBaseUrl` instead. */ | ||
protected BaseEppoClient( | ||
@NotNull Mapper mapper, | ||
@NotNull String apiKey, | ||
@NotNull String sdkName, | ||
@NotNull String sdkVersion, | ||
@Deprecated @Nullable String host, | ||
@Nullable String apiBaseUrl, | ||
@Nullable AssignmentLogger assignmentLogger, | ||
@Nullable BanditLogger banditLogger, | ||
@Nullable IConfigurationStore configurationStore, | ||
boolean isGracefulMode, | ||
boolean expectObfuscatedConfig, | ||
boolean supportBandits, | ||
@Nullable CompletableFuture<Configuration> initialConfiguration, | ||
@Nullable IAssignmentCache assignmentCache, | ||
@Nullable IAssignmentCache banditAssignmentCache) { | ||
this.mapper = mapper; | ||
|
||
if (apiKey == null) { | ||
throw new IllegalArgumentException("Unable to initialize Eppo SDK due to missing API key"); | ||
|
@@ -436,7 +480,12 @@ public JsonNode getJSONAssignment( | |
subjectAttributes, | ||
EppoValue.valueOf(defaultValue.toString()), | ||
VariationType.JSON); | ||
return parseJsonString(value.stringValue()); | ||
MapperNode mapperNode = parseJsonString(value.stringValue()); | ||
if (mapperNode != null) { | ||
return ((JacksonMapperNode) mapperNode).getJsonNode(); | ||
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. Would we want to stay away from Jackson-specific stuff here? I vaguely recall one of the main reasons we used Jackson is because it could more easily differentiate between arrays and objects than Java's built-in JSON parsing. But if we code around that, we may be able to remove Jackson entirely. Unless it's much slower on parsing, which may matter for Android app initialization times. In which case, I think upstream-provided parser as you're doing here may be the move. 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. Since this API returns a Jackson-specific type, we must rely on Jackson in this specific method. I might create a parallel method that returns a |
||
} else { | ||
return null; | ||
} | ||
} catch (Exception e) { | ||
return throwIfNotGraceful(e, defaultValue); | ||
} | ||
|
@@ -482,10 +531,10 @@ public String getJSONStringAssignment(String flagKey, String subjectKey, String | |
return this.getJSONStringAssignment(flagKey, subjectKey, new Attributes(), defaultValue); | ||
} | ||
|
||
private JsonNode parseJsonString(String jsonString) { | ||
private MapperNode parseJsonString(String jsonString) { | ||
try { | ||
return mapper.readTree(jsonString); | ||
} catch (JsonProcessingException e) { | ||
} catch (MapperJsonProcessingException e) { | ||
return null; | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,99 @@ | ||
package cloud.eppo; | ||
|
||
import org.jetbrains.annotations.NotNull; | ||
import org.slf4j.Logger; | ||
import org.slf4j.LoggerFactory; | ||
|
||
import java.util.concurrent.atomic.AtomicBoolean; | ||
import java.util.concurrent.atomic.AtomicReference; | ||
import java.util.function.Consumer; | ||
|
||
import cloud.eppo.api.Configuration; | ||
import cloud.eppo.callback.CallbackManager; | ||
|
||
public class ConfigurationRequestorJava6 { | ||
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. To help maximize code reuse and prevent drift, what if this extends the non-java-6 flavor and then uses the template pattern, with only the language level differences overwritten in the Note: I don't know if this is possible for building. A quick chat with AI suggests its not, and what you have is the way to go 😞 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. Would we end up in callback hell if we just refactored the core to use callbacks instead of 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'd actually like to get rid of You could certainly provide wrapper APIs that expose A simple way to do this would be to provide a synchronous API like I did here, and then provide a 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 agree that moving a direction of un-opinionated (i.e. un thread-pool coupled) async/futures/callbacks is ideal in an SDK that is used across such a vast array of deployments. In your opinion, what is the "async" API that most java developers would prefer to see on the SDK? Callbacks, CompletableFutures or something else? I am eager to get your feedback here. 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 callbacks make the most sense, and then if you wanted to go for extra credit, you could publish extension libraries with RxJava2, RxJava3, Kotlin Coroutines, and CompletableFutures bindings. Modern Android developers are mostly using Kotlin Coroutines now. RxJava2 was common until about 2020, so there are probably still a lot of apps out there using it. I don't think RxJava3 is very common because it came out around the same time as Kotlin Coroutines. |
||
private static final Logger log = LoggerFactory.getLogger(ConfigurationRequestorJava6.class); | ||
private enum ConfigState { | ||
UNSET, | ||
INITIAL_SET, | ||
REMOTE_SET, | ||
; | ||
} | ||
|
||
private final EppoHttpClient client; | ||
private final IConfigurationStoreJava6 configurationStoreJava6; | ||
private final boolean expectObfuscatedConfig; | ||
private final boolean supportBandits; | ||
|
||
private final Object configStateLock = new Object(); | ||
private ConfigState configState; | ||
|
||
private final CallbackManager<Configuration> configChangeManager = new CallbackManager<>(); | ||
|
||
public ConfigurationRequestorJava6( | ||
@NotNull IConfigurationStoreJava6 configurationStoreJava6, | ||
@NotNull EppoHttpClient client, | ||
boolean expectObfuscatedConfig, | ||
boolean supportBandits) { | ||
this.configurationStoreJava6 = configurationStoreJava6; | ||
this.client = client; | ||
this.expectObfuscatedConfig = expectObfuscatedConfig; | ||
this.supportBandits = supportBandits; | ||
|
||
synchronized (configStateLock) { | ||
configState = ConfigState.UNSET; | ||
} | ||
} | ||
|
||
// Synchronously set the initial configuration. | ||
public boolean setInitialConfigurationJava6(@NotNull Configuration configuration) { | ||
synchronized (configStateLock) { | ||
switch(configState) { | ||
case UNSET: | ||
configState = ConfigState.INITIAL_SET; | ||
break; | ||
case INITIAL_SET: | ||
throw new IllegalStateException("Initial configuration has already been set"); | ||
case REMOTE_SET: | ||
return false; | ||
} | ||
} | ||
|
||
saveConfigurationAndNotifyJava6(configuration); | ||
return true; | ||
} | ||
|
||
/** Loads configuration synchronously from the API server. */ | ||
void fetchAndSaveFromRemoteJava6() { | ||
log.debug("Fetching configuration"); | ||
|
||
// Reuse the `lastConfig` as its bandits may be useful | ||
Configuration lastConfig = configurationStoreJava6.getConfiguration(); | ||
|
||
byte[] flagConfigurationJsonBytes = client.getJava6(Constants.FLAG_CONFIG_ENDPOINT); | ||
Configuration.Builder configBuilder = | ||
Configuration.builder(flagConfigurationJsonBytes, expectObfuscatedConfig) | ||
.banditParametersFromConfig(lastConfig); | ||
|
||
if (supportBandits && configBuilder.requiresUpdatedBanditModels()) { | ||
byte[] banditParametersJsonBytes = client.getJava6(Constants.BANDIT_ENDPOINT); | ||
configBuilder.banditParameters(banditParametersJsonBytes); | ||
} | ||
|
||
synchronized (configStateLock) { | ||
configState = ConfigState.REMOTE_SET; | ||
} | ||
saveConfigurationAndNotifyJava6(configBuilder.build()); | ||
} | ||
|
||
private void saveConfigurationAndNotifyJava6(Configuration configuration) { | ||
configurationStoreJava6.saveConfigurationJava6(configuration); | ||
synchronized (configChangeManager) { | ||
configChangeManager.notifyCallbacks(configuration); | ||
} | ||
} | ||
|
||
public Runnable onConfigurationChange(Consumer<Configuration> callback) { | ||
return configChangeManager.subscribe(callback); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
package cloud.eppo; | ||
|
||
import org.jetbrains.annotations.NotNull; | ||
|
||
import java.util.concurrent.CompletableFuture; | ||
|
||
import cloud.eppo.api.Configuration; | ||
|
||
/** Memory-only configuration store. */ | ||
public class ConfigurationStoreJava6 implements IConfigurationStoreJava6 { | ||
|
||
// this is the fallback value if no configuration is provided (i.e. by fetch or initial config). | ||
@NotNull private volatile Configuration configuration = Configuration.emptyConfig(); | ||
|
||
public ConfigurationStoreJava6() {} | ||
|
||
public void saveConfigurationJava6(@NotNull final Configuration configuration) { | ||
this.configuration = configuration; | ||
} | ||
|
||
@NotNull public Configuration getConfiguration() { | ||
return configuration; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
package cloud.eppo; | ||
|
||
import org.jetbrains.annotations.NotNull; | ||
|
||
import java.util.concurrent.CompletableFuture; | ||
import java.util.function.Consumer; | ||
|
||
import cloud.eppo.api.Configuration; | ||
|
||
/** | ||
* Common interface for extensions of this SDK to support caching and other strategies for | ||
* persisting configuration data across sessions. | ||
*/ | ||
public interface IConfigurationStoreJava6 { | ||
@NotNull Configuration getConfiguration(); | ||
|
||
void saveConfigurationJava6(Configuration 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 think dropping in a mutually compatible library is fine. However, we faced a similar challenge with PHP and caching, and opted to have the upstream SDK essentially pass in the library. I wonder if we should consider doing similar here, basically having this SDK take an interface that can Base64 encode/decode and then Java SDK will pass in something that uses
java.util.Base64
and Android SDK something that usesandroid.util.Base64
?Note it seems you're doing something similar for JSON mapping
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 of that too, but since this is a static method, it will require me to add a lot more indirection in the SDK. If you're ok with that level of indirection, I can add it.
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.
The added indirection is OK.
What do you think about a "compatibility" class to capture everything, like
interface EppoApiCompat {
String base64Encode(...)}
FlagConfigResponse parseUfcJson(String...)
...
// could do string joining, but the algo is simple enough to just have our own loop.
String joinStrings(String[] arrayOfStrings){...}
}
It can be an param to the
EppoClient
constructor and be passed around as needed. Test harnesses can use a default implementation based on java.util here (or android.util inandroid-sdk
). wdyt?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.
That sounds good to me. We're still investigating whether we want to modify the Eppo SDK further, so I won't make this update yet, but if we decide to press forward, I'll definitely incorporate this change.