Repository navigation
Prepare Java server SDK 6.0.0 with stable common v4 #158
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
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 | ||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -11,7 +11,7 @@ java { | |||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| group = 'cloud.eppo' | ||||||||||||||||||||||||||||||||||||||||||
| version = '6.0.0-SNAPSHOT' | ||||||||||||||||||||||||||||||||||||||||||
| version = '6.0.0' | ||||||||||||||||||||||||||||||||||||||||||
|
Collaborator
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. 🤖 Comment from Claude working with Aaron Silverman: Reminder: release steps for 6.0.0. No code change is needed.
A release checklist in the PR body would capture these. Evidence
|
||||||||||||||||||||||||||||||||||||||||||
| ext.isReleaseVersion = !version.endsWith("SNAPSHOT") | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| import org.apache.tools.ant.filters.ReplaceTokens | ||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -30,7 +30,7 @@ repositories { | |||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| dependencies { | ||||||||||||||||||||||||||||||||||||||||||
| api 'cloud.eppo:sdk-common-jvm:4.0.0-SNAPSHOT' | ||||||||||||||||||||||||||||||||||||||||||
| api 'cloud.eppo:sdk-common-jvm:4.0.1' | ||||||||||||||||||||||||||||||||||||||||||
|
Collaborator
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. 🤖 Comment from Claude working with Aaron Silverman: [P1] Please hold 6.0.0 until common 4.0.2 ships Eppo-exp/sdk-common-jdk#274, then pin Framework 0.1.1 sends Evidence — the same test on three buildsThe test is in the last section. Flags send
The live config endpoint sends ETags: even a 401 from The fix — sdk-common-jdk#274,
|
||||||||||||||||||||||||||||||||||||||||||
| Path | Server 5.3.4 | This PR | This PR + #275 |
|---|---|---|---|
Split matched (numeric_flag, integer-flag) |
~135 ns, ~410 B per call | ~370 ns, ~1,790 B | ~204 ns, 800 B |
Flags with targeting rules (new-user-onboarding, kill-switch) |
~380 ns, ~1,500 B | ~618 ns, ~3,190 B | ~495 ns, ~2,040 B |
*AssignmentDetails calls |
n/a | 627–678 ns, 3,163–3,187 B | 627–628 ns, 3,163–3,211 B |
These are loop timings that include building the subject string. Compare them across columns rather than reading them as absolute latency. I measured #275 at its first commit; its later commits change only tests, docs, Javadoc and visibility.
A JFR allocation profile of this PR put the matched-rule set, the String.format description and the details builders at the top.
The fix — sdk-common-jdk#275
Plain calls skip the allocation details, the matched rule and the match description. They still build a small EvaluationDetails, and the fallback descriptions for a missing, disabled or mistyped flag remain. For valid configurations, evaluation and assignment logging stay the same for callers, and the *Details methods are unchanged. In graceful mode, a plain call now also returns the default when the configuration store throws or returns null.
A subclass that overrides the four-argument evaluateAndMaybeLog or a get*AssignmentDetails method no longer intercepts plain calls. EppoClient and CommonEppoClient override neither.
/**
* Value-only counterpart of {@link #getTypedAssignmentWithDetails}: same evaluation and logging,
* without the allocation lists, matched rule or match description.
*/
private <T> T getTypedAssignment(
String flagKey,
String subjectKey,
Attributes subjectAttributes,
T defaultValue,
VariationType expectedType) {
try {
EvaluationDetails details =
evaluateAndMaybeLog(flagKey, subjectKey, subjectAttributes, expectedType, false);
return details.evaluationSuccessful()
? details.getVariationValue().unwrap(expectedType, configurationParser::parseJsonValue)
: defaultValue;
} catch (Exception e) {
return throwIfNotGraceful(e, defaultValue);
}
}#275 has no version bump. If it merges before #274 is released, it ships in common 4.0.2. Its ./gradlew check passes 392 tests, and 407 when merged with #274 at 3c95f0e.
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.
🤖 Comment from Claude working with Aaron Silverman:
[P2] Please consider three overrides in EppoClient, so code compiled against 5.3.4 that calls these EppoClient methods still links against 6.0.0.
With common 4.0.1, EppoClient extends BaseEppoClient<Configuration, JsonNode>. That erases the compiled signatures of both getJSONAssignment overloads (return type and default parameter) and the return type of getConfiguration(). A jar compiled against 5.3.4 that calls them on an EppoClient throws NoSuchMethodError on 6.0.0. That is an Error, so graceful mode does not catch it. Code that uses only these calls recompiles cleanly, so the failure shows up at runtime.
The likely case is a wrapper library compiled against 5.x. By default, Gradle resolves the highest requested version, so an app on 6.0.0 can run that wrapper without recompiling it. GitHub's public code search finds no published Java library built on eppo-server-sdk; I did not check private or prebuilt components.
Evidence — one class file compiled against 5.3.4, run on three classpaths
| Call | 5.3.4 | This PR at 40b0ac5 |
This PR + the overrides |
|---|---|---|---|
getBooleanAssignment |
works | works | works |
getJSONAssignment |
works | NoSuchMethodError |
works |
getConfiguration |
works | NoSuchMethodError |
works |
javap lists 18 public method descriptors on 5.3.4's EppoClient and BaseEppoClient. This PR is missing 3 of them; with the overrides, none are missing.
Suggested fix — EppoClient.java, plus a regression test
Add import cloud.eppo.api.Attributes; and, after the constructor:
// Concrete return types keep callers compiled against 5.x linking: generics erased these
// descriptors in 6.0.0.
@Override
public Configuration getConfiguration() {
return super.getConfiguration();
}
@Override
public JsonNode getJSONAssignment(String flagKey, String subjectKey, JsonNode defaultValue) {
return super.getJSONAssignment(flagKey, subjectKey, defaultValue);
}
@Override
public JsonNode getJSONAssignment(
String flagKey, String subjectKey, Attributes subjectAttributes, JsonNode defaultValue) {
return super.getJSONAssignment(flagKey, subjectKey, subjectAttributes, defaultValue);
}In EppoClientTest, add import com.fasterxml.jackson.databind.JsonNode; and:
@Test
public void testKeepsVersion5MethodDescriptors() throws NoSuchMethodException {
assertEquals(
Configuration.class, EppoClient.class.getMethod("getConfiguration").getReturnType());
assertEquals(
JsonNode.class,
EppoClient.class
.getMethod("getJSONAssignment", String.class, String.class, JsonNode.class)
.getReturnType());
assertEquals(
JsonNode.class,
EppoClient.class
.getMethod(
"getJSONAssignment", String.class, String.class, Attributes.class, JsonNode.class)
.getReturnType());
}With both, this PR's 44 tests and the new test pass. The new test fails without the overrides: expected: <cloud.eppo.api.Configuration> but was: <cloud.eppo.api.SerializableEppoConfiguration>. Spotless passes.
With these overrides, every public method descriptor of 5.3.4's EppoClient and BaseEppoClient resolves again through EppoClient. Calls compiled against BaseEppoClient directly, and code that uses moved DTO types, still need a recompile, which the README block in the comment on line 18 asks for.
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.
🤖 Comment from Claude working with Aaron Silverman:
[P2] Please remove java-semver (this line) and ehcache (line 37). Nothing in src/ uses either one.
- The framework already brings
java-semverat runtime. - Removing
ehcachetakes nine jars off the runtime classpath that this SDK brings to consumers. - The removal can break a consumer that gets
ehcacheonly through this SDK. A major release is the right time for that.
Evidence
- No file under
src/mainorsrc/test/javareferencesehcache,zafarkhajaorsemver. - With both lines removed, this PR's suite passes 44/44.
- These jars leave the runtime classpath:
ehcache,cache-api,jaxb-api,jaxb-runtime,txw2,istack-commons-runtime,stax-ex,FastInfosetandjavax.activation-api. - The framework 0.1.1 POM declares
java-semver0.10.2 at runtime scope.
Lines to delete — line 35 and line 37
Line 35:
implementation 'com.github.zafarkhaja:java-semver:0.10.2'Line 37:
implementation 'org.ehcache:ehcache:3.11.1'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.
🤖 Comment from Claude working with Aaron Silverman:
[P2] Please drop the version from the tests-jar dependency, so the tests run against the framework version that ships.
Gradle resolves one eppo-sdk-framework version on the test classpath. If this pin is higher than the version common requires, the tests run on the higher version. The published POM still ships the lower one. Dependabot can bump this pin on its own.
Evidence — a local mutation passed the suite while the POM shipped the buggy framework
I pinned common 4.0.0, which requires framework 0.1.0 with the JSON-string ClassCastException, and kept this line at 0.1.1.
| Classpath | Framework |
|---|---|
runtimeClasspath (what the POM ships) |
0.1.0 |
testRuntimeClasspath |
0.1.0 → 0.1.1 |
The suite passed 44/44.
Suggested change — replaces lines 41–42
// No version: tests use the framework version sdk-common-jvm resolves, which is what ships.
testImplementation 'cloud.eppo:eppo-sdk-framework::tests'
// Logback classic 1.3.x is compatible with java 8 - only needed for testsWith common 4.0.1, both classpaths resolve framework 0.1.1 and the suite passes 44/44. With common 4.0.0, both resolve 0.1.0. Spotless passed on a build.gradle that includes this change.
This makes the tests match what ships. It does not add coverage that an older test harness lacks.
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.
🤖 Comment from Claude working with Aaron Silverman:
[P1] Please replace this section. Its import rule can break an import, and it misses several breaking changes.
cloud.eppo.ufc.dto.adapters.EppoModule, which did not move.cloud.eppo.model.ShardRangemoved, five public methods onConfigurationandAttributeswere removed, and four adapters are no longer public.onConfigurationChangeno longer fires when a poll gets a 304. In 5.3.4 it fired after every successful poll.What I checked — against the published 3.13.2, 4.0.1 and framework 0.1.1 jars
I compared
javap -publicoutput and the class lists of common 3.13.2 with common 4.0.1 plus framework 0.1.1.cloud.eppo.ufc.dtotocloud.eppo.api.dto." Common 4.0.1 still shipscloud.eppo.ufc.dto.adapters.EppoModuleandEppoValueDeserializerstay public there; the other four adapters are now package-private.Defaultimplementation, for exampleFlagConfig.Default.BanditAttributeCoefficientshas noDefault.cloud.eppo.model.ShardRangeis nowcloud.eppo.api.dto.ShardRange.Configuration.builder(byte[]),builder(byte[], boolean),serializeFlagConfigToBytes(),serializeBanditParamsToBytes()andAttributes.serializeNonNullAttributesToJSONString().Configuration.builder(FlagConfigResponse)infers the obfuscation flag from the payload format;new Configuration.Builder(FlagConfigResponse, boolean)takes it explicitly. Both replacements in the block below compiled and parsedflags-v1.json(22 of 22 flags).cloud.eppo.http.EppoConfigurationClient, butEppoClient.Builderhas no way to supply one.testConfigurationChangeListenerasserted a third callback after an unchanged reload, with the comment "Java doesn't check eTag (yet)". Upgrade sdk-common-jvm to v4.0.0 (major version bump to 6.0.0) #156 removed that assertion. 5.3.4 never sentIf-None-Match, so every successful poll saved and notified. In 6.0.0, a poll that gets a 304 returns before the store saves, so no callback fires.The strict-mode paragraph is correct, including the non-null qualifier for
getJSONAssignment. The block keeps it.Suggested replacement for lines 18–35
If 6.0.0 pins common 4.0.2 (see the comment on
build.gradleline 33), change4.0.1to4.0.2and0.1.1to0.1.2in the first paragraph.