feat: add Timeout pattern (#2845) - #3598
Conversation
PR SummaryIntroduces a new timeout module implementing per-service timeouts with Changes
autogenerated by presubmit.ai |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🚨 Pull request needs attention.
Review Summary
Files Processed (16)
- pom.xml (1 hunk)
- timeout/README.md (1 hunk)
- timeout/etc/timeout.urm.puml (1 hunk)
- timeout/pom.xml (1 hunk)
- timeout/src/main/java/com/iluwatar/timeout/App.java (1 hunk)
- timeout/src/main/java/com/iluwatar/timeout/ProductCatalogService.java (1 hunk)
- timeout/src/main/java/com/iluwatar/timeout/RecommendationService.java (1 hunk)
- timeout/src/main/java/com/iluwatar/timeout/ServiceCallException.java (1 hunk)
- timeout/src/main/java/com/iluwatar/timeout/TimeoutExecutor.java (1 hunk)
- timeout/src/main/java/com/iluwatar/timeout/TimeoutMetrics.java (1 hunk)
- timeout/src/main/java/com/iluwatar/timeout/TimeoutPolicy.java (1 hunk)
- timeout/src/main/java/com/iluwatar/timeout/TimeoutRegistry.java (1 hunk)
- timeout/src/test/java/com/iluwatar/timeout/AppTest.java (1 hunk)
- timeout/src/test/java/com/iluwatar/timeout/TimeoutExecutorTest.java (1 hunk)
- timeout/src/test/java/com/iluwatar/timeout/TimeoutPolicyTest.java (1 hunk)
- timeout/src/test/java/com/iluwatar/timeout/TimeoutRegistryTest.java (1 hunk)
Actionable Comments (7)
-
timeout/src/main/java/com/iluwatar/timeout/App.java [61-61]
possible bug: "Logger name should match Lombok's @slf4j field"
-
timeout/src/main/java/com/iluwatar/timeout/App.java [67-67]
readability: "Use the Lombok 'log' logger"
-
timeout/src/main/java/com/iluwatar/timeout/App.java [73-73]
readability: "Use the Lombok 'log' logger"
-
timeout/src/main/java/com/iluwatar/timeout/App.java [81-81]
readability: "Use the Lombok 'log' logger for final status"
-
timeout/src/main/java/com/iluwatar/timeout/ProductCatalogService.java [59-59]
possible bug: "Logger field name mismatch in ProductCatalogService"
-
timeout/src/main/java/com/iluwatar/timeout/RecommendationService.java [60-66]
possible bug: "Logger field name mismatch in RecommendationService"
-
timeout/src/main/java/com/iluwatar/timeout/TimeoutExecutor.java [82-82]
possible bug: "Logger name mismatch in TimeoutExecutor"
Skipped Comments (1)
-
timeout/pom.xml [46-50]
enhancement: "Add junit-jupiter-api for tests"
| new TimeoutRegistry(Duration.ofMillis(300)) | ||
| .register(TimeoutPolicy.of(ProductCatalogService.NAME, 500)) | ||
| .register(TimeoutPolicy.of(RecommendationService.NAME, 100)); | ||
| LOGGER.info("Configured per-service limits: catalog 500 ms, recommendations 100 ms"); |
There was a problem hiding this comment.
Logger instance name coming from Lombok's @slf4j is 'log', not 'LOGGER'. This will cause a compilation error. Use 'log' consistently.
| var recommendations = new RecommendationService(Duration.ofMillis(400)); | ||
|
|
||
| try (var executor = new TimeoutExecutor()) { | ||
| LOGGER.info("Calling {}", ProductCatalogService.NAME); |
There was a problem hiding this comment.
Replace the logger with the Lombok-generated field 'log'.
| registry.policyFor(ProductCatalogService.NAME), catalog::fetchProducts, List::of); | ||
| LOGGER.info("Products: {}", products); | ||
|
|
||
| LOGGER.info("Calling {}", RecommendationService.NAME); |
There was a problem hiding this comment.
Replace the logger with the Lombok-generated field 'log'.
| () -> POPULAR_ITEMS); | ||
| LOGGER.info("Recommendations shown to alice: {}", suggested); | ||
|
|
||
| LOGGER.info("Timeouts per service: {}", executor.metrics().snapshot()); |
There was a problem hiding this comment.
Replace the logger with the Lombok-generated field 'log'.
| * @throws InterruptedException if the call is cancelled while waiting for the simulated backend | ||
| */ | ||
| public List<String> fetchProducts() throws InterruptedException { | ||
| LOGGER.info("{}: fetching products, expected latency {} ms", NAME, latency.toMillis()); |
There was a problem hiding this comment.
Replace the logger with the Lombok-generated 'log' instance.
| LOGGER.info( | ||
| "{}: computing recommendations for {}, expected latency {} ms", | ||
| NAME, | ||
| customer, | ||
| latency.toMillis()); | ||
| try { | ||
| Thread.sleep(latency); |
There was a problem hiding this comment.
Replace all usages of the Lombok-generated logger with the 'log' field. The multi-line log call needs to reference 'log'.
| var future = executor.submit(call); | ||
| try { | ||
| var result = future.get(limitMillis, TimeUnit.MILLISECONDS); | ||
| LOGGER.info("{} responded within its {} ms limit", serviceName, limitMillis); |
There was a problem hiding this comment.
Replace the logger with the Lombok-generated 'log' instance.
|
Note on the automated review comments: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3598 +/- ##
============================================
+ Coverage 83.79% 83.91% +0.11%
- Complexity 4277 4311 +34
============================================
Files 1121 1129 +8
Lines 15144 15243 +99
Branches 723 726 +3
============================================
+ Hits 12690 12791 +101
+ Misses 2159 2157 -2
Partials 295 295 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Follow-up on the Codecov note: added a test for an interrupted caller in |
There was a problem hiding this comment.
🚨 Pull request needs attention.
Review Summary
Files Processed (17)
- pom.xml (1 hunk)
- timeout/README.md (1 hunk)
- timeout/etc/timeout.urm.puml (1 hunk)
- timeout/pom.xml (1 hunk)
- timeout/src/main/java/com/iluwatar/timeout/App.java (1 hunk)
- timeout/src/main/java/com/iluwatar/timeout/ProductCatalogService.java (1 hunk)
- timeout/src/main/java/com/iluwatar/timeout/RecommendationService.java (1 hunk)
- timeout/src/main/java/com/iluwatar/timeout/ServiceCallException.java (1 hunk)
- timeout/src/main/java/com/iluwatar/timeout/TimeoutExecutor.java (1 hunk)
- timeout/src/main/java/com/iluwatar/timeout/TimeoutMetrics.java (1 hunk)
- timeout/src/main/java/com/iluwatar/timeout/TimeoutPolicy.java (1 hunk)
- timeout/src/main/java/com/iluwatar/timeout/TimeoutRegistry.java (1 hunk)
- timeout/src/test/java/com/iluwatar/timeout/AppTest.java (1 hunk)
- timeout/src/test/java/com/iluwatar/timeout/RecommendationServiceTest.java (1 hunk)
- timeout/src/test/java/com/iluwatar/timeout/TimeoutExecutorTest.java (1 hunk)
- timeout/src/test/java/com/iluwatar/timeout/TimeoutPolicyTest.java (1 hunk)
- timeout/src/test/java/com/iluwatar/timeout/TimeoutRegistryTest.java (1 hunk)
Actionable Comments (3)
-
timeout/src/main/java/com/iluwatar/timeout/App.java [61-61]
readability: "Replace Lombok-generated logger usage with 'log'."
-
timeout/src/main/java/com/iluwatar/timeout/ProductCatalogService.java [59-59]
readability: "Use Lombok 'log' instead of 'LOGGER' in logging."
-
timeout/src/main/java/com/iluwatar/timeout/RecommendationService.java [60-64]
readability: "Replace Lombok-generated logger usage with 'log' in multi-line logging."
Skipped Comments (0)
| new TimeoutRegistry(Duration.ofMillis(300)) | ||
| .register(TimeoutPolicy.of(ProductCatalogService.NAME, 500)) | ||
| .register(TimeoutPolicy.of(RecommendationService.NAME, 100)); | ||
| LOGGER.info("Configured per-service limits: catalog 500 ms, recommendations 100 ms"); |
There was a problem hiding this comment.
This project uses Lombok's @slf4j which provides a 'log' field. The code currently references 'LOGGER', which will fail to compile. Replace LOGGER.info(...) with log.info(...) (and similarly for other logging calls in this file).
| * @throws InterruptedException if the call is cancelled while waiting for the simulated backend | ||
| */ | ||
| public List<String> fetchProducts() throws InterruptedException { | ||
| LOGGER.info("{}: fetching products, expected latency {} ms", NAME, latency.toMillis()); |
There was a problem hiding this comment.
Replace the LOGGER.info call with log.info since the class uses @slf4j. The current line will not compile as LOGGER is undefined.
| LOGGER.info( | ||
| "{}: computing recommendations for {}, expected latency {} ms", | ||
| NAME, | ||
| customer, | ||
| latency.toMillis()); |
There was a problem hiding this comment.
Replace the multi-line LOGGER.info(...) logging with a single log.info(...) block using the same message and arguments. The Lombok @slf4j field is named 'log'.
|
Coverage follow-up: extracted the two demo calls in |
There was a problem hiding this comment.
✅ LGTM!
Review Summary
Files Processed (17)
- pom.xml (1 hunk)
- timeout/README.md (1 hunk)
- timeout/etc/timeout.urm.puml (1 hunk)
- timeout/pom.xml (1 hunk)
- timeout/src/main/java/com/iluwatar/timeout/App.java (1 hunk)
- timeout/src/main/java/com/iluwatar/timeout/ProductCatalogService.java (1 hunk)
- timeout/src/main/java/com/iluwatar/timeout/RecommendationService.java (1 hunk)
- timeout/src/main/java/com/iluwatar/timeout/ServiceCallException.java (1 hunk)
- timeout/src/main/java/com/iluwatar/timeout/TimeoutExecutor.java (1 hunk)
- timeout/src/main/java/com/iluwatar/timeout/TimeoutMetrics.java (1 hunk)
- timeout/src/main/java/com/iluwatar/timeout/TimeoutPolicy.java (1 hunk)
- timeout/src/main/java/com/iluwatar/timeout/TimeoutRegistry.java (1 hunk)
- timeout/src/test/java/com/iluwatar/timeout/AppTest.java (1 hunk)
- timeout/src/test/java/com/iluwatar/timeout/RecommendationServiceTest.java (1 hunk)
- timeout/src/test/java/com/iluwatar/timeout/TimeoutExecutorTest.java (1 hunk)
- timeout/src/test/java/com/iluwatar/timeout/TimeoutPolicyTest.java (1 hunk)
- timeout/src/test/java/com/iluwatar/timeout/TimeoutRegistryTest.java (1 hunk)
Actionable Comments (0)
Skipped Comments (8)
-
timeout/src/main/java/com/iluwatar/timeout/App.java [61-61]
best_practice: "Use Lombok's generated logger"
-
timeout/src/main/java/com/iluwatar/timeout/App.java [67-67]
best_practice: "Use Lombok's generated logger"
-
timeout/src/main/java/com/iluwatar/timeout/App.java [73-73]
best_practice: "Use Lombok's generated logger"
-
timeout/src/main/java/com/iluwatar/timeout/App.java [75-75]
best_practice: "Use Lombok's generated logger"
-
timeout/src/main/java/com/iluwatar/timeout/ProductCatalogService.java [59-59]
best_practice: "Use Lombok's generated logger"
-
timeout/src/main/java/com/iluwatar/timeout/RecommendationService.java [60-66]
best_practice: "Use Lombok's generated logger"
-
timeout/src/main/java/com/iluwatar/timeout/TimeoutExecutor.java [82-82]
best_practice: "Use Lombok's generated logger"
-
timeout/src/main/java/com/iluwatar/timeout/TimeoutExecutor.java [88-90]
best_practice: "Use Lombok's generated logger"
1 participant
What does this PR do?
Adds the Timeout pattern as a new
timeoutmodule.TimeoutPolicy. When the limit is exceeded the call is cancelled with an interrupt, the event is logged and counted, and a fallback answer is returned.TimeoutPolicy(record) andTimeoutRegistry: per-service configurable limits with a default.TimeoutExecutor: enforces the limit (Future.get(timeout)), cancels the overrunning call, records the event inTimeoutMetrics, invokes the fallback. Service failures are surfaced asServiceCallException, not as timeouts.ProductCatalogService(fast) andRecommendationService(slow, interruptible): simulated dependencies.App: catalog answers within its 500 ms limit; recommendations exceed their 100 ms limit, get cancelled and replaced by popular items; timeout counters are printed. Logging traces every step.README.md: intent, real-world example, sequence diagram, code walkthrough, applicability, trade-offs, related patterns (including how this differs from the existingfallbackmodule, where the time limit is only one of several triggers). PlantUML class diagram underetc/.AppTest.pom.xml../mvnw clean verify -pl timeoutpasses locally on JDK 21 and inside aneclipse-temurin:21container.Fixes #2845