-
Notifications
You must be signed in to change notification settings - Fork 1
Remove singleton pattern #28
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
Changes from all commits
e2f9074
e7880d5
b964d83
58e28d6
5e856be
b10ffb0
1519ba6
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 |
---|---|---|
|
@@ -62,11 +62,17 @@ java { | |
withSourcesJar() | ||
} | ||
|
||
tasks.register('testJar', Jar) { | ||
archiveClassifier.set('tests') | ||
from sourceSets.test.output | ||
} | ||
|
||
publishing { | ||
publications { | ||
mavenJava(MavenPublication) { | ||
artifactId = 'sdk-common-jvm' | ||
from components.java | ||
artifact testJar // Include the test-jar in the published artifacts | ||
versionMapping { | ||
usage('java-api') { | ||
fromResolutionOf('runtimeClasspath') | ||
|
@@ -114,7 +120,7 @@ publishing { | |
|
||
// Custom task to ensure we can conditionally publish either a release or snapshot artifact | ||
// based on a command line switch. See github workflow files for more details on usage. | ||
task checkVersion { | ||
tasks.register('checkVersion') { | ||
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. IntelliJ said 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. h4x0r |
||
doLast { | ||
if (!project.hasProperty('release') && !project.hasProperty('snapshot')) { | ||
throw new GradleException("You must specify either -Prelease or -Psnapshot") | ||
|
@@ -135,7 +141,7 @@ tasks.named('publish').configure { | |
} | ||
|
||
// Conditionally enable or disable publishing tasks | ||
tasks.withType(PublishToMavenRepository) { | ||
tasks.withType(PublishToMavenRepository).configureEach { | ||
onlyIf { | ||
project.ext.has('shouldPublish') && project.ext.shouldPublish | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,37 +22,46 @@ public class BaseEppoClient { | |
private final ObjectMapper mapper = | ||
new ObjectMapper() | ||
.registerModule(EppoModule.eppoModule()); // TODO: is this the best place for this? | ||
private static final String DEFAULT_HOST = "https://fscdn.eppo.cloud"; | ||
private static final boolean DEFAULT_IS_GRACEFUL_MODE = true; | ||
|
||
protected static final String DEFAULT_HOST = "https://fscdn.eppo.cloud"; | ||
protected final ConfigurationRequestor requestor; | ||
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. protected so upstream SDKs can trigger a (re)load |
||
|
||
private final ConfigurationStore configurationStore; | ||
private final ConfigurationRequestor requestor; | ||
private final AssignmentLogger assignmentLogger; | ||
private final BanditLogger banditLogger; | ||
private final String sdkName; | ||
private final String sdkVersion; | ||
private final boolean isConfigObfuscated; | ||
private boolean isGracefulMode; | ||
|
||
private static BaseEppoClient instance; | ||
|
||
// Fields useful for testing in situations where we want to mock the http client or configuration | ||
// store (accessed via reflection) | ||
/** @noinspection FieldMayBeFinal */ | ||
private static EppoHttpClient httpClientOverride = null; | ||
|
||
private BaseEppoClient( | ||
protected BaseEppoClient( | ||
String apiKey, | ||
String sdkName, | ||
String sdkVersion, | ||
String host, | ||
ConfigurationStore configurationStore, | ||
AssignmentLogger assignmentLogger, | ||
BanditLogger banditLogger, | ||
boolean isGracefulMode) { | ||
boolean isGracefulMode, | ||
boolean expectObfuscatedConfig) { | ||
|
||
if (apiKey == null) { | ||
throw new IllegalArgumentException("Unable to initialize Eppo SDK due to missing API key"); | ||
} | ||
if (sdkName == null || sdkVersion == null) { | ||
throw new IllegalArgumentException( | ||
"Unable to initialize Eppo SDK due to missing SDK name or version"); | ||
} | ||
if (host == null) { | ||
host = DEFAULT_HOST; | ||
} | ||
Comment on lines
+52
to
+61
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. Moved validation to the constructor so each SDK's builder doesn't have to worry about it |
||
|
||
EppoHttpClient httpClient = buildHttpClient(host, apiKey, sdkName, sdkVersion); | ||
this.configurationStore = configurationStore; | ||
this.configurationStore = new ConfigurationStore(); | ||
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. we were always just passing in |
||
requestor = new ConfigurationRequestor(configurationStore, httpClient); | ||
this.assignmentLogger = assignmentLogger; | ||
this.banditLogger = banditLogger; | ||
|
@@ -61,8 +70,10 @@ private BaseEppoClient( | |
this.sdkName = sdkName; | ||
this.sdkVersion = sdkVersion; | ||
// For now, the configuration is only obfuscated for Android clients | ||
this.isConfigObfuscated = sdkName.toLowerCase().contains("android"); | ||
this.isConfigObfuscated = expectObfuscatedConfig; | ||
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. π |
||
|
||
// TODO: caching initialization (such as setting an API-key-specific prefix | ||
// will probably involve passing in configurationStore to the constructor | ||
} | ||
|
||
private EppoHttpClient buildHttpClient( | ||
|
@@ -78,51 +89,7 @@ private EppoHttpClient buildHttpClient( | |
return httpClient; | ||
} | ||
|
||
public static BaseEppoClient init( | ||
String apiKey, | ||
String sdkName, | ||
String sdkVersion, | ||
String host, | ||
AssignmentLogger assignmentLogger, | ||
BanditLogger banditLogger, | ||
boolean isGracefulMode) { | ||
|
||
if (apiKey == null) { | ||
throw new IllegalArgumentException("Unable to initialize Eppo SDK due to missing API key"); | ||
} | ||
if (sdkName == null || sdkVersion == null) { | ||
throw new IllegalArgumentException( | ||
"Unable to initialize Eppo SDK due to missing SDK name or version"); | ||
} | ||
|
||
if (instance != null) { | ||
// TODO: also check we're not running a test | ||
log.warn("Reinitializing an Eppo Client instance that was already initialized"); | ||
} | ||
instance = | ||
new BaseEppoClient( | ||
apiKey, | ||
sdkName, | ||
sdkVersion, | ||
host, | ||
new ConfigurationStore(), | ||
assignmentLogger, | ||
banditLogger, | ||
isGracefulMode); | ||
instance.refreshConfiguration(); | ||
|
||
return instance; | ||
} | ||
|
||
/** | ||
* Ability to ad-hoc kick off a configuration load. Will load from a filesystem cached file as | ||
* well as fire off an HTTPS request for an updated configuration. If the cache load finishes | ||
* first, those assignments will be used until the fetch completes. | ||
* | ||
* <p>Deprecated, as we plan to make a more targeted and configurable way to do so in the future. | ||
*/ | ||
@Deprecated | ||
public void refreshConfiguration() { | ||
protected void loadConfiguration() { | ||
requestor.load(); | ||
} | ||
|
||
|
@@ -479,65 +446,7 @@ private <T> T throwIfNotGraceful(Exception e, T defaultValue) { | |
throw new RuntimeException(e); | ||
} | ||
|
||
public static BaseEppoClient getInstance() { | ||
if (BaseEppoClient.instance == null) { | ||
throw new IllegalStateException("Eppo SDK has not been initialized"); | ||
} | ||
|
||
return BaseEppoClient.instance; | ||
} | ||
|
||
public void setIsGracefulFailureMode(boolean isGracefulFailureMode) { | ||
this.isGracefulMode = isGracefulFailureMode; | ||
} | ||
|
||
public static class Builder { | ||
private String apiKey; | ||
private String sdkName; | ||
private String sdkVersion; | ||
private String host = DEFAULT_HOST; | ||
private AssignmentLogger assignmentLogger; | ||
private BanditLogger banditLogger; | ||
private boolean isGracefulMode = DEFAULT_IS_GRACEFUL_MODE; | ||
|
||
public Builder apiKey(String apiKey) { | ||
this.apiKey = apiKey; | ||
return this; | ||
} | ||
|
||
public Builder sdkName(String sdkName) { | ||
this.sdkName = sdkName; | ||
return this; | ||
} | ||
|
||
public Builder sdkVersion(String sdkVersion) { | ||
this.sdkVersion = sdkVersion; | ||
return this; | ||
} | ||
|
||
public Builder host(String host) { | ||
this.host = host; | ||
return this; | ||
} | ||
|
||
public Builder assignmentLogger(AssignmentLogger assignmentLogger) { | ||
this.assignmentLogger = assignmentLogger; | ||
return this; | ||
} | ||
|
||
public Builder banditLogger(BanditLogger banditLogger) { | ||
this.banditLogger = banditLogger; | ||
return this; | ||
} | ||
|
||
public Builder isGracefulMode(boolean isGracefulMode) { | ||
this.isGracefulMode = isGracefulMode; | ||
return this; | ||
} | ||
|
||
public BaseEppoClient buildAndInit() { | ||
return BaseEppoClient.init( | ||
apiKey, sdkName, sdkVersion, host, assignmentLogger, banditLogger, isGracefulMode); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -59,7 +59,7 @@ public Response get(String path) { | |
} | ||
} | ||
|
||
public void get(String path, RequestCallback callback) { | ||
public void get(String path, EppoHttpClientRequestCallback callback) { | ||
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. It was hard to use this upstream due to the visibility of |
||
HttpUrl httpUrl = | ||
HttpUrl.parse(baseUrl + path) | ||
.newBuilder() | ||
|
@@ -83,13 +83,11 @@ public void onResponse(Call call, Response response) { | |
callback.onFailure("Failed to read response from URL " + httpUrl); | ||
} | ||
} else { | ||
switch (response.code()) { | ||
case HttpURLConnection.HTTP_FORBIDDEN: | ||
callback.onFailure("Invalid API key"); | ||
break; | ||
default: | ||
log.debug("Fetch failed with status code: " + response.code()); | ||
callback.onFailure("Bad response from URL " + httpUrl); | ||
if (response.code() == HttpURLConnection.HTTP_FORBIDDEN) { | ||
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. According to IntelliJ (and I agree!) 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. π |
||
callback.onFailure("Invalid API key"); | ||
} else { | ||
log.debug("Fetch failed with status code: {}", response.code()); | ||
callback.onFailure("Bad response from URL " + httpUrl); | ||
} | ||
} | ||
response.close(); | ||
|
@@ -98,19 +96,12 @@ public void onResponse(Call call, Response response) { | |
@Override | ||
public void onFailure(Call call, IOException e) { | ||
log.error( | ||
"Http request failure: " | ||
+ e.getMessage() | ||
+ " " | ||
+ Arrays.toString(e.getStackTrace()), | ||
"Http request failure: {} {}", | ||
e.getMessage(), | ||
Arrays.toString(e.getStackTrace()), | ||
e); | ||
callback.onFailure("Unable to fetch from URL " + httpUrl); | ||
} | ||
}); | ||
} | ||
} | ||
|
||
interface RequestCallback { | ||
void onSuccess(String responseBody); | ||
|
||
void onFailure(String errorMessage); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
package cloud.eppo; | ||
|
||
public interface EppoHttpClientRequestCallback { | ||
void onSuccess(String responseBody); | ||
|
||
void onFailure(String errorMessage); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,9 +2,9 @@ | |
|
||
public interface DiscriminableAttributes { | ||
|
||
public Attributes getNumericAttributes(); | ||
Attributes getNumericAttributes(); | ||
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. No need for |
||
|
||
public Attributes getCategoricalAttributes(); | ||
Attributes getCategoricalAttributes(); | ||
|
||
public Attributes getAllAttributes(); | ||
Attributes getAllAttributes(); | ||
} |
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.
Also providing a testJar artifact so upstream SDKs can use our handy test utilities. Sharing is caring!