Add caching mechanism to AsyncRequestProcessor and update Main with t…#251
Add caching mechanism to AsyncRequestProcessor and update Main with t…#251chertiav wants to merge 3 commits into
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because some core task requirements are not yet fully met, which affects both functionality and the expected output. In AsyncRequestProcessor.processRequest, the cache lookup currently happens inside the CompletableFuture.supplyAsync supplier, so even when a userId is already cached you still schedule work on the executor instead of returning the cached value immediately; the requirement explicitly says that repeated requests must be served from the cache right away. To fix this, first check cache.get(userId) before creating the future, return CompletableFuture.completedFuture(cached) on a hit, and only call supplyAsync (and update the cache) when the value is absent. The UserData details string is also not aligned with the expected output: it uses something like "User data for ..." instead of the required "Details for <userId>", so please adjust the constructor call (or toString) so that printed lines exactly match the required format. Additionally, there is an extra System.out.println such as "Processing request for ..." in AsyncRequestProcessor that will produce lines not present in the expected console output, so it should be removed or changed if you want to keep logging separate from the required output. A smaller, non-blocking issue is the Checkstyle error in Main.java line 10 where a line exceeds 100 characters; you can fix this by breaking the long statement into multiple shorter lines. Once you make these adjustments, your use of CompletableFuture, ConcurrentHashMap, and the overall async flow will nicely satisfy the task, so you are very close—treat these changes as the final polish to a generally solid solution.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
|
|
||
| public CompletableFuture<UserData> processRequest(String userId) { | ||
| return null; | ||
| return CompletableFuture.supplyAsync(() -> |
There was a problem hiding this comment.
Creating the CompletableFuture with supplyAsync and calling cache.computeIfAbsent inside the supplier means you still schedule work on the executor even when the value is already cached. This violates the requirement to "return the cached result immediately". Consider checking cache.get(userId) first and returning CompletableFuture.completedFuture(cached) if present, and only run async computation when absent.
| public CompletableFuture<UserData> processRequest(String userId) { | ||
| return null; | ||
| return CompletableFuture.supplyAsync(() -> | ||
| cache.computeIfAbsent(userId, id -> { |
There was a problem hiding this comment.
Using cache.computeIfAbsent is fine for atomic compute-and-store, but because it's invoked inside the async supplier the lookup isn't immediate for callers. Move the cache check outside the async supplier (or return a completed future if present) so repeated requests are served from cache without scheduling executor work.
| throw new RuntimeException(e); | ||
| } | ||
|
|
||
| System.out.println("Processing request for " + id); |
There was a problem hiding this comment.
This log prints "Processing request for ..." which is not part of the expected console output and will produce extra lines. Remove or adjust this println so output matches the required lines.
| } | ||
|
|
||
| System.out.println("Processing request for " + id); | ||
| return new UserData(id, "User data for " + id); |
There was a problem hiding this comment.
The UserData is created with details "User data for ..." but the task requires the details format to be "Details for " to match the expected console output. Change to new UserData(id, "Details for " + id) (or adjust UserData.toString accordingly).
| return null; | ||
| return CompletableFuture.supplyAsync(() -> | ||
| cache.computeIfAbsent(userId, id -> { | ||
| try { |
There was a problem hiding this comment.
Main calls processRequest(...) and expects repeated requests to be served from cache quickly; because processRequest currently schedules async work even when cached, the repeated 'user1' won't be returned immediately. After fixing processRequest to return a completed future when cached, Main's behavior will meet the requirement.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your solution is approved because it implements the required async processing with CompletableFuture, uses a ConcurrentHashMap cache, and correctly constructs and returns UserData with details in the format "Details for <userId>".
In AsyncRequestProcessor.processRequest (around the cache check near the top of the method), you can simplify and slightly optimize the logic by doing a single lookup, e.g. UserData cached = cache.get(userId); if (cached != null) return CompletableFuture.completedFuture(cached); instead of calling containsKey and then get.
If you want to avoid duplicate work for concurrent requests with the same userId, consider changing the cache to Map<String, CompletableFuture<UserData>> and using computeIfAbsent so all callers share the same in-flight future instead of starting multiple supplyAsync tasks.
Also double-check that UserData.toString() returns exactly UserData[userId=<id>, details=Details for <id>], since the printed output in Main relies on that exact format for tests to pass.
Overall, your implementation meets the functional requirements, and these suggestions are mainly to align more closely with the checklist and to improve efficiency and robustness.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| if (cache.containsKey(userId)) { | ||
| return CompletableFuture.completedFuture(cache.get(userId)); |
There was a problem hiding this comment.
Instead of calling cache.containsKey(userId) and then cache.get(userId), retrieve the cached value once with UserData cached = cache.get(userId); and if non-null return CompletableFuture.completedFuture(cached) — this matches the checklist wording and avoids a double lookup.
| return CompletableFuture.supplyAsync(() -> { | ||
| try { | ||
| Thread.sleep(100); | ||
| } catch (InterruptedException e) { | ||
| Thread.currentThread().interrupt(); | ||
| throw new RuntimeException(e); | ||
| } | ||
|
|
||
| UserData userData = new UserData(userId, "Details for " + userId); | ||
| cache.put(userId, userData); | ||
| return userData; | ||
| }, executor); |
There was a problem hiding this comment.
Currently if the first async task for a userId is still running when a duplicate request arrives, you will schedule another supplyAsync. If you want duplicate concurrent requests to be served immediately from cache (no duplicate work), consider caching CompletableFuture<UserData> (in-flight futures) or another deduplication strategy so second callers can reuse the same future instead of triggering new work.
|
|
||
| if (cache.containsKey(userId)) { | ||
| return CompletableFuture.completedFuture(cache.get(userId)); | ||
| } |
There was a problem hiding this comment.
The printed line depends on UserData.toString(). Ensure toString() formats instances exactly as UserData[userId=<id>, details=Details for <id>] so console output matches the expected lines.
2 participants
…hread pool executor