feat: async user request processor#253
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Changes are needed before this solution can be considered complete.
- Missing
UserDataclass (critical, blocking): The code inAsyncRequestProcessor.processRequestreturnsCompletableFuture<UserData>, but there is noUserDataclass in the project, so the code will not compile and cannot run. You need to addmate.academy.UserDatawith fieldsuserIdanddetails, a constructor and getters, and overridetoString()so it produces exactlyUserData[userId=user1, details=Details for user1](withuser1and its details replaced by the actual values) to match the expected console output. - Cache race condition for duplicate requests (critical, functional): In
AsyncRequestProcessor.processRequest(where you currently doif (cache.containsKey(userId)) { return CompletableFuture.completedFuture(cache.get(userId)); }), two concurrent calls for the sameuserIdcan both see a cache miss and trigger duplicate simulated processing instead of sharing one result; this breaks the requirement that repeated requests (like the second"user1"inMain) are immediately served from cache and follow the expected timing/ordering. To fix this, use an atomic pattern such as cachingCompletableFuture<UserData>in aMap<String, CompletableFuture<UserData>>and creating it viacomputeIfAbsent, or usingputIfAbsentso only the first thread starts processing and the others reuse the same future.
Minor improvements: you could replace Thread.sleep(400) with TimeUnit.MILLISECONDS.sleep(400) for clarity and consider handling exceptional completions of the futures (e.g., logging if something goes wrong). Overall, your use of CompletableFuture, an executor, and a concurrent cache is on the right track; once you add UserData correctly and make the cache access atomic, the solution will better satisfy all the task requirements.
✨ 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 class AsyncRequestProcessor { | ||
| private final Executor executor; | ||
| private final Map<String, UserData> cache = new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
The code refers to UserData, but no UserData class is present in the provided files. Requirement: “Define a UserData class as a simple class with user details.” Add a UserData class with fields userId and details and appropriate constructor/getters.
| throw new RuntimeException("Task was interrupted", e); | ||
| } | ||
|
|
||
| UserData userData = new UserData(userId, "Details for " + userId); |
There was a problem hiding this comment.
When you create UserData here, ensure its toString() produces the exact format required by the task: UserData[userId=user1, details=Details for user1]. Otherwise the console output won't match the expected lines.
|
|
||
| public CompletableFuture<UserData> processRequest(String userId) { | ||
| return null; | ||
| if (cache.containsKey(userId)) { |
There was a problem hiding this comment.
Using cache.containsKey(userId) then cache.get(userId) can produce a race: if two threads call processRequest concurrently for the same userId and the cache isn't populated yet, both will run the simulated processing. If you need subsequent requests to be served from cache immediately (as implied by the expected output), consider preventing duplicate in-flight processing (for example by caching in-progress CompletableFutures or using a synchronized computeIfAbsent pattern). Note: caching futures changes the Map type, so choose an approach that fits the assignment constraints.
| public CompletableFuture<UserData> processRequest(String userId) { | ||
| return null; | ||
| if (cache.containsKey(userId)) { | ||
| return CompletableFuture.completedFuture(cache.get(userId)); |
There was a problem hiding this comment.
The thenAccept prints the userData object; make sure UserData.toString() matches the expected console output exactly, otherwise the printed lines won't match the requirement.
|
|
||
| // Simulating multiple concurrent requests | ||
| String[] userIds = {"user1", "user2", "user3", "user1"}; // Note: "user1" is repeated | ||
| String[] userIds = {"user1", "user2", "user3", "user1"}; |
There was a problem hiding this comment.
The input String[] userIds includes a repeated "user1" and the task expects a specific console output ordering driven by caching behavior. With the current implementation the output order is not guaranteed because duplicate requests may be processed concurrently. Fix the caching approach (cache futures or use computeIfAbsent) so the repeated request is served from cache and the output matches the expected behavior.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because a key requirement about the cache type and behavior is not yet satisfied, which may cause the task’s tests to fail. The task explicitly requires a cache declared exactly as Map<String, UserData> cache = new ConcurrentHashMap<>();, but in both Main.java and AsyncRequestProcessor.java you currently use Map<String, CompletableFuture<UserData>>, which violates the specification and checklist. In addition, processRequest(String userId) never stores the final UserData result into a Map<String, UserData> cache, so repeated requests after the first completion are not served from the required cache of completed values. To fix this, keep the required Map<String, UserData> cache = new ConcurrentHashMap<>(); for completed data and introduce a separate concurrent map (e.g. Map<String, CompletableFuture<UserData>> inFlight = new ConcurrentHashMap<>();) to track in‑flight work; use inFlight.computeIfAbsent(...) (or putIfAbsent) to start processing once, and in a thenApply/whenComplete handler put the completed UserData into cache. Also note that printing results inside thenAccept can lead to nondeterministic output ordering compared to the sample; if your tests expect exact ordering, you may need to ensure cached results are returned as already-completed futures or otherwise coordinate the prints. Aside from these issues, your use of CompletableFuture, the asynchronous delay, and the UserData.toString() format look solid, so once you align the cache type/usage with the requirements, your solution should be in great shape.
✨ 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
| this.userId = userId; | ||
| this.details = details; | ||
| } | ||
|
|
There was a problem hiding this comment.
The task description explicitly requires the cache to be declared exactly as Map<String, UserData> cache = new ConcurrentHashMap<>();. Here the cache is declared as Map<String, CompletableFuture<UserData>>, which does not match the requirement. Change the declaration to the required type and use a separate map (e.g., an "in-flight" map of CompletableFuture<UserData>) or putIfAbsent to avoid race conditions while still keeping the required Map<String, UserData> cache.
|
|
||
| public class AsyncRequestProcessor { | ||
| private final Executor executor; | ||
| private final Map<String, CompletableFuture<UserData>> cache = new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
You store futures in the cache (Map<String, CompletableFuture<UserData>> cache = new ConcurrentHashMap<>()) and use computeIfAbsent, which correctly prevents the race condition so that duplicate requests share one processing task. Note: the original description explicitly showed Map<String, UserData> cache = new ConcurrentHashMap<>(); using a futures map deviates from that exact declaration but is the intended atomic pattern to avoid duplicate processing.
|
|
||
| public CompletableFuture<UserData> processRequest(String userId) { | ||
| return null; | ||
| return cache.computeIfAbsent(userId, id -> CompletableFuture.supplyAsync(() -> { |
There was a problem hiding this comment.
The thenAccept print handlers are attached to asynchronous tasks so the order of the "Processed: ..." console lines is not guaranteed. If the exercise/automated tests require the exact ordering shown in the description, you'll need to introduce coordination (e.g. ordered completion logic or controlled delays) to make outputs deterministic.
| ExecutorService executor = null; // Provide implementation that fits your needs | ||
| ExecutorService executor = Executors.newFixedThreadPool(4); | ||
| AsyncRequestProcessor asyncRequestProcessor = new AsyncRequestProcessor(executor); | ||
|
|
There was a problem hiding this comment.
The task description explicitly requires declaring the cache exactly as
Map<String, UserData> cache = new ConcurrentHashMap<>(); — here you declare a Map<String, CompletableFuture<UserData>>. Change the declaration to match the requirement, and adapt the logic (for example: keep a separate Map<String, CompletableFuture<UserData>> for in-flight processing and a Map<String, UserData> for completed cached values, or use an atomic putIfAbsent approach so only the first thread processes and others reuse the result).
| String userId = userIds[i]; | ||
| futures[i] = asyncRequestProcessor.processRequest(userId) | ||
| .thenAccept(userData -> System.out.println("Processed: " + userData)); | ||
| .thenAccept(userData -> System.out.println("Processed: " + userData)) |
There was a problem hiding this comment.
Using computeIfAbsent on a Map<String, CompletableFuture<UserData>> correctly avoids the race condition (good). But the description also requires that after processing you store the resulting UserData in a Map<String, UserData> cache; currently the code only stores the future. Ensure the completed UserData is placed into the required Map<String, UserData> after processing so other code can read the cached value.
| String userId = userIds[i]; | ||
| futures[i] = asyncRequestProcessor.processRequest(userId) | ||
| .thenAccept(userData -> System.out.println("Processed: " + userData)); | ||
| .thenAccept(userData -> System.out.println("Processed: " + userData)) |
There was a problem hiding this comment.
Printing the result inside thenAccept is fine, but note that the exact console ordering shown in the description can be nondeterministic with purely asynchronous completion. If the exercise's tests expect the exact sample output ordering, make sure cached results are returned as already-completed futures (so their thenAccept runs immediately) or otherwise coordinate ordering.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your solution is approved; I’m allowing it to pass as an educational override so you can keep progressing while you refine the remaining details. You correctly implemented asynchronous processing with CompletableFuture, used a ConcurrentHashMap cache declared as Map<String, UserData> cache = new ConcurrentHashMap<>();, and your UserData.toString() format matches the required output. The main critical issue is that you print results directly in asynchronous callbacks (for example, in Main.java where you call thenAccept on each processRequest future), which can lead to nondeterministic console ordering compared to the sample output [CHECKLIST ITEM: sample output ordering]. To improve this, keep the futures in an array aligned with the input user IDs and, in a separate loop, call futures[i].join() in order and print the results so the console output always follows the original input sequence. Similarly, in AsyncRequestProcessor.java and UserData.java, avoid doing final printing in thenAccept callbacks and instead return the data so a central place (like Main) can control when and in what order to print. As a minor improvement, consider handling and logging exceptions closer to where each CompletableFuture is created to make debugging easier and error traces clearer. Overall, your async and caching logic is strong, and polishing the output ordering will make your solution both robust and test-friendly.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| this.executor = executor; | ||
| } | ||
|
|
||
| public CompletableFuture<UserData> processRequest(String userId) { |
There was a problem hiding this comment.
Printing the result directly in the asynchronous callback can lead to nondeterministic ordering of console output compared to the sample. If tests require exact ordering, consider collecting results and printing them in the original input order after all futures complete (or otherwise coordinate printing). Note: your caching logic already returns completed futures for cached entries, but concurrent completion timing can still reorder prints.
| String userId = userIds[i]; | ||
| futures[i] = asyncRequestProcessor.processRequest(userId) | ||
| .thenAccept(userData -> System.out.println("Processed: " + userData)); | ||
| .thenAccept(userData -> System.out.println("Processed: " + userData)) |
There was a problem hiding this comment.
Printing the results inside the asynchronous callback (thenAccept) can lead to nondeterministic console ordering versus the sample output in the task description. If tests expect the exact ordering, consider printing in input order after waiting on each corresponding future (e.g. keep the futures array and for each index call futures[i].join() then print), or explicitly ensure cached values are returned as already-completed futures (you already return CompletableFuture.completedFuture(...) in the processor) and handle printing so repeated requests appear immediately in the expected position.
|
|
||
| public String details() { | ||
| return details; | ||
| } |
There was a problem hiding this comment.
Printing the results inside thenAccept can produce nondeterministic ordering compared to the sample output in the description. If automated tests expect the exact sequence, consider coordinating prints (e.g., collect results and print in original input order after joining, or otherwise enforce ordering).
No description provided.