Conversation
Add new android-sdk-framework module that provides base client functionality and configuration storage abstractions for Android SDK implementations. Key components: - AndroidBaseClient: Base class for Android SDK clients - CachingConfigurationStore: Abstract store for cached configurations - FileBackedConfigStore: File-based configuration persistence - ConfigurationCodec: Pluggable serialization/deserialization - ByteStore: Low-level byte storage abstraction This module uses the v4 Java SDK framework and is added as a standalone module without integration into the existing eppo module yet.
The v4 framework snapshots are deployed to the new Maven Central snapshot repository at https://central.sonatype.com/repository/maven-snapshots, not the old OSSRH at https://oss.sonatype.org/content/repositories/snapshots/
- resumePolling(): store pollingIntervalMs/pollingJitterMs on instance so pause/resume works after initial polling is started via Builder - safeCacheKey(): guard against keys shorter than 8 characters with Math.min to avoid StringIndexOutOfBoundsException - BaseCacheFile.setContents(): use try-with-resources to close writer even when write() throws - FileBackedByteStore: use a dedicated single-thread executor for I/O instead of ForkJoinPool.commonPool() to avoid pool saturation on low-core-count devices - ConfigCacheFile: document why deprecated package-private constructors exist (v3→v4 migration support) - build.gradle: clarify intent of EPPO_VERSION build config field
- safeCacheKey(): add null/empty guard returning "" to match Copilot suggestion; also prevents NPE on null key during initialization - GsonConfigurationCodec: delete from framework module; uses reflection on private fields of Configuration which R8 will rename in release builds (no proguard keep rules), causing NoSuchFieldException at runtime — flagged by both Copilot and code reviewer
|
@copilot review |
There was a problem hiding this comment.
Pull request overview
Adds a new android-sdk-framework Android library module that provides a shared base client and configuration persistence abstractions for Android SDK implementations, while keeping the existing :eppo module unchanged.
Changes:
- Introduces
AndroidBaseClient(builder-based singleton) extending the v4 Java SDK framework client. - Adds storage abstractions (
CachingConfigurationStore,ByteStore,ConfigurationCodec) and a file-backed persistence implementation. - Adds Robolectric unit tests, an instrumentation test for polling pause/resume, and a large JSON fixture used by tests.
Reviewed changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
settings.gradle |
Adds the new module include and adjusts the snapshots Maven repo URL. |
android-sdk-framework/build.gradle |
Declares the new Android library module, dependencies, publishing/signing, and Spotless config. |
android-sdk-framework/src/main/AndroidManifest.xml |
Minimal manifest for the library module. |
android-sdk-framework/src/main/java/cloud/eppo/android/framework/AndroidBaseClient.java |
Base Android client with builder/init flow and polling pause/resume. |
android-sdk-framework/src/main/java/cloud/eppo/android/framework/exceptions/NotInitializedException.java |
Exception thrown when accessing the singleton before initialization. |
android-sdk-framework/src/main/java/cloud/eppo/android/framework/exceptions/EppoInitializationException.java |
Initialization error wrapper exception. |
android-sdk-framework/src/main/java/cloud/eppo/android/framework/util/Utils.java |
Shared helpers for log tags and cache key derivation. |
android-sdk-framework/src/main/java/cloud/eppo/android/framework/storage/ByteStore.java |
Async byte I/O abstraction. |
android-sdk-framework/src/main/java/cloud/eppo/android/framework/storage/BaseCacheFile.java |
File wrapper for cache read/write helpers. |
android-sdk-framework/src/main/java/cloud/eppo/android/framework/storage/ConfigCacheFile.java |
Cache filename/extension mapping based on content type. |
android-sdk-framework/src/main/java/cloud/eppo/android/framework/storage/ConfigurationCodec.java |
Pluggable config serialization API with Java-serialization default. |
android-sdk-framework/src/main/java/cloud/eppo/android/framework/storage/CachingConfigurationStore.java |
In-memory config cache with async persistence support. |
android-sdk-framework/src/main/java/cloud/eppo/android/framework/storage/FileBackedByteStore.java |
File-backed ByteStore implementation. |
android-sdk-framework/src/main/java/cloud/eppo/android/framework/storage/FileBackedConfigStore.java |
Wiring of CachingConfigurationStore + file-backed persistence. |
android-sdk-framework/src/test/resources/flags-v1.json |
Large JSON fixture used to create a non-empty configuration in tests. |
android-sdk-framework/src/test/java/cloud/eppo/android/framework/storage/CachingConfigurationStoreTest.java |
Tests for caching, persistence behavior, and concurrency scenarios. |
android-sdk-framework/src/test/java/cloud/eppo/android/framework/storage/ConfigurationCodecTest.java |
Tests for default codec behavior and error cases. |
android-sdk-framework/src/test/java/cloud/eppo/android/framework/storage/FileBackedByteStoreTest.java |
Tests for file-backed byte store read/write behavior. |
android-sdk-framework/src/test/java/cloud/eppo/android/framework/storage/FileBackedConfigStoreTest.java |
Tests for file-backed config store behavior. |
android-sdk-framework/src/androidTest/java/cloud/eppo/android/framework/EppoClientPollingTest.java |
Instrumentation tests for polling pause/resume call safety. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @Nullable private static AndroidBaseClient<?> instance; | ||
|
|
||
| /** | ||
| * Private constructor. Use Builder to construct instances. | ||
| * | ||
| * @param apiKey API key for Eppo | ||
| * @param sdkName SDK name identifier | ||
| * @param sdkVersion SDK version string | ||
| * @param apiBaseUrl Base URL for API calls | ||
| * @param assignmentLogger Logger for assignments | ||
| * @param configurationStore Store for configuration persistence | ||
| * @param isGracefulMode Whether to operate in graceful mode | ||
| * @param expectObfuscatedConfig Whether configuration is obfuscated | ||
| * @param initialConfiguration Initial configuration future | ||
| * @param assignmentCache Cache for assignments | ||
| * @param configurationParser Parser for configuration JSON | ||
| * @param configurationClient HTTP client for configuration fetching | ||
| */ | ||
| protected AndroidBaseClient( | ||
| String apiKey, | ||
| String sdkName, | ||
| String sdkVersion, | ||
| @Nullable String apiBaseUrl, | ||
| @Nullable AssignmentLogger assignmentLogger, | ||
| CachingConfigurationStore configurationStore, | ||
| boolean isGracefulMode, | ||
| boolean expectObfuscatedConfig, | ||
| @Nullable CompletableFuture<Configuration> initialConfiguration, | ||
| @Nullable IAssignmentCache assignmentCache, | ||
| ConfigurationParser<JsonFlagType> configurationParser, | ||
| EppoConfigurationClient configurationClient) { | ||
| super( | ||
| apiKey, | ||
| sdkName, | ||
| sdkVersion, | ||
| apiBaseUrl, | ||
| assignmentLogger, | ||
| null, // banditLogger is not supported in Android | ||
| configurationStore, | ||
| isGracefulMode, | ||
| expectObfuscatedConfig, | ||
| false, // no bandits. | ||
| initialConfiguration, | ||
| assignmentCache, | ||
| null, | ||
| configurationParser, | ||
| configurationClient); | ||
| } | ||
|
|
||
| /** | ||
| * Gets the singleton instance of EppoClient. | ||
| * | ||
| * @return The singleton instance | ||
| * @throws NotInitializedException if the client has not been initialized | ||
| * @param <T> The JSON type parameter | ||
| */ | ||
| @SuppressWarnings("unchecked") | ||
| public static <T> AndroidBaseClient<T> getInstance() throws NotInitializedException { | ||
| if (instance == null) { | ||
| throw new NotInitializedException(); | ||
| } | ||
| return (AndroidBaseClient<T>) instance; | ||
| } |
There was a problem hiding this comment.
instance is a mutable static used for a singleton but isn't volatile and access isn't synchronized. If initialization and getInstance() can occur across threads, this risks visibility issues (reading a partially constructed instance) and racy reinitialization. Consider making instance volatile and/or synchronizing the initialization path (or using an AtomicReference).
|
|
||
| // Dedicated single-thread executor avoids saturating ForkJoinPool.commonPool() with blocking I/O | ||
| // on low-core-count Android devices. | ||
| private static final Executor IO_EXECUTOR = Executors.newSingleThreadExecutor(); |
There was a problem hiding this comment.
IO_EXECUTOR is a static newSingleThreadExecutor() that is never shut down and uses a non-daemon thread by default. This can leak a thread for the lifetime of the process and can also prevent JVM test runners from exiting cleanly. Consider using a ThreadFactory that creates daemon/named threads and/or an ExecutorService that can be shut down (or delegate to an app-provided executor).
| private static final Executor IO_EXECUTOR = Executors.newSingleThreadExecutor(); | |
| private static final Executor IO_EXECUTOR = | |
| Executors.newSingleThreadExecutor(r -> { | |
| Thread t = new Thread(r); | |
| t.setName("FileBackedByteStore-IO"); | |
| t.setDaemon(true); | |
| return t; | |
| }); |
| * | ||
| * @param configClass the class of the configuration type | ||
| */ | ||
| public Default(@NotNull Class<T> configClass) { |
There was a problem hiding this comment.
Default accepts configClass as @NotNull but doesn't check it. If callers accidentally pass null, fromBytes() will throw a confusing NPE when calling configClass.isInstance(...). Consider validating configClass in the constructor and throwing IllegalArgumentException if it's null.
| public Default(@NotNull Class<T> configClass) { | |
| public Default(@NotNull Class<T> configClass) { | |
| if (configClass == null) { | |
| throw new IllegalArgumentException("configClass must not be null"); | |
| } |
| package cloud.eppo.android.framework.util; | ||
|
|
||
| public class Utils { | ||
| public static String logTag(Class loggingClass) { |
There was a problem hiding this comment.
logTag uses a raw Class type and doesn't guard against null, which can lead to a runtime NPE (loggingClass.getSimpleName()). Consider changing the signature to Class<?> (or @NotNull Class<?>) and validating the argument.
| public static String logTag(Class loggingClass) { | |
| public static String logTag(Class<?> loggingClass) { | |
| if (loggingClass == null) { | |
| // Fallback tag when no class is provided | |
| return "EppoSDK"; | |
| } |
| mavenLocal() | ||
| maven { | ||
| url "https://central.sonatype.com/repository/maven-snapshots/" | ||
| url "https://central.sonatype.com/repository/maven-snapshots" |
There was a problem hiding this comment.
Maven repository base URLs typically need a trailing slash for correct URI resolution of artifact paths. Without it, some resolvers can treat the last path segment as a file and resolve dependencies incorrectly. Consider restoring the trailing / on this snapshots repo URL.
| url "https://central.sonatype.com/repository/maven-snapshots" | |
| url "https://central.sonatype.com/repository/maven-snapshots/" |
| public AndroidBaseClient<JsonFlagType> buildAndInit() { | ||
| try { | ||
| return buildAndInitAsync().get(); | ||
| } catch (ExecutionException | InterruptedException | CompletionException e) { | ||
| // If the exception was an `EppoInitializationException`, we know for sure that | ||
| // `buildAndInitAsync` logged it (and wrapped it with a RuntimeException) which was then | ||
| // wrapped by `CompletableFuture` with a `CompletionException`. | ||
| if (e instanceof CompletionException) { | ||
| Throwable cause = e.getCause(); | ||
| if (cause instanceof RuntimeException | ||
| && cause.getCause() instanceof EppoInitializationException) { | ||
| @SuppressWarnings("unchecked") | ||
| AndroidBaseClient<JsonFlagType> typedInstance = | ||
| (AndroidBaseClient<JsonFlagType>) instance; | ||
| return typedInstance; | ||
| } | ||
| } | ||
| Log.e(TAG, "Exception caught during initialization: " + e.getMessage(), e); | ||
| if (!isGracefulMode) { | ||
| throw new RuntimeException(e); | ||
| } | ||
| } |
There was a problem hiding this comment.
buildAndInit() catches InterruptedException but does not restore the thread interrupt status (e.g., via Thread.currentThread().interrupt()). Swallowing interrupts can break caller cancellation/timeout logic. Consider re-interrupting the thread when InterruptedException is caught (and decide whether to propagate or return gracefully).
| AndroidBaseClient<JsonNode> androidBaseClient = buildOfflineClientWithoutPolling(); | ||
| assertNotNull("Client should be initialized", androidBaseClient); | ||
|
|
||
| // Try to resume polling (should log warning and not crash per EppoClient.java:436-441) |
There was a problem hiding this comment.
These comments reference EppoClient.java:436-441, but this module uses AndroidBaseClient and line numbers will drift over time. Consider referencing the specific method (AndroidBaseClient.resumePolling) or the expected behavior instead of a hard-coded file/line reference.
| // Try to resume polling (should log warning and not crash per EppoClient.java:436-441) | |
| // Try to resume polling; AndroidBaseClient.resumePolling should log a warning and not crash if polling was never started |
| } else if (offlineMode || failCount.incrementAndGet() == 2) { | ||
| ret.completeExceptionally( | ||
| new EppoInitializationException( | ||
| "Unable to initialize client; Configuration could not be loaded", ex)); | ||
| } else { | ||
| Log.i(TAG, "Initial config was not used."); | ||
| failCount.incrementAndGet(); |
There was a problem hiding this comment.
The failCount increment logic here double-increments on the first "initial config not used" path: failCount.incrementAndGet() == 2 is evaluated (incrementing), then the else block increments again. This can cause initialization to fail/short-circuit earlier than intended. Refactor to increment exactly once per failure (e.g., store the incremented value in a local variable and reuse it in the condition/logging).
| } else if (offlineMode || failCount.incrementAndGet() == 2) { | |
| ret.completeExceptionally( | |
| new EppoInitializationException( | |
| "Unable to initialize client; Configuration could not be loaded", ex)); | |
| } else { | |
| Log.i(TAG, "Initial config was not used."); | |
| failCount.incrementAndGet(); | |
| } else { | |
| int failures = failCount.incrementAndGet(); | |
| if (offlineMode || failures == 2) { | |
| ret.completeExceptionally( | |
| new EppoInitializationException( | |
| "Unable to initialize client; Configuration could not be loaded", | |
| ex)); | |
| } else { | |
| Log.i(TAG, "Initial config was not used."); | |
| } |
| private volatile Configuration configuration = Configuration.emptyConfig(); | ||
|
|
||
| protected CachingConfigurationStore( | ||
| @NotNull ConfigurationCodec<Configuration> codec, @NotNull ByteStore byteStore) { |
There was a problem hiding this comment.
The constructor accepts codec/byteStore as @NotNull but doesn't validate them. Passing null will lead to later NPEs that are harder to diagnose. Consider adding explicit null checks and throwing IllegalArgumentException with a clear message.
| @NotNull ConfigurationCodec<Configuration> codec, @NotNull ByteStore byteStore) { | |
| @NotNull ConfigurationCodec<Configuration> codec, @NotNull ByteStore byteStore) { | |
| if (codec == null) { | |
| throw new IllegalArgumentException("codec must not be null"); | |
| } | |
| if (byteStore == null) { | |
| throw new IllegalArgumentException("byteStore must not be null"); | |
| } |
| ConfigCacheFile( | ||
| @NotNull Application application, | ||
| @NotNull String configType, | ||
| @NotNull String suffix, | ||
| @NotNull String contentType) { | ||
| this(application, configType + "-" + suffix, contentType); | ||
| } | ||
|
|
||
| /** | ||
| * Creates a cache file with the given full file name (no prefix). Used when the caller supplies | ||
| * the complete filename (e.g. baseName + "." + extension). | ||
| * | ||
| * @deprecated Use {@link #ConfigCacheFile(Application, String, String)} instead. These | ||
| * package-private constructors exist only to support migration of the eppo module from v3 to | ||
| * v4; they will be removed once that migration is complete. | ||
| */ | ||
| ConfigCacheFile(@NotNull Application application, @NotNull String fullFileName) { | ||
| super(application, fullFileName); |
There was a problem hiding this comment.
These constructors are documented with @deprecated in Javadoc but aren't annotated with @Deprecated. Without the annotation, callers won't get compile-time deprecation warnings. Consider adding @Deprecated to each deprecated constructor (and optionally @SuppressWarnings("deprecation") in call sites if needed).
aarsilv
left a comment
aarsilv
left a comment
There was a problem hiding this comment.
Nice work! You've really put together a flexible, extensible, and well-tested Android SDK with many lessons we should take with us to Datadog. (On that note, please write up lessons from all this in a confluence page). I like the clean code and good test coverage including concurrency tests.
In addition to my own review, I added co-pilot and also had Claude take a pass. I'm approving as-is, although there are some things that I and the tools found that I'd love for you to engage on:
- EppoClientPollingTest should check for actual poll calls (or lack of, when paused) using spys or some other mechanism
- Copilot-identified leaky newSingleThreadExecutor
- Claude-identified saveConfiguration disk/memory race condition
- Name of AndroidBaseClient
- Consider a default no-op instance
- Claude-identified possibility of
buildAndInit()hanging forever - Copilot-identified need of atomic singleton and/or thread safe access of instance in AndroidBaseClient (volatile/synchronized)
- gson shouldn't be a dependency anymore
- slf4j-android maybe should be runtime only
| * | ||
| * @return Initialized EppoClient | ||
| */ | ||
| private AndroidBaseClient<JsonNode> buildOfflineClientWithoutPolling() |
There was a problem hiding this comment.
pretty much same as previous method but with a different boolean passed for pollingEnabled() and the call to pollingIntervalMs() Could be consolidated to a single method (with 0 pollingMs meaning no polling) or the explicitly named methods with a shared base method.
| } | ||
|
|
||
| @Test | ||
| public void testPauseAndResumePolling() throws ExecutionException, InterruptedException { |
There was a problem hiding this comment.
This test doesn't seem to be doing much. Consider a shorter polling interval and then pausing longer than the interval to make sure no polls happens and then check polling happened again after unpausing.
| } | ||
|
|
||
| @Test | ||
| public void testPauseResumeSequenceDoesNotCrash() |
| } | ||
|
|
||
| @Test | ||
| public void testMultiplePauseResumeCycles() throws ExecutionException, InterruptedException { |
| ConfigCacheFile cacheFile = | ||
| new ConfigCacheFile(application, cacheFileSuffix, codec.getContentType()); | ||
| return new FileBackedByteStore(cacheFile); |
There was a problem hiding this comment.
Pretty small--any reason not to inline it int he constructor?
| (success, ex) -> { | ||
| if (ex == null && Boolean.TRUE.equals(success)) { | ||
| ret.complete(newInstance); | ||
| } else if (offlineMode || failCount.incrementAndGet() == 2) { |
There was a problem hiding this comment.
Had Claude give this a look and it is worried that buildAndInitAsync() can hang forever.
buildAndInitAsync() can hang forever. If initialConfigFuture is non-null and
resolves with success = false/null, the else branch increments
failCount twice (to 2, then 3). If loadConfigurationAsync then also fails,
incrementAndGet() returns 4 — the == 2 check never matches, and ret is never
completed. The caller blocks forever on .get().
Also on that note, consider providing an optional timeout (with a default to something generous like 30 seconds?)
There was a problem hiding this comment.
It also has a possible other recommendation regarding use of anyOf()
ret.complete() can be called from two different callbacks on success. Both
loadConfigurationAsync and initialConfigFuture handlers call
ret.complete(newInstance) on success. CompletableFuture.complete() is
idempotent so this isn't a bug, but it suggests the dual-callback design is
more complex than it needs to be. A clearer approach would be
CompletableFuture.anyOf() for success with an explicit failure combiner.
| delete(); | ||
| try (BufferedWriter writer = getWriter()) { | ||
| writer.write(contents); |
There was a problem hiding this comment.
Technically, a crash between these two will result in the loss of file. So you could do write and then rename. But if this is is only used for tests not worth worrying about this edge case.
| private long pollingIntervalMs; | ||
| private long pollingJitterMs; | ||
|
|
||
| @Nullable private static AndroidBaseClient<?> instance; |
There was a problem hiding this comment.
By not having a default no-op instance, buildAndInit() could return null if graceful mode is at play and an error happens before instance is set.
| api 'cloud.eppo:eppo-sdk-framework:0.1.0-SNAPSHOT' | ||
|
|
||
| api 'com.google.code.gson:gson:2.10.1' | ||
| api 'org.slf4j:slf4j-android:1.7.36' |
There was a problem hiding this comment.
Don't see this in the code, should this be runtimeOnly?
| .thenRun( | ||
| () -> { | ||
| this.configuration = config; | ||
| }); |
There was a problem hiding this comment.
Had Claude dig into this, and it thinks there a race condition.
The write goes to the single-thread IO_EXECUTOR, but thenRun executes on
whatever thread completes the future — typically the IO thread, but not
guaranteed. Here's the race:
Thread 1: saveConfiguration(A)
1. codec.toBytes(A)
2. byteStore.write(A) → queued on IO_EXECUTOR
Thread 2: saveConfiguration(B)
3. codec.toBytes(B)
4. byteStore.write(B) → queued on IO_EXECUTOR
IO_EXECUTOR runs them in order:
5. writes A to disk
6. writes B to disk ← disk now has B
thenRun callbacks fire:
7. this.configuration = B ← from step 4's callback
8. this.configuration = A ← from step 2's callback (ran slightly later)
Result: disk=B, memory=A
Step 8 can run after step 7 because thenRun doesn't run on the IO_EXECUTOR —
it runs on whatever thread calls complete(), or the calling thread if the
future is already done. The two thenRun callbacks are not ordered by the
single-thread executor.
The simplest fix is to run the in-memory update inside the IO_EXECUTOR too:
return byteStore
.write(bytes)
.thenRunAsync(() -> { this.configuration = config; }, IO_EXECUTOR);
This guarantees the disk write and memory update for A both happen before B's,
since they're all serialized on the same single thread. Though that would
require exposing the executor or moving the logic into FileBackedByteStore.
|
I asked Claude to check past PRs for past issues and make sure we're not reintroducing them, it came up: Details: |
Summary
Add new android-sdk-framework module that provides base client functionality and configuration storage abstractions for Android SDK implementations.
Key Components
Dependencies
This module depends on:
Testing
Notes