feat: add Scatter-Gather pattern (#3577) - #3601
Conversation
PR SummaryIntroduced the Scatter-Gather pattern as a new module. The solution scatters a request to multiple providers concurrently, gathers replies within a bounded timeout, and aggregates the results while tolerating slow or failing providers. Includes fast, slow, and failing provider variants, an Aggregator strategy, an App demonstration, and thorough tests. Updated the parent pom to include the new module and added README/diagrams for guidance. Changes
autogenerated by presubmit.ai |
There was a problem hiding this comment.
🚨 Pull request needs attention.
Review Summary
Files Processed (17)
- pom.xml (1 hunk)
- scatter-gather/README.md (1 hunk)
- scatter-gather/etc/scatter-gather.urm.puml (1 hunk)
- scatter-gather/pom.xml (1 hunk)
- scatter-gather/src/main/java/com/iluwatar/scattergather/Aggregator.java (1 hunk)
- scatter-gather/src/main/java/com/iluwatar/scattergather/App.java (1 hunk)
- scatter-gather/src/main/java/com/iluwatar/scattergather/DelayedRateProvider.java (1 hunk)
- scatter-gather/src/main/java/com/iluwatar/scattergather/FailingRateProvider.java (1 hunk)
- scatter-gather/src/main/java/com/iluwatar/scattergather/InMemoryRateProvider.java (1 hunk)
- scatter-gather/src/main/java/com/iluwatar/scattergather/RateProvider.java (1 hunk)
- scatter-gather/src/main/java/com/iluwatar/scattergather/RateQuote.java (1 hunk)
- scatter-gather/src/main/java/com/iluwatar/scattergather/RateRequest.java (1 hunk)
- scatter-gather/src/main/java/com/iluwatar/scattergather/ScatterGather.java (1 hunk)
- scatter-gather/src/test/java/com/iluwatar/scattergather/AggregatorTest.java (1 hunk)
- scatter-gather/src/test/java/com/iluwatar/scattergather/AppTest.java (1 hunk)
- scatter-gather/src/test/java/com/iluwatar/scattergather/RateProviderTest.java (1 hunk)
- scatter-gather/src/test/java/com/iluwatar/scattergather/ScatterGatherTest.java (1 hunk)
Actionable Comments (6)
-
scatter-gather/src/main/java/com/iluwatar/scattergather/App.java [66-66]
bug: "Logger instance naming mismatch with Lombok"
-
scatter-gather/src/main/java/com/iluwatar/scattergather/App.java [69-69]
bug: "Logger usage mismatch in gather log line"
-
scatter-gather/src/main/java/com/iluwatar/scattergather/App.java [72-72]
bug: "Logger usage mismatch in aggregate log line"
-
scatter-gather/src/main/java/com/iluwatar/scattergather/DelayedRateProvider.java [60-60]
bug: "Incorrect duration handling in Delay provider"
-
scatter-gather/src/main/java/com/iluwatar/scattergather/InMemoryRateProvider.java [56-56]
bug: "Logger naming inconsistency in InMemoryRateProvider"
-
scatter-gather/src/main/java/com/iluwatar/scattergather/ScatterGather.java [83-87]
bug: "Logger naming mismatch in ScatterGather.scatter"
Skipped Comments (0)
|
Note on the automated review comments: |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3601 +/- ##
============================================
+ Coverage 83.79% 83.86% +0.07%
- Complexity 4277 4311 +34
============================================
Files 1121 1129 +8
Lines 15144 15257 +113
Branches 723 728 +5
============================================
+ Hits 12690 12796 +106
- Misses 2159 2163 +4
- Partials 295 298 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
31d348c to
2df15af
Compare
|
Follow-up on the Codecov note: added two |
2df15af to
c4563e9
Compare
|
Coverage follow-up: moved the result reporting in |
There was a problem hiding this comment.
🚨 Pull request needs attention.
Review Summary
Commits Considered (4)
- bc38fb3: docs: embed the rendered class diagram in the scatter-gather README
Render etc/scatter-gather.urm.puml to PNG and embed it in the detailed explanation section, matching the other modules, instead of the inline mermaid block.
- ff263fb: refactor: simplify aggregator generics and make shutdown grace configurable
Aggregator only ever reduced lists of RateQuote, so the reply type parameter carried no information. Drop it and let the interface be Aggregator over List; update ScatterGather, the README snippets and the PUML diagram to match.
Also lift the hard-coded one second awaitTermination in close() into a field. The public constructor keeps the one second default; a package-private overload takes an explicit grace period so the test that closes a coordinator whose task ignores interrupts no longer pays a full second.
-
ea26143: fix: handle cancelled replies and correct timeout docs in scatter-gather
-
gather() now catches CancellationException so a cancelled pending reply is
dropped like a failed one instead of escaping to the caller, with a test
that cancels one reply and asserts the remaining quotes are still gathered -
class javadoc states that a timed-out provider's task keeps running on its
pool thread until it finishes or the coordinator is closed -
timeout parameter documented as starting at scatter time, in the javadoc and
in the README -
class diagram: ScatterGather depends on RateProvider instead of holding a
collection of them, and PendingReply is shown as a nested type -
README embeds the class diagram as mermaid instead of linking the PlantUML file
Files Processed (18)
- pom.xml (1 hunk)
- scatter-gather/README.md (1 hunk)
- scatter-gather/etc/scatter-gather.urm.png (0 hunks)
- scatter-gather/etc/scatter-gather.urm.puml (1 hunk)
- scatter-gather/pom.xml (1 hunk)
- scatter-gather/src/main/java/com/iluwatar/scattergather/Aggregator.java (1 hunk)
- scatter-gather/src/main/java/com/iluwatar/scattergather/App.java (1 hunk)
- scatter-gather/src/main/java/com/iluwatar/scattergather/DelayedRateProvider.java (1 hunk)
- scatter-gather/src/main/java/com/iluwatar/scattergather/FailingRateProvider.java (1 hunk)
- scatter-gather/src/main/java/com/iluwatar/scattergather/InMemoryRateProvider.java (1 hunk)
- scatter-gather/src/main/java/com/iluwatar/scattergather/RateProvider.java (1 hunk)
- scatter-gather/src/main/java/com/iluwatar/scattergather/RateQuote.java (1 hunk)
- scatter-gather/src/main/java/com/iluwatar/scattergather/RateRequest.java (1 hunk)
- scatter-gather/src/main/java/com/iluwatar/scattergather/ScatterGather.java (1 hunk)
- scatter-gather/src/test/java/com/iluwatar/scattergather/AggregatorTest.java (1 hunk)
- scatter-gather/src/test/java/com/iluwatar/scattergather/AppTest.java (1 hunk)
- scatter-gather/src/test/java/com/iluwatar/scattergather/RateProviderTest.java (1 hunk)
- scatter-gather/src/test/java/com/iluwatar/scattergather/ScatterGatherTest.java (1 hunk)
Actionable Comments (5)
-
scatter-gather/src/main/java/com/iluwatar/scattergather/App.java [67-75]
maintainability: "Logger name mismatch with Lombok @slf4j"
-
scatter-gather/src/main/java/com/iluwatar/scattergather/InMemoryRateProvider.java [56-56]
maintainability: "Logger usage with Lombok"
-
scatter-gather/src/main/java/com/iluwatar/scattergather/DelayedRateProvider.java [58-58]
maintainability: "Logger usage with Lombok"
-
scatter-gather/src/main/java/com/iluwatar/scattergather/DelayedRateProvider.java [60-60]
possible bug: "Thread.sleep with Duration"
-
scatter-gather/src/main/java/com/iluwatar/scattergather/ScatterGather.java [103-107]
maintainability: "Logger usage with Lombok"
Skipped Comments (0)
A reply that times out or is cancelled cancels its task, gather tolerates cancelled replies, Aggregator loses its unused type parameter and the shutdown grace is configurable for tests. The class diagram is a rendered PNG.
8580c13 to
5fab90e
Compare
There was a problem hiding this comment.
🚨 Pull request needs attention.
Review Summary
Commits Considered (1)
- 5fab90e: fix: cancel timed-out provider calls and simplify the aggregator
A reply that times out or is cancelled cancels its task, gather tolerates cancelled replies, Aggregator loses its unused type parameter and the shutdown grace is configurable for tests. The class diagram is a rendered PNG.
Files Processed (6)
- scatter-gather/README.md (1 hunk)
- scatter-gather/etc/scatter-gather.urm.png (0 hunks)
- scatter-gather/etc/scatter-gather.urm.puml (1 hunk)
- scatter-gather/src/main/java/com/iluwatar/scattergather/Aggregator.java (1 hunk)
- scatter-gather/src/main/java/com/iluwatar/scattergather/ScatterGather.java (1 hunk)
- scatter-gather/src/test/java/com/iluwatar/scattergather/ScatterGatherTest.java (1 hunk)
Actionable Comments (5)
-
scatter-gather/src/main/java/com/iluwatar/scattergather/ScatterGather.java [103-107]
best_practice: "Logger field name mismatch with Lombok"
-
scatter-gather/src/main/java/com/iluwatar/scattergather/ScatterGather.java [150-150]
best_practice: "Use Lombok-provided logger"
-
scatter-gather/src/main/java/com/iluwatar/scattergather/ScatterGather.java [154-156]
best_practice: "Logger usage fix for timeout drop warning"
-
scatter-gather/src/main/java/com/iluwatar/scattergather/ScatterGather.java [160-160]
best_practice: "Logger usage fix for cancellation path"
-
scatter-gather/src/main/java/com/iluwatar/scattergather/ScatterGather.java [163-163]
best_practice: "Logger usage fix for gather summary"
Skipped Comments (0)
* fix: a rejected AI review poll stops instead of retrying for two minutes fetchAiReviewStatus went through bgRequest, which throws on a non-ok reply, so neither poller's `if (!resp.ok)` branch could ever run. The 403 case that cancels polling and reports "Authorization required" was dead code: the throw landed in the generic catch, which scheduled another attempt, and a token the server had rejected was retried every five seconds until the overall timeout. The same shape made the test hook's poll-failed branch unreachable, which is why a forbidden poll surfaced only as the opaque reason "HTTP 403". Hand the reply back rather than throwing on it, and carry the reply on the error bgRequest raises so a caller can tell a terminal 403 from a retryable 502. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014LUHgJkbycjaDgapzj8uG1 * test: the mocked AI review checks no longer ride on the live backend's verdict runAiReviewManualChecks mocks fetchAiReviewStatus outright and drives the Architecture Review button through READY and FAILED, so not one of its seven assertions depends on what the backend decided for a given pull request. They were nonetheless gated on shouldSkipLiveAiReview, which answers a different question: did the real review surface a note for this fixture. So the day the backend stopped returning 403 was the day those seven stopped running. Worse, the run still reported "AI review manual checks completed: ok", because aiReviewManualOk was initialised true and nothing had set it otherwise -- a backend fix quietly removed extension-side coverage and called it a pass. Drop the branch. aiReviewManualOk now starts false, so the completion line can no longer claim success for work that did not happen. The live check keeps its own skip, which is about the backend and belongs there. Verified against a run whose live check skipped as ready-no-notes -- the exact case that used to suppress them: all seven ran and passed, 103 pass / 0 fail. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014LUHgJkbycjaDgapzj8uG1 * chore: stop tracking 19MB of Chrome profiles in a public repository 647 files under test/test/.pw-profile-debug and .pw-profile-debug2 -- whole Chrome profiles left behind by a debug run that started from the wrong working directory, so the three pw-profile patterns already in .gitignore never matched them. Among them are Chrome's credential stores: Cookies, Login Data, Login Data For Account, Safe Browsing Cookies, and two copies of Local State. Every one is empty (0 rows in each of the six databases, checked rather than assumed) because these were throwaway profiles that never signed in, so this is repository hygiene rather than an exposure. History is left alone on the same reasoning: rewriting it breaks every existing clone to remove files that carry nothing. Ignore the doubled path and any .pw-profile directory wherever it lands, so the next run that starts somewhere unexpected does not do this again. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014LUHgJkbycjaDgapzj8uG1 * fix: offer a token on the large-upload 413 the server actually returns shouldPromptForTokenForZipLimit consulted ZIP_LIMIT_ERROR_CODES only, and matched a message pattern written for a different refusal. The server answers an oversized upload with 413 ZIP_UPLOAD_TOO_LARGE and "Uploaded file exceeds the maximum allowed size." -- a code that lives in ZIP_REDUCE_SCOPE_ERROR_CODES, and a message that says "file" where the pattern wanted "zip entry" -- so both arms missed it. The result was that the one refusal connecting a token actually fixes was the one that offered no token: the pull request reported a bare "API request failed 413" and stopped there. Observed on iluwatar/java-design-patterns#3601, where the API refused the upload on declared size in 4ms. Consult both sets, and match the message the server sends. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014LUHgJkbycjaDgapzj8uG1 * fix: make background-utils.js actually load, and keep one copy of what it holds The service worker has never imported it. It shares one global scope across importScripts, and background.js declared STATIC_PROXY_HOSTS, CACHE_KEYS, normalizeApiBase, readApiErrorResponse and ten more names of its own -- every one of them also declared here -- so importScripts threw "Identifier ... has already been declared" into the try/catch around it. BgUtils was {} in production, every `BgUtils.x || (inline fallback)` ran the fallback, and test/background-utils.test.js exercised the copy that never executed. Wrap the module so it declares one name instead of twenty-three. That fixes the collision without asking the importer to rename anything, and it holds under the unit tests too, which stub importScripts with require() and so give the file its own scope regardless. With BgUtils populated in both environments the fourteen fallbacks are dead, so they collapse to plain bindings. The two copies agreed character for character today; nothing kept them agreeing, and the drift would have shown up only in the one the tests do not cover. Verified in a service worker: BgUtils goes from 0 keys to 23, with normalizeApiBase, isGithubPullRequestUrl and STATIC_PROXY_HOSTS behaving as their tests say. The packaging script rewrote CACHE_KEYS in background.js to strip dev overrides; there is no copy there now, so that rewrite no longer has to change anything while its mustNotContain assertion stays as the proof the keys are absent from what ships. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014LUHgJkbycjaDgapzj8uG1 * refactor: one structured dump per mapping stage, and warnings off by default docs/CODE_REVIEW_PLAN.md §3 counts fourteen clog dumps in buildPathIdMapping: SVG node counts, entity samples and seven whole maps, one line each, on every render. Two debugDump calls now, one for what went in and one for what came out. They are read together or not at all, and DevTools renders an expandable object better than fourteen console entries. dumpStriffsMaps stays callable by hand. Same section, option (a): cwarn is gated behind the debug flag. It reports what this file has already handled and recovered from, which is not the user's console to fill. cerr stays unconditional, for a failure nothing handled. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014LUHgJkbycjaDgapzj8uG1 * test: count skips, and stop a quiet model skipping the checks that do not depend on it Three things. A skipped assertion was invisible. The run reported "N passed, 0 failed" whether or not a check had run, so a suppressed one looked exactly like a passing one. Skips are counted now and listed in an end-of-run summary beside the failures. A READY review that surfaced nothing skipped the whole live block -- including the overview, the structural-checks roster and the diagram swap, none of which depend on the model finding anything to say. Only the note assertion does, so only it is skipped now, and it is made at all only when the backend surfaced an item, which tests the surfaced-item to note mapping rather than the model's judgment. The hook no longer returns early on no-notes, so the panel is rendered and reported either way. shouldSkipLiveAiReview had a clause that could never run: the NOT_REQUESTED test above it already covered (timeout && NOT_REQUESTED). Dropped, along with the ready-no-notes clause the split makes unnecessary. SKIPPED is handled now, which it never was -- the backend declining a trivial pull request was being reported as a failure. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014LUHgJkbycjaDgapzj8uG1 * feat: route public-repo analysis through the queued upload path A stored token put a cold analysis on the slower of the two paths, and the one that can time out before the server finishes. The token GET holds a socket open for the whole analysis -- measured at 177-483s -- against a bgToken budget of 180s, while the upload path has been queued and polled to completion since the API started answering 202. So connecting a token, which reads as an upgrade, moved a user off the async path onto a synchronous one that can expire mid-analysis. Both reasons the upload path was the worse choice are gone. It is filtered to the files the analysis reads, so it is no longer a whole repository on the wire, and it is queued rather than held open. requestPrimary makes it the single entry point: private repository -> token GET, because codeload cannot fetch one unauthenticated; public repository -> upload, with the token GET as the fallback when an upload is refused for size. bgGenerate rises to 360s to cover a full analysis plus polling rather than a single POST. The two token assertions in the smoke test asserted the dispatch this replaces. They move rather than go: one now asserts the upload path on a public pull request, the other that a stored token still reaches GitHub on a path where the token is what makes the call possible, which the primary request can no longer demonstrate. shouldPromptForTokenForZipLimit now asks isUploadPathTooLargeError instead of restating it, so the prompt and the fallback cannot disagree again. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014LUHgJkbycjaDgapzj8uG1 * test: wait for a queued analysis, and judge the overview only when there is one Two failures from the first full run of this branch, neither of them a break. The new-UI render check allowed 45s. A cold analysis is queued and polled to completion rather than served inline, and this one took 50.1s: the view rendered fine, five seconds after the check had given up on it, and the Diffs toggle then failed as a knock-on because the view it toggles away from was still arriving. Wait for what the extension itself waits for. The overview assertions fail on a counts placeholder, on the reasoning that a review which fell back to counts says nothing. They were written when a review that surfaced nothing skipped this whole block, so they never had to consider one -- and now that only the note assertion is skipped, they run against a quiet review, where a placeholder is the server's correct answer. Scoped to a review that actually surfaced something, so they keep testing the panel's rendering rather than the model's judgment. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014LUHgJkbycjaDgapzj8uG1 * release: 1.1.0 * feat: surface documented-rule review automatically (#14) The server auto-starts the review and reports aiReviewStatus on the diagram payload, but the extension discarded that and gated the whole review behind a manual "AI Review" click and a cold poll -- so the first diagram shown was the un-enriched one, missing the doc-rule boxes and heatmap that are the point. Collect the already-running job instead of waiting for a click: - Auto-poll on render. renderStriffsResult now starts background polling when the payload reports PENDING/RUNNING (via the existing startEnrichmentPolling path), and keeps the already-enriched payload on READY. SKIPPED/NOT_REQUESTED map to null upstream and never poll; the remote kill switch and an in-flight poll both short-circuit. A late-arriving engagement token starts the poll off the background context refresh. - Progressive coverage headline on the diagram surface, no click: "Checking documented rules…" while polling, resolving to "N documented rules · M at risk" / "all upheld" from result.docFactVerdicts. At-risk = VIOLATED / PRE_EXISTING; upheld = MAINTAINED / RESTORED; UNCLEAR counted separately, never as at-risk. Hidden when there are zero documented rules or no review. - Auto-enrich on READY: the poll's refreshDiagramWithEnrichment swap now makes the annotated diagram the first meaningful paint rather than a second reveal. - Button becomes a view affordance: "Analyzing…" while a poll runs, then "View review (N rules)". It still only toggles the opt-in panel; an auto-started poll no longer auto-opens the 400px side panel (manual trigger still does). - Prefetch warms /ai-review too: on a successful diagram prefetch, a best-effort fire-and-forget GET nudges the review the server already started. No new prefetch trigger points. computeDocRuleCoverage / formatDocRuleHeadline (pure) and extractAiReviewWarmTarget (pure) are unit-tested; prefetch warm behaviour is covered in prefetch-flow.test.js. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017vk3svyJgrPxEekwF7uEjJ * test: expect the 'View review (N rules)' button label after READY (#14) The #14 flow renamed the AI-review button from a trigger ('View AI Review') to a view affordance ('View review (N rules)'), but the live smoke still asserted the old label and failed 3x on it while the flow itself passed (enriched diagram, documented-rules table, poll completed). Match the new label; docstring updated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017vk3svyJgrPxEekwF7uEjJ --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
What does this PR do?
Adds the Scatter-Gather pattern as a new
scatter-gathermodule.RateRequest,RateQuote,RateProvider: a travel site asking several hotel rate providers for the same stay.InMemoryRateProvider(fast),DelayedRateProvider(slow, interrupt-safe),FailingRateProvider.ScatterGather:scatter()(oneCompletableFutureper provider withorTimeout),gather()(waits for all to settle, keeps successes, logs timeouts and failures),scatterGather()chaining both with anAggregator;AutoCloseable.Aggregator: functional reduction of the gathered replies, with acheapestQuote()factory.App: four providers (two fast, one slower than the 300 ms timeout, one failing); logs the scatter, gather and aggregate phases and the best offer.README.md: intent, real-world example, sequence diagram, code walkthrough, applicability, trade-offs, related patterns including an explicit note on how Scatter-Gather differs from the existing Fan-Out/Fan-In module. PlantUML class diagram underetc/.AppTest.pom.xml../mvnw clean verify -pl scatter-gatherpasses locally on JDK 21 and inside aneclipse-temurin:21container.Note: PR #3590 also targets this issue. This implementation was written independently with a different domain and design; it includes the populated, logging
Appentry point requested in the review of #3590 and treats partial results and timeouts as the core of the pattern. Happy to align with whichever direction the maintainers prefer.Fixes #3577