-
Notifications
You must be signed in to change notification settings - Fork 41
feat: improve java Provider performance #928
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 |
|---|---|---|
|
|
@@ -5,8 +5,10 @@ | |
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
||
| import java.lang.ref.WeakReference; | ||
| import java.util.Optional; | ||
| import java.util.concurrent.*; | ||
| import java.util.concurrent.atomic.AtomicReference; | ||
| import java.util.function.Supplier; | ||
|
|
||
| interface RefreshJob<T> { | ||
|
|
@@ -19,46 +21,79 @@ interface RefreshJob<T> { | |
| final class Poll<T> implements RefreshJob<T> { | ||
| private final RefreshStrategy.Polling config; | ||
| private final Supplier<CompletableFuture<T>> action; | ||
| private final CompletableFuture<T> output; | ||
| private final Runnable onChange; | ||
| private final CompletableFuture<T> firstOutput; | ||
| private volatile T latestOutput = null; | ||
| private ScheduledFuture<?> poll; | ||
|
|
||
| Poll(RefreshStrategy.Polling config, Supplier<CompletableFuture<T>> action) { | ||
| Poll(RefreshStrategy.Polling config, Supplier<CompletableFuture<T>> action, Runnable onChange) { | ||
| this.config = config; | ||
| this.action = action; | ||
| this.output = new CompletableFuture<>(); | ||
| this.onChange = onChange; | ||
| this.firstOutput = new CompletableFuture<>(); | ||
| } | ||
|
|
||
| void start() { | ||
| log.debug("Starting polling-refresh."); | ||
| poll = SEXEC.schedule( | ||
| WeakReference<Poll<T>> weakSelf = new WeakReference<>(this); | ||
| AtomicReference<ScheduledFuture<?>> taskRef = new AtomicReference<>(); | ||
|
|
||
| ScheduledFuture<?> scheduled = SEXEC.scheduleAtFixedRate( | ||
| () -> { | ||
| var o = RefreshJob.runRefreshWithTimeout(action, config.timeout); | ||
| Poll<T> self = weakSelf.get(); | ||
| if (self == null) { | ||
| log.debug("Poll referent GC'd — self-cancelling polling task."); | ||
| ScheduledFuture<?> t = taskRef.get(); | ||
| if (t != null) t.cancel(false); | ||
| return; | ||
| } | ||
| var o = RefreshJob.runRefreshWithTimeout(self.action, self.config.timeout); | ||
| if (o != null) { | ||
| output.complete(o); | ||
| boolean changed = !o.equals(self.latestOutput); | ||
|
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. Generally the equals method provided by Object.class just does a referencial check (== or just checks if the pointer is the same), not a value check unless you define a custom implementation. Given T is generic, we should use some other way of checking if data has changed here
Collaborator
Author
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's always a Map<String, Object>), so this works here.
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. Can we scope it in such a way then if possible?
Collaborator
Author
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. pushed the change with custom equals() |
||
| self.latestOutput = o; | ||
| if (!self.firstOutput.isDone()) { | ||
| self.firstOutput.complete(o); | ||
| } | ||
| if (changed && self.onChange != null) { | ||
| try { | ||
| self.onChange.run(); | ||
| } catch (Exception e) { | ||
| log.error("onChange callback error: {}", e.getMessage()); | ||
| } | ||
| } else if (!changed) { | ||
| log.debug("Output unchanged, skipping onChange callback."); | ||
|
Comment on lines
+50
to
+64
Contributor
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. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -e
echo "== EvaluationArgs declaration =="
fd 'EvaluationArgs.kt$' --exec sed -n '20,80p' {}
echo
echo "== Provider creates a new EvaluationArgs for each config refresh =="
fd 'SuperpositionOpenFeatureProvider.java$' --exec rg -n 'thenApply\(EvaluationArgs::new\)' {}
echo
echo "== Poll change detector =="
fd 'RefreshJob.java$' --exec rg -n 'equals\(self\.latestOutput\)|latestOutput' {}Repository: juspay/superposition Length of output: 2994
To fix this, either make 🤖 Prompt for AI Agents |
||
| } | ||
| } | ||
| }, | ||
| 0, | ||
| config.interval, | ||
| TimeUnit.MILLISECONDS | ||
| ); | ||
|
|
||
| taskRef.set(scheduled); | ||
| this.poll = scheduled; | ||
| } | ||
|
|
||
| @Override | ||
| public Optional<T> getOutput() { | ||
| if (latestOutput != null) { | ||
| return Optional.of(latestOutput); | ||
| } | ||
| try { | ||
| if (poll == null) { | ||
| log.warn("Polling hasn't started but the output is being used."); | ||
| } else if (!poll.isCancelled() && !output.isDone()) { | ||
| return Optional.ofNullable(output.get(config.timeout, TimeUnit.MILLISECONDS)); | ||
| } else if (!firstOutput.isDone()) { | ||
| return Optional.ofNullable(firstOutput.get(config.timeout, TimeUnit.MILLISECONDS)); | ||
| } | ||
| } catch (Exception e) { | ||
| log.warn("Attempted to await for poll output but an exception occurred: {}", e.toString()); | ||
| } | ||
| return Optional.ofNullable(output.getNow(null)); | ||
| return Optional.ofNullable(latestOutput); | ||
| } | ||
|
|
||
| @Override | ||
| public void shutdown() { | ||
| if (!poll.isCancelled()) { | ||
| if (poll != null && !poll.isCancelled()) { | ||
| log.debug("Shutting down polling-refresh."); | ||
| poll.cancel(false); | ||
| } | ||
|
|
@@ -71,11 +106,13 @@ final class OnDemand<T> implements RefreshJob<T> { | |
| private T output = null; | ||
| private final RefreshStrategy.OnDemand config; | ||
| private final Supplier<CompletableFuture<T>> action; | ||
| private final Runnable onChange; | ||
| private boolean stopped = false; | ||
|
|
||
| OnDemand(RefreshStrategy.OnDemand config, Supplier<CompletableFuture<T>> action) { | ||
| OnDemand(RefreshStrategy.OnDemand config, Supplier<CompletableFuture<T>> action, Runnable onChange) { | ||
| this.config = config; | ||
| this.action = action; | ||
| this.onChange = onChange; | ||
| } | ||
|
|
||
| @Override | ||
|
|
@@ -85,8 +122,18 @@ public Optional<T> getOutput() { | |
| log.debug("Running refresh as current output is stale."); | ||
| var o = RefreshJob.runRefreshWithTimeout(action, config.timeout); | ||
| if (o != null) { | ||
| boolean changed = !o.equals(output); | ||
| output = o; | ||
| lastUpdated = System.currentTimeMillis(); | ||
| if (changed && onChange != null) { | ||
| try { | ||
| onChange.run(); | ||
| } catch (Exception e) { | ||
| log.error("onChange callback error: {}", e.getMessage()); | ||
| } | ||
| } else if (!changed) { | ||
| log.debug("Output unchanged, skipping onChange callback."); | ||
| } | ||
| } | ||
| } else { | ||
| log.debug("Current output is fresh, no refresh required."); | ||
|
|
@@ -114,10 +161,14 @@ private static<T> T runRefreshWithTimeout(Supplier<CompletableFuture<T>> action, | |
| } | ||
|
|
||
| static <T> RefreshJob<T> create(RefreshStrategy config, Supplier<CompletableFuture<T>> action) { | ||
| return create(config, action, null); | ||
| } | ||
|
|
||
| static <T> RefreshJob<T> create(RefreshStrategy config, Supplier<CompletableFuture<T>> action, Runnable onChange) { | ||
| if (config instanceof RefreshStrategy.Polling) { | ||
| return new Poll<>((RefreshStrategy.Polling)config, action); | ||
| return new Poll<>((RefreshStrategy.Polling)config, action, onChange); | ||
| } else if (config instanceof RefreshStrategy.OnDemand) { | ||
| return new OnDemand<>((RefreshStrategy.OnDemand)config, action); | ||
| return new OnDemand<>((RefreshStrategy.OnDemand)config, action, onChange); | ||
| } | ||
| throw new IllegalArgumentException("Invalid refresh-strategy: " + config); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -15,10 +15,15 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import uniffi.superposition_client.FfiExperiment; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import uniffi.superposition_client.FfiExperimentGroup; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import uniffi.superposition_client.OperationException; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import uniffi.superposition_client.ProviderCache; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import uniffi.superposition_types.MergeStrategy; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import java.util.List; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import java.util.Map; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import java.util.Optional; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import java.util.concurrent.CompletableFuture; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import java.util.concurrent.TimeUnit; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import java.util.concurrent.TimeoutException; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import java.util.stream.Collectors; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -65,11 +70,14 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public class SuperpositionOpenFeatureProvider implements FeatureProvider { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private static final Gson gson = new Gson(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private final SuperpositionAsyncClient sdk; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private final ProviderCache cache; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private final RefreshJob<EvaluationArgs> configRefresh; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private final Optional<RefreshJob<List<FfiExperiment>>> expRefresh; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private final Optional<RefreshJob<List<FfiExperimentGroup>>> expGroupRefresh; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private Optional<EvaluationContext> defaultCtx; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private final Optional<EvaluationArgs> fallbackArgs; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private final CompletableFuture<Void> cacheReady = new CompletableFuture<>(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private final int configTimeout; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public SuperpositionOpenFeatureProvider(@NonNull SuperpositionProviderOptions options) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (options.fallbackConfig != null) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -84,14 +92,18 @@ public SuperpositionOpenFeatureProvider(@NonNull SuperpositionProviderOptions op | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| builder.transport(options.transport); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| this.sdk = builder.build(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| this.cache = new ProviderCache(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| this.configTimeout = options.refreshStrategy.getTimeout(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| var getConfigInput = GetConfigInput.builder() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .context(Map.of()) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .orgId(options.orgId) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .workspaceId(options.workspaceId) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .build(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| this.configRefresh = RefreshJob.create( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| options.refreshStrategy, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| () -> sdk.getConfig(getConfigInput).thenApply(EvaluationArgs::new) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| () -> sdk.getConfig(getConfigInput).thenApply(EvaluationArgs::new), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| this::reinitConfigCache | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (options.experimentationOptions != null) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -109,7 +121,8 @@ public SuperpositionOpenFeatureProvider(@NonNull SuperpositionProviderOptions op | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .stream() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .map(EvaluationArgs.Helpers::toFfiExperiment) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .toList() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| )) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| this::reinitExperimentsCache) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // New logic for experiment_groups | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -126,7 +139,8 @@ public SuperpositionOpenFeatureProvider(@NonNull SuperpositionProviderOptions op | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .stream() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .map(EvaluationArgs.Helpers::toFfiExperimentGroup) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .toList() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| )) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| this::reinitExperimentsCache) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| this.expRefresh = Optional.empty(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -162,6 +176,9 @@ public void initialize(EvaluationContext eCtx) { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @Override | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public void shutdown() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| configRefresh.shutdown(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| expRefresh.ifPresent(RefreshJob::shutdown); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| expGroupRefresh.ifPresent(RefreshJob::shutdown); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cache.close(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @SneakyThrows | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -285,9 +302,19 @@ private EvaluationArgs getEvaluationArgs(EvaluationContext ctx) throws Exception | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private Map<String, String> evaluateConfigInternal(EvaluationContext ctx) throws Exception { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| EvaluationArgs args = getEvaluationArgs(ctx); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Block until cache.initConfig has been called (completed inside reinitConfigCache). | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // This guarantees cache.evalConfig never runs on an uninitialized cache. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!cacheReady.isDone()) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cacheReady.get(configTimeout, TimeUnit.MILLISECONDS); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } catch (TimeoutException e) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| throw new Exception("Config cache not initialized within timeout (" + configTimeout + "ms)."); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
304
to
+312
Contributor
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.
In 🤖 Prompt for AI Agents
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. Ankit can you check this?
Contributor
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. 🧩 Analysis chainLet me inspect the 🏁 Script executed: Length of output: 7032
🔴 Bug 1:
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| var ctx_ = defaultCtx.isPresent() ? ctx.merge(defaultCtx.get()) : ctx; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return args.evaluate(ctx_, getExperimentationArgs(ctx_)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| var queryData = EvaluationArgs.Companion.buildQueryData(ctx_); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| String targetingKey = ctx_.getTargetingKey(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return cache.evalConfig(queryData, MergeStrategy.MERGE, null, targetingKey); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
304
to
+317
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private List<String> getApplicableVariantsInternal(EvaluationContext ctx) throws Exception { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -314,4 +341,38 @@ private ExperimentationArgs getExperimentationArgs(EvaluationContext ctx) { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return null; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private void reinitConfigCache() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
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. why reinit? Similar question for experiments. Should we just call it refresh? What is the name used in other providers
Collaborator
Author
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. this reinitializes the config cache in rust |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| var out = configRefresh.getOutput(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (out.isPresent()) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| var args = out.get(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cache.initConfig( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| args.getDefaultConfig(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| args.getContexts(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| args.getOverrides(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| args.getDimensions() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cacheReady.complete(null); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| log.debug("Config cache re-initialized"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } catch (Exception e) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| log.error("Failed to reinitialize config cache: {}", e.getMessage()); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+347
to
+360
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (out.isPresent()) { | |
| var args = out.get(); | |
| try { | |
| cache.initConfig( | |
| args.getDefaultConfig(), | |
| args.getContexts(), | |
| args.getOverrides(), | |
| args.getDimensions() | |
| ); | |
| cacheReady.complete(null); | |
| log.debug("Config cache re-initialized"); | |
| } catch (Exception e) { | |
| log.error("Failed to reinitialize config cache: {}", e.getMessage()); | |
| } | |
| var args = out.orElse(fallbackArgs.orElse(null)); | |
| if (args == null) { | |
| log.warn("Config refresh output is unavailable and no fallback config is present."); | |
| return; | |
| } | |
| try { | |
| cache.initConfig( | |
| args.getDefaultConfig(), | |
| args.getContexts(), | |
| args.getOverrides(), | |
| args.getDimensions() | |
| ); | |
| cacheReady.complete(null); | |
| if (out.isPresent()) { | |
| log.debug("Config cache re-initialized"); | |
| } else { | |
| log.warn("Config refresh output unavailable; initialized config cache from fallback config."); | |
| } | |
| } catch (Exception e) { | |
| log.error("Failed to reinitialize config cache: {}", e.getMessage()); |
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.
Fallback config no longer seeds the cache.
If the first remote refresh never produces output, this method just returns without calling cache.initConfig(...) even when fallbackConfig was provided. The old evaluation path used fallbackArgs; the new cache path now drops straight to the default / PROVIDER_NOT_READY result instead.
🛠️ One way to preserve the old fallback behavior
private void reinitConfigCache() {
- var out = configRefresh.getOutput();
- if (out.isPresent()) {
- var args = out.get();
+ var args = configRefresh.getOutput().orElse(fallbackArgs.orElse(null));
+ if (args != null) {
try {
cache.initConfig(
args.getDefaultConfig(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/SuperpositionOpenFeatureProvider.java`
around lines 342 - 358, reinitConfigCache() currently returns without seeding
the cache when configRefresh.getOutput() is empty, so ensure we fall back to the
provided fallback config: if out.isEmpty() and a fallbackConfig (or the
previously used fallbackArgs) is present, call cache.initConfig(...) with the
fallback default config, contexts, overrides and dimensions, then complete
cacheReady and log the reinitialization; keep the existing try/catch and error
logging around cache.initConfig() so behavior on exceptions remains unchanged.
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.
Why do we need both of these?
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.
So latestOutput is the steady-state store and firstOutput is purely the one-time synchronization primitive for that cold-start window. We could replace both with a single AtomicReference<CompletableFuture> (replacing it on each poll with an already-completed future), but that would allocate a new CompletableFuture on every poll cycle. The current change is more efficient.