-
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?
[Draft] Android api 21 support #109
Conversation
String.join() is not available until Android API 26
java.util.Base64 isn't available until Android API 26.
} | ||
|
||
@Test | ||
public void testInterruptedFetchDoesNotClobberCache() throws Exception { |
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.
An equivalent to this test didn't previously exist, but I think it's important to verify that thread interruption works since there needs to be a way to cancel blocking operations.
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.
Thanks for engaging on this! Things are making sense to me at a high-level, making the ability to drop in compatible internals where needed as well creating compatible flavors of those internals.
For encoding, I wonder if we just fully delegate that to upstream.
For json parsing its trickier, as Jackson will likely be faster something we want for both Android and Java. However, will Api level 21 compile with it present in the codebase? And if so, could we do something like the template pattern to reduce duplication? The latter is my main concern, the duplication could lead to drift between Java6 and "normal" flavors of things (and their tests!).
I'm less familiar with multi-API-level programming practices as well as the available annotations and their effects, so apologies if some of what I say does not make sense.
@@ -20,6 +20,8 @@ dependencies { | |||
implementation 'com.fasterxml.jackson.core:jackson-databind:2.18.3' | |||
implementation 'com.github.zafarkhaja:java-semver:0.10.2' | |||
implementation "com.squareup.okhttp3:okhttp:4.12.0" | |||
// For base64 that works on on Android API 21 | |||
implementation "net.iharder:base64:2.3.8" |
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 uses android.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 in android-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.
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 comment
The 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 comment
The 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 MapperNode
when I finalize this PR
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 comment
The 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 ConfigurationRequestorJava6
class?
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 comment
The 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 CompletableFuture
s? The initialization code is certainly cleaner with CompletableFuture
s
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'd actually like to get rid of CompletableFuture
if possible and provide callbacks because CompletableFuture
relies on the ForkJoin Pool, and clients would probably like to minimize the number of thread pools in their apps.
You could certainly provide wrapper APIs that expose CompletableFuture
, but some clients are going to use RxJava
(which has its own thread pools), and some will use Kotlin coroutines (which has its own thread pools). I imagine customers would like to avoid creating one more thread at startup if possible.
A simple way to do this would be to provide a synchronous API like I did here, and then provide a CompletableFuture
wrapper around 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.
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 comment
The 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.
for (String string : this.stringArrayValue) { | ||
stringBuilder.append(string).append(delimiter); | ||
} | ||
stringBuilder.setLength(stringBuilder.length() - delimiter.length()); |
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.
Ah this is to avoid a trailing delimiter 👌
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 could also do:
for (Iterator<String> i = this.stringArrayValue; i.hasNext();) {
String string = i.next();
stringBuilder.append(string);
if (i.hasNext()) {
stringBuilder.append(delimiter);
}
}
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 more readable.
|
||
import cloud.eppo.api.Configuration; | ||
|
||
public class ConfigurationRequestorJava6Test { |
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 wonder if to remove code duplication and keep tests in sync we could have one test suite that using a data provider or something runs each test on both implementations. Although things may get funky with mocking, but perhaps we could have an abstract parent class or shared helper methods or something.
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 could create some abstractions to reduce duplication, but I find that too much abstraction in tests obscures the point of the test itself, so I'm generally ok with copypasta in test-land.
I also like this as a separate class because it establishes strong boundaries between Java6-land and Java8-land.
However, if this doesn't match the philosophy of your project, I'm happy to refactor.
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.
Thank you for tackling this! It's a tough set of use cases to make work all while respecting the API
As mentioned, I'd really like to get away from code duplication as much as possible.
If we just convert all the CompletableFuture
s to callbacks, for instance, how much does that simplify the implementation? Do you feel limited by respecting the API and not introducing a breaking change?
for (String string : this.stringArrayValue) { | ||
stringBuilder.append(string).append(delimiter); | ||
} | ||
stringBuilder.setLength(stringBuilder.length() - delimiter.length()); |
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 more readable.
@@ -20,6 +20,8 @@ dependencies { | |||
implementation 'com.fasterxml.jackson.core:jackson-databind:2.18.3' | |||
implementation 'com.github.zafarkhaja:java-semver:0.10.2' | |||
implementation "com.squareup.okhttp3:okhttp:4.12.0" | |||
// For base64 that works on on Android API 21 | |||
implementation "net.iharder:base64:2.3.8" |
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 in android-sdk
). wdyt?
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 comment
The 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 CompletableFuture
s? The initialization code is certainly cleaner with CompletableFuture
s
Yes. I was really trying not to introduce any breaking changes into your API. I was also trying to keep the Java6 implementation very separate to make it easy to rip back out again One Day. |
If we have to introduce some breaking changes to eliminate developer friction, I'm all for it. Especially in the common SDK where we can adapt as needed in the server and android SDKs. |
This is a draft of implementations necessary to run on Android API 21. I haven't begun actually testing this on an Android API 21 emulator yet because I still need to work through backporting android-sdk, but this should give you an idea of the direction I'm going.
Major issues:
String.join()
requires JDK8, so use StringBuilder insteadjava.util.Base64
requires JDK8, so usenet.iharder:base64
(Public Domain) insteadCompletableFuture
require JDK8, so create blocking versions of these classes, and have Android API 21 consumers handle concurrency manually.Jackson
requires JDK7, so put all Jackson consumption behind an interface so that Android API 21 can inject aJSONObject
-based implementation.Testing:
I added what I thought were tests equivalent to what already existed. If you want more tests, please LMK