develop asynchronous request#243
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job on setting up the overall structure with CompletableFuture and the ExecutorService! However, a couple of changes are needed before this can be approved.
First, the task requires simulating a processing delay for new requests to properly demonstrate the benefits of caching. Please add a small delay (e.g., using Thread.sleep()) inside the logic that fetches new user data in AsyncRequestProcessor.java.
Second, your current caching logic has a race condition. The if (cache.get(userId) != null) check and the subsequent cache.put() are not atomic. A safer, more efficient approach is to use the cache.computeIfAbsent() method, which guarantees atomicity for this check-and-put operation.
Addressing these points will make your solution fully compliant and robust. Keep up the good work!
✨ 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
| if (cache.get(userId) != null) { | ||
| res = cache.get(userId); | ||
| } else { | ||
| res = new UserData(userId, "Details for " + userId); | ||
| cache.put(userId, res); | ||
| } |
There was a problem hiding this comment.
This check-then-put pattern is not atomic and can lead to a race condition. If two threads try to process the same new userId concurrently, both could find that the user is not in the cache, and both would proceed to create a new UserData object. Using an atomic operation like cache.computeIfAbsent() would be a safer and more efficient way to handle this.
| if (cache.get(userId) != null) { | ||
| res = cache.get(userId); | ||
| } else { | ||
| res = new UserData(userId, "Details for " + userId); |
There was a problem hiding this comment.
The task requirements mention simulating a processing delay (e.g., for a database call). You should add a small delay here inside the else block, for example using Thread.sleep(), to properly simulate fetching new data. This would also make the benefits of caching more obvious.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job on the overall structure and for correctly using CompletableFuture and computeIfAbsent!
I am requesting a change because there's a key logical issue that prevents the caching from working as required. In AsyncRequestProcessor.java, the Thread.sleep(1000) delay is applied to every single request, even if the data is already in the cache. This negates the performance benefit, as a core requirement is that cached results should be returned immediately.
To fix this, the simulated delay should only happen when a new UserData object is being created. Please move the Thread.sleep(1000) call inside the lambda function passed to cache.computeIfAbsent(). That way, the delay will only be triggered for non-cached requests.
You're very close to finishing this task. Keep up the great work!
✨ 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
| return null; | ||
| return CompletableFuture.supplyAsync(() -> { | ||
| try { | ||
| Thread.sleep(1000); |
There was a problem hiding this comment.
This delay is currently applied to every request, even for data that is already in the cache. According to the requirements, cached results should be returned immediately to demonstrate a significant reduction in response time.
To fix this, the simulated processing delay should only happen when a new UserData object is being created. This logic should be inside the mapping function provided to cache.computeIfAbsent(). That function is only executed when the userId is not found in the cache.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work! I'm happy to approve your solution. You've perfectly addressed the feedback from the previous review by moving the Thread.sleep call inside the computeIfAbsent lambda. This ensures the delay only happens for new, non-cached requests, which was the main goal of the task. Your overall implementation is clean, efficient, and correctly handles asynchronous processing. Keep up the great work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.