feat(multiprovider): add ComparisonStrategy - #2003
Conversation
📝 WalkthroughWalkthrough
ChangesComparison strategy
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Strategy as ComparisonStrategy
participant Executor as ExecutorService
participant Providers as FeatureProviders
participant Callback as MismatchCallback
Strategy->>Executor: submit provider evaluations
Executor->>Providers: evaluate feature key
Providers-->>Executor: return ProviderEvaluation
Executor-->>Strategy: return provider results
Strategy->>Callback: notify when successful values differ
Strategy-->>Strategy: return fallback evaluation
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2003 +/- ##
============================================
+ Coverage 92.52% 93.10% +0.58%
- Complexity 728 764 +36
============================================
Files 60 61 +1
Lines 1739 1857 +118
Branches 202 221 +19
============================================
+ Hits 1609 1729 +120
+ Misses 80 78 -2
Partials 50 50
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
62b831a to
fb8fae0
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: 2
🧹 Nitpick comments (1)
src/test/java/dev/openfeature/sdk/multiprovider/ComparisonStrategyTest.java (1)
187-197: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the callback flag key.
This test verifies evaluation order but does not verify
key. A regression that passesnullor another provider-derived value will pass this test.Proposed test update
AtomicReference<Map<String, ProviderEvaluation<?>>> captured = new AtomicReference<>(); + AtomicReference<String> capturedKey = new AtomicReference<>(); ComparisonStrategy strategy = - new ComparisonStrategy("provider2", (key, evaluations) -> captured.set(evaluations)); + new ComparisonStrategy("provider2", (key, evaluations) -> { + capturedKey.set(key); + captured.set(evaluations); + }); strategy.evaluate( providers, FLAG_KEY, DEFAULT_STRING, null, p -> p.getStringEvaluation(FLAG_KEY, DEFAULT_STRING, null)); assertNotNull(captured.get()); + assertEquals(FLAG_KEY, capturedKey.get()); assertEquals( List.of("provider1", "provider2"), List.copyOf(captured.get().keySet()));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/dev/openfeature/sdk/multiprovider/ComparisonStrategyTest.java` around lines 187 - 197, Extend the callback capture in ComparisonStrategyTest to record the key argument alongside evaluations, then assert it equals FLAG_KEY after strategy.evaluate. Keep the existing provider-order assertion unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/dev/openfeature/sdk/multiprovider/ComparisonStrategy.java`:
- Around line 165-170: Update the timeout handling in ComparisonStrategy to
associate each submitted Future with its provider name, iterate all completed
futures, and record a timeout ProviderError for every cancelled future before
constructing the aggregate errorResult. Avoid returning on the first
cancellation, and extend shouldReturnTimeoutErrorWhenProvidersExceedTheTimeout
to verify each timed-out provider entry.
- Around line 67-70: Update the default ComparisonStrategy constructor to use a
dedicated executor instead of ForkJoinPool.commonPool() for provider
evaluations, and ensure that executor has an explicit lifecycle consistent with
the class’s cleanup behavior. Preserve the existing fallback provider, mismatch
handler, and timeout configuration.
---
Nitpick comments:
In `@src/test/java/dev/openfeature/sdk/multiprovider/ComparisonStrategyTest.java`:
- Around line 187-197: Extend the callback capture in ComparisonStrategyTest to
record the key argument alongside evaluations, then assert it equals FLAG_KEY
after strategy.evaluate. Keep the existing provider-order assertion unchanged.
🪄 Autofix
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 Plus
Run ID: bedde7d4-d623-4dd3-ad61-8c83d7aa3ca1
📒 Files selected for processing (2)
src/main/java/dev/openfeature/sdk/multiprovider/ComparisonStrategy.javasrc/test/java/dev/openfeature/sdk/multiprovider/ComparisonStrategyTest.java
| public ComparisonStrategy( | ||
| String fallbackProvider, BiConsumer<String, Map<String, ProviderEvaluation<?>>> onMismatch) { | ||
| this(fallbackProvider, onMismatch, ForkJoinPool.commonPool(), DEFAULT_TIMEOUT_MS); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the strategy structure and locate default-constructor call sites.
ast-grep outline src/main/java/dev/openfeature/sdk/multiprovider/ComparisonStrategy.java --items all
rg -nP --type java 'new\s+ComparisonStrategy\s*\(' src
# Inspect declared Java/build toolchain files before selecting an executor lifecycle design.
fd -HI -t f '^(pom\.xml|build\.gradle(\.kts)?|\.tool-versions|\.java-version)$' . \
-x sh -c 'echo "=== $1 ==="; sed -n "1,220p" "$1"' sh {}Repository: open-feature/java-sdk
Length of output: 10692
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant ComparisonStrategy implementation and public APIs that document or expose evaluation behavior.
for f in src/main/java/dev/openfeature/sdk/multiprovider/ComparisonStrategy.java \
src/main/java/dev/openfeature/sdk/multiprovider/*.java \
src/main/java/dev/openfeature/sdk/multiprovider/**/*.java; do
if [ -f "$f" ]; then echo "=== $f ==="; wc -l "$f"; done
done
printf '\n--- ComparisonStrategy.java ---\n'
sed -n '1,260p' src/main/java/dev/openfeature/sdk/multiprovider/ComparisonStrategy.java
printf '\n--- Strategy.java (if exists) ---\n'
if [ -f src/main/java/dev/openfeature/sdk/multiprovider/Strategy.java ]; then sed -n '1,220p' src/main/java/dev/openfeature/sdk/multiprovider/Strategy.java; fi
printf '\n--- All references to ComparisonStrategy / parallel evaluation words ---\n'
rg -n "ComparisonStrategy|parallel|executor|thread|blocking|runEvaluations|evaluate|fallbackProvider|DEFAULT_TIMEOUT" src/main/javaRepository: open-feature/java-sdk
Length of output: 215
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- ComparisonStrategy.java ---'
sed -n '1,280p' src/main/java/dev/openfeature/sdk/multiprovider/ComparisonStrategy.java
printf '%s\n' '--- all Multiprovider files under src/main/java/dev/openfeature/sdk/multiprovider ---'
find src/main/java/dev/openfeature/sdk/multiprovider -maxdepth 2 -type f -print | sort -u
printf '%s\n' '--- references to ComparisonStrategy / parallel/executor/thread words ---'
rg -n "ComparisonStrategy|parallel|executor|Executor|thread|Thread|blocking|runEvaluations|evaluate|fallbackProvider|DEFAULT_TIMEOUT|ForkJoinPool" src/main/javaRepository: open-feature/java-sdk
Length of output: 32431
🌐 Web query:
JDK ForkJoinPool commonPool parallelism default documentation
💡 Result:
In the JDK, the ForkJoinPool.commonPool parallelism level defaults to Runtime.availableProcessors minus 1 [1]. If the system has only one processor, the default parallelism is 1 [1]. The parallelism level can be customized or overridden by setting the system property java.util.concurrent.ForkJoinPool.common.parallelism [2][3][4]. The property must be a non-negative integer [2][3][4]. Key details regarding the common pool parallelism include: - The default formula aims to leave at least one processor available for the calling thread, unless only one processor is available [1]. - The setting can be checked programmatically using the static method ForkJoinPool.getCommonPoolParallelism [5]. - While the default is calculated based on available processors, it is specifically designed to support scenarios like parallel streams [1]. If the property is set to 0, the pool will effectively run tasks on the calling thread [1].
Citations:
- 1: https://github.com/openjdk/jdk/blob/c419dda4e99c3b72fbee95b93159db2e23b994b6/src/java.base/share/classes/java/util/concurrent/ForkJoinPool.java
- 2: https://docs.oracle.com/en/java/javase/24/docs/api/java.base/java/util/concurrent/ForkJoinPool.html
- 3: https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/util/concurrent/ForkJoinPool.html
- 4: https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/util/concurrent/ForkJoinPool.html
- 5: https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/ForkJoinPool.html
Use a dedicated executor for default provider evaluation.
This strategy submits provider evaluations to ForkJoinPool.commonPool(). Provider evaluations can run longer tasks, and the common pool may serialize them when its parallelism is constrained. This violates the documented parallel evaluation behavior and can starve unrelated common-pool work.
Use a dedicated executor with an explicit lifecycle in the default constructor, or do not run evaluations on ForkJoinPool.commonPool().
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/dev/openfeature/sdk/multiprovider/ComparisonStrategy.java`
around lines 67 - 70, Update the default ComparisonStrategy constructor to use a
dedicated executor instead of ForkJoinPool.commonPool() for provider
evaluations, and ensure that executor has an explicit lifecycle consistent with
the class’s cleanup behavior. Preserve the existing fallback provider, mismatch
handler, and timeout configuration.
| List<Future<Void>> futures = executorService.invokeAll(tasks, timeoutMs, TimeUnit.MILLISECONDS); | ||
| for (Future<Void> future : futures) { | ||
| if (future.isCancelled()) { | ||
| return Optional.of(errorResult( | ||
| "Comparison strategy timed out after " + timeoutMs + "ms", providers, providerErrors)); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Record a ProviderError for every timed-out provider.
Lines 165-170 return on the first cancelled Future. They do not create ProviderError entries for cancelled providers. A timeout result can therefore contain no structured provider errors.
Associate each submitted task with its provider name. Mark every cancelled future as a timeout error before building the aggregate result. Extend shouldReturnTimeoutErrorWhenProvidersExceedTheTimeout to assert the timed-out provider entries.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/dev/openfeature/sdk/multiprovider/ComparisonStrategy.java`
around lines 165 - 170, Update the timeout handling in ComparisonStrategy to
associate each submitted Future with its provider name, iterate all completed
futures, and record a timeout ProviderError for every cancelled future before
constructing the aggregate errorResult. Avoid returning on the first
cancellation, and extend shouldReturnTimeoutErrorWhenProvidersExceedTheTimeout
to verify each timed-out provider entry.
There was a problem hiding this comment.
Pull request overview
Adds parallel multi-provider result comparison with configurable fallback, mismatch handling, timeouts, and structured errors.
Changes:
- Adds
ComparisonStrategy. - Adds comprehensive behavior and concurrency tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
ComparisonStrategy.java |
Implements parallel comparison and fallback resolution. |
ComparisonStrategyTest.java |
Tests agreement, mismatch, failures, ordering, concurrency, and timeouts. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| List<Callable<Void>> tasks = new ArrayList<>(providers.size()); | ||
| for (Map.Entry<String, FeatureProvider> entry : providers.entrySet()) { | ||
| String providerName = entry.getKey(); | ||
| FeatureProvider provider = entry.getValue(); | ||
| tasks.add(() -> { | ||
| recordEvaluation(providerName, provider, providerFunction, successfulResults, providerErrors); | ||
| return null; | ||
| }); | ||
| } | ||
| List<Future<Void>> futures = executorService.invokeAll(tasks, timeoutMs, TimeUnit.MILLISECONDS); | ||
| for (Future<Void> future : futures) { | ||
| if (future.isCancelled()) { | ||
| return Optional.of(errorResult( | ||
| "Comparison strategy timed out after " + timeoutMs + "ms", providers, providerErrors)); | ||
| } | ||
| future.get(); | ||
| } | ||
| return Optional.empty(); |
Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
|
Summary
ComparisonStrategy, which evaluates every configured provider in parallel and compares the results.ProviderErrorentries on the returned evaluation.Usage
Notes
Providers are evaluated in parallel on a dedicated daemon thread pool, deliberately not
ForkJoinPool.commonPool(), since provider calls block and would otherwise starve unrelated parallel work in the host application.The public constructors mirror the js-sdk reference implementation. The executor and the 30s timeout are internal details rather than API.
A provider that doesn't respond in time does not fail the evaluation: the fallback provider's result is returned with a
ProviderErrordescribing the timeout. Only a timeout of the fallback provider itself yields an error result.Related PRs
This is one of three independent PRs that together close the multi-provider gaps identified in #1882. They branch off
mainseparately and can be reviewed and merged in any order. Together they replace #1897.ComparisonStrategy(this PR)Relates to #1882