feat: improve java Provider performance#928
Conversation
|
Review changes with Changed Files |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThese changes introduce an Changes
Sequence DiagramsequenceDiagram
participant Client
participant Provider as SuperpositionOpenFeatureProvider
participant RefreshJob
participant Cache as ProviderCache
participant ExternalAPI as Config/Experiment Source
Client->>Provider: evaluateConfigInternal(args)
Provider->>Provider: Wait for cacheReady with timeout
rect rgba(100, 150, 255, 0.5)
Note over RefreshJob,Cache: Background Refresh Cycle
RefreshJob->>ExternalAPI: Poll/OnDemand fetch
ExternalAPI-->>RefreshJob: Updated config/experiment
RefreshJob->>Provider: Invoke onChange callback
Provider->>Cache: reinitConfigCache() or reinitExperimentsCache()
Cache->>Cache: initConfig() / initExperiments()
Provider->>Provider: Complete cacheReady
end
Provider->>Cache: evalConfig(queryData, MERGE, key)
Cache-->>Provider: Evaluation result
Provider-->>Client: Return feature flag value
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
mahatoankitkumar
force-pushed
the
java-performance-enhancement
branch
2 times, most recently
from
097d6d1 to
ca035c4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/RefreshJob.java (1)
124-139:⚠️ Potential issue | 🔴 CriticalReverse the staleness check before invoking
onChange.Line 127 uses
lastUpdated - System.currentTimeMillis() < config.ttl, which is true immediately after a refresh. With the new callback,onChangere-entersgetOutput()viareinitConfigCache()/reinitExperimentsCache(), so on-demand reads can recurse into repeated refreshes instead of returning the cached value.🐛 Minimal fix
- if (lastUpdated - System.currentTimeMillis() < config.ttl) { + if (System.currentTimeMillis() - lastUpdated > config.ttl) {🤖 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/RefreshJob.java` around lines 124 - 139, The staleness check in RefreshJob.getOutput() is inverted and becomes true immediately after a refresh, causing onChange (which re-enters getOutput() via reinitConfigCache/reinitExperimentsCache) to trigger repeated refreshes; replace the incorrect condition `lastUpdated - System.currentTimeMillis() < config.ttl` with a proper staleness check like `System.currentTimeMillis() - lastUpdated >= config.ttl` so RefreshJob.runRefreshWithTimeout(action, config.timeout) only runs when the cached output is actually stale and prevents reentrant refresh loops triggered by onChange.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/RefreshJob.java`:
- Around line 56-70: The equality check on the refreshed config uses
EvaluationArgs instances created each poll (via configRefresh ->
thenApply(EvaluationArgs::new)), so equals() falls back to reference identity
and triggers onChange every time; fix by making EvaluationArgs a Kotlin data
class or add proper equals() and hashCode() implementations that compare the
core fields (defaultConfig, contexts, overrides, dimensions) so that
RefreshJob's changed detection (comparing o with self.latestOutput in
runRefreshWithTimeout / RefreshJob) correctly detects real changes.
In
`@clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/SuperpositionOpenFeatureProvider.java`:
- Around line 301-309: The evaluateConfigInternal method blocks on cacheReady
before any trigger when using RefreshStrategy.OnDemand, causing on-demand
providers to always timeout; modify the logic so OnDemand primes the cache
before waiting: if refreshStrategy == RefreshStrategy.OnDemand (or equivalent),
call reinitConfigCache() or invoke/subscribe to configRefresh.getOutput() to
trigger the onChange callback (which initializes cache and completes cacheReady)
prior to calling cacheReady.get(...), otherwise keep the existing wait
path—update references in evaluateConfigInternal, reinitConfigCache, and where
configRefresh.getOutput() is used so on-demand evaluation lazily primes the
cache rather than timing out.
- Around line 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.
---
Outside diff comments:
In
`@clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/RefreshJob.java`:
- Around line 124-139: The staleness check in RefreshJob.getOutput() is inverted
and becomes true immediately after a refresh, causing onChange (which re-enters
getOutput() via reinitConfigCache/reinitExperimentsCache) to trigger repeated
refreshes; replace the incorrect condition `lastUpdated -
System.currentTimeMillis() < config.ttl` with a proper staleness check like
`System.currentTimeMillis() - lastUpdated >= config.ttl` so
RefreshJob.runRefreshWithTimeout(action, config.timeout) only runs when the
cached output is actually stale and prevents reentrant refresh loops triggered
by onChange.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ec11f31d-21d0-4e0c-9699-0eb95946d50a
📒 Files selected for processing (3)
clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/RefreshJob.javaclients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/SuperpositionOpenFeatureProvider.javaclients/java/openfeature-provider/src/main/kotlin/io/juspay/superposition/openfeature/EvaluationArgs.kt
| var o = RefreshJob.runRefreshWithTimeout(self.action, self.config.timeout); | ||
| if (o != null) { | ||
| output.complete(o); | ||
| boolean changed = !o.equals(self.latestOutput); | ||
| 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."); |
There was a problem hiding this comment.
🧩 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
equals() falls back to reference identity for config change detection.
EvaluationArgs is a regular Kotlin class (not a data class), and configRefresh creates a fresh instance on every poll via thenApply(EvaluationArgs::new). The comparison at line 58 therefore always returns false, causing onChange to fire every refresh cycle regardless of whether the actual config changed.
To fix this, either make EvaluationArgs a data class or implement equals() and hashCode() based on the struct fields (defaultConfig, contexts, overrides, dimensions).
🤖 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/RefreshJob.java`
around lines 56 - 70, The equality check on the refreshed config uses
EvaluationArgs instances created each poll (via configRefresh ->
thenApply(EvaluationArgs::new)), so equals() falls back to reference identity
and triggers onChange every time; fix by making EvaluationArgs a Kotlin data
class or add proper equals() and hashCode() implementations that compare the
core fields (defaultConfig, contexts, overrides, dimensions) so that
RefreshJob's changed detection (comparing o with self.latestOutput in
runRefreshWithTimeout / RefreshJob) correctly detects real changes.
| 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)."); | ||
| } |
There was a problem hiding this comment.
cacheReady is waiting on a callback that on-demand refresh never fires.
In RefreshStrategy.OnDemand, reinitConfigCache() only runs as the onChange callback from configRefresh.getOutput(). This method blocks on cacheReady before any getOutput() call, so on-demand providers will time out on every evaluation instead of lazily priming the cache.
🤖 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 301 - 309, The evaluateConfigInternal method blocks on cacheReady
before any trigger when using RefreshStrategy.OnDemand, causing on-demand
providers to always timeout; modify the logic so OnDemand primes the cache
before waiting: if refreshStrategy == RefreshStrategy.OnDemand (or equivalent),
call reinitConfigCache() or invoke/subscribe to configRefresh.getOutput() to
trigger the onChange callback (which initializes cache and completes cacheReady)
prior to calling cacheReady.get(...), otherwise keep the existing wait
path—update references in evaluateConfigInternal, reinitConfigCache, and where
configRefresh.getOutput() is used so on-demand evaluation lazily primes the
cache rather than timing out.
There was a problem hiding this comment.
🧩 Analysis chain
Let me inspect the RefreshJob.java implementation to verify when onChange fires in OnDemand mode.
🏁 Script executed:
#!/bin/bash
cat clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/RefreshJob.java
Length of output: 7032
@Datron — the original review comment is confirmed correct. Here's a detailed breakdown of both issues found:
🔴 Bug 1: OnDemand mode always times out (the flagged issue)
In OnDemand mode, cacheReady is never completed because:
cacheReady.complete(null)is only called insidereinitConfigCache().reinitConfigCache()is only called as theonChangecallback.onChangeis only called insideOnDemand.getOutput()after a successful refresh.- But
evaluateConfigInternalblocks oncacheReadybefore ever callingconfigRefresh.getOutput(), sogetOutput()is never triggered,onChangenever fires, and the wait always hits the timeout.
Fix for evaluateConfigInternal: trigger an eager refresh for OnDemand before blocking:
private Map<String, String> evaluateConfigInternal(EvaluationContext ctx) throws Exception {
if (!cacheReady.isDone()) {
// For OnDemand, no background loop fires onChange; we must trigger getOutput()
// explicitly so the chain: refresh → onChange → reinitConfigCache → cacheReady.complete() runs.
if (!(configRefresh instanceof RefreshJob.Poll)) {
configRefresh.getOutput();
}
try {
cacheReady.get(configTimeout, TimeUnit.MILLISECONDS);
} catch (TimeoutException e) {
throw new Exception("Config cache not initialized within timeout (" + configTimeout + "ms).");
}
}
var ctx_ = defaultCtx.isPresent() ? ctx.merge(defaultCtx.get()) : ctx;
var queryData = EvaluationArgs.Companion.buildQueryData(ctx_);
String targetingKey = ctx_.getTargetingKey();
return cache.evalConfig(queryData, MergeStrategy.MERGE, null, targetingKey);
}🔴 Bug 2: OnDemand TTL condition is inverted (bonus)
In RefreshJob.java, the staleness check in OnDemand.getOutput() is:
// Line ~119 — condition is inverted
if (lastUpdated - System.currentTimeMillis() < config.ttl) {- When
lastUpdated = 0(initial):0 - huge_positive < ttl→ alwaystrue✅ (first call works) - After first update where
lastUpdated ≈ currentTimeMillis():≈0 < ttl→ still alwaystrue❌
This means every single getOutput() call fires a network request, completely defeating the TTL caching. The correct condition should be:
// Refresh only when data is actually stale
if (System.currentTimeMillis() - lastUpdated > config.ttl) {Both bugs should be addressed together. Bug 2 also has a secondary effect on Bug 1's fix — without the TTL fix, the reinitConfigCache() callback (called from within onChange) will trigger a second unnecessary network call via configRefresh.getOutput() before completing cacheReady.
🧠 Learnings used
Learnt from: ayushjain17
Repo: juspay/superposition PR: 951
File: crates/superposition_provider/src/local_provider.rs:271-336
Timestamp: 2026-04-01T17:53:15.684Z
Learning: In `crates/superposition_provider/src/local_provider.rs`, the `do_refresh()` method intentionally allows `cached_config` and `cached_experiments` to be updated independently (potentially out of sync). Temporary divergence between the two caches is an accepted trade-off by design — do not flag partial/non-atomic cache updates in this file as an issue.
| private void reinitConfigCache() { | ||
| 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()); | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
ca035c4 to
9dabd43
Compare
mahatoankitkumar
force-pushed
the
java-performance-enhancement
branch
from
9dabd43 to
799d628
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces a cache-backed evaluation path for the Java OpenFeature provider to reduce per-evaluation work by initializing a ProviderCache on refresh and using it for config evaluation.
Changes:
- Add a Java-callable helper in
EvaluationArgsto build query data from anEvaluationContext. - Introduce
ProviderCacheinitialization/re-initialization hooks tied to refresh jobs, and switch config evaluation to usecache.evalConfig(...). - Extend
RefreshJobto support anonChangecallback and track first/latest outputs for more efficient refresh handling.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
clients/java/openfeature-provider/src/main/kotlin/io/juspay/superposition/openfeature/EvaluationArgs.kt |
Exposes query-data builder for Java usage. |
clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/SuperpositionOpenFeatureProvider.java |
Adds ProviderCache and switches evaluation to cache-based path with cache init hooks. |
clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/RefreshJob.java |
Adds onChange callback support; refactors polling output handling; updates on-demand output logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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)."); | ||
| } | ||
| } | ||
| 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); |
There was a problem hiding this comment.
evaluateConfigInternal now waits on cacheReady, but nothing triggers a refresh/initialization when refreshStrategy is OnDemand (since initialize() only starts polling jobs and evaluation no longer calls configRefresh.getOutput()). This means cacheReady may never complete and all evaluations will time out. Consider explicitly triggering a config refresh for OnDemand (e.g., call configRefresh.getOutput() / getEvaluationArgs() before awaiting) and initializing the cache from that output.
| 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()); | ||
| } |
There was a problem hiding this comment.
The new cache-based evaluation path no longer uses fallbackArgs: if the initial config refresh fails, reinitConfigCache() won't run cache.initConfig(...), cacheReady won't complete, and evaluation will fail with a timeout even though a fallback config is available. Consider initializing the cache from fallbackArgs when refresh output is empty (especially on first run), and/or completing cacheReady based on fallback initialization.
| 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()); |
| public Optional<T> getOutput() { | ||
| if (!stopped) { | ||
| if (lastUpdated - System.currentTimeMillis() < config.ttl) { | ||
| log.debug("Running refresh as current output is stale."); | ||
| var o = RefreshJob.runRefreshWithTimeout(action, config.timeout); |
There was a problem hiding this comment.
The TTL freshness check is inverted: if (lastUpdated - System.currentTimeMillis() < config.ttl) will almost always be true after the first refresh (0 < ttl), causing a refresh on every getOutput() call instead of only when the cached value is stale. This should compare the age (System.currentTimeMillis() - lastUpdated) against ttl (and likely use > to determine staleness).
| 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)."); | ||
| } | ||
| } | ||
| 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); |
There was a problem hiding this comment.
This change introduces new behavior around cache initialization (blocking on cacheReady and using ProviderCache.evalConfig(...)), but the existing tests only cover the polling path and don't exercise OnDemand refresh or fallback-config initialization. Adding tests for (1) OnDemand strategy successfully evaluating without timing out and (2) fallback config being used when initial refresh fails would help prevent regressions.
Datron
left a comment
Datron
left a comment
There was a problem hiding this comment.
Can you check if there is a better way to check equality between output and latestOutput?
| return null; | ||
| } | ||
|
|
||
| private void reinitConfigCache() { |
There was a problem hiding this comment.
why reinit? Similar question for experiments. Should we just call it refresh? What is the name used in other providers
There was a problem hiding this comment.
this reinitializes the config cache in rust
refresh are there for fetching it from the server for configs and exps
will make it confusing to have refresh here, and yes its reinit everywhere else.
| 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)."); | ||
| } |
| var o = RefreshJob.runRefreshWithTimeout(self.action, self.config.timeout); | ||
| if (o != null) { | ||
| output.complete(o); | ||
| boolean changed = !o.equals(self.latestOutput); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
it's always a Map<String, Object>), so this works here.
There was a problem hiding this comment.
Can we scope it in such a way then if possible?
There was a problem hiding this comment.
pushed the change with custom equals()
| private final CompletableFuture<T> firstOutput; | ||
| private volatile T latestOutput = null; |
There was a problem hiding this comment.
Why do we need both of these?
There was a problem hiding this comment.
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.
mahatoankitkumar
force-pushed
the
java-performance-enhancement
branch
from
799d628 to
78a13f8
Compare
Problem
Describe the problem you are trying to solve here
Solution
Provide a brief summary of your solution so that reviewers can understand your code
Environment variable changes
What ENVs need to be added or changed
Pre-deployment activity
Things needed to be done before deploying this change (if any)
Post-deployment activity
Things needed to be done after deploying this change (if any)
API changes
Possible Issues in the future
Describe any possible issues that could occur because of this change