서버가 동기화 될 때 Pre-Warm Redis 정보가 유실 대응 및 Miss 시 대응 전략 도입 #195
Conversation
…r implementation and adding load methods for max capacities and current applications
… current applications
|
Caution Review failedThe pull request is closed. WalkthroughThe changes refactor the exam quota caching mechanism. Cache initialization logic is removed from Changes
Sequence Diagram(s)sequenceDiagram
participant SpringBoot
participant ExamQuotaCacheInitializer
participant ExamQuotaLoadService
participant ExamQuotaCacheManager
participant ExamJpaRepository
SpringBoot->>ExamQuotaCacheInitializer: onApplicationStart()
ExamQuotaCacheInitializer->>ExamQuotaLoadService: loadMaxCapacities() [async]
ExamQuotaCacheInitializer->>ExamQuotaLoadService: loadCurrentApplications() [async]
ExamQuotaLoadService->>ExamJpaRepository: fetchUpcomingExamsWithCapacities()
ExamQuotaLoadService->>ExamQuotaCacheManager: loadMaxCapacities(map)
ExamQuotaLoadService->>ExamJpaRepository: countApplicationsGroupedByExamId()
ExamQuotaLoadService->>ExamQuotaCacheManager: loadCurrentApplications(map)
ExamQuotaCacheInitializer->>ExamQuotaCacheInitializer: Wait for both futures to complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15–20 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (8)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @polyglot-k, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
I've refactored the Redis cache pre-warming mechanism for exam quotas to address potential data loss and improve miss handling during server synchronization. Previously, the ExamQuotaCacheManager handled its own initialization on startup. This change introduces a dedicated service (ExamQuotaLoadService) to manage the loading of exam capacities and current application counts from the database, and an initializer (ExamQuotaCacheInitializer) that orchestrates this loading asynchronously when the application starts. Additionally, I've updated a database query to ensure all exams are correctly included in application counts, preventing potential data misses in the cache.
Highlights
- Refactored Cache Initialization: I've refactored the cache pre-warming logic by extracting it from
ExamQuotaCacheManagerinto a newExamQuotaLoadServiceand anExamQuotaCacheInitializercomponent. This separation of concerns makes the cache management more modular and maintainable. - Asynchronous Cache Loading: The new
ExamQuotaCacheInitializernow leveragesCompletableFutureto load exam capacities and current application counts in parallel on application startup. This asynchronous approach improves the efficiency and speed of cache pre-warming. - Enhanced Data Consistency: To ensure the cache is populated with complete and accurate data, I've updated the
countApplicationsGroupedBySchoolNamequery inExamJpaRepositoryto use aRIGHT JOIN. This change guarantees that all exams, including those that currently have no applications, are correctly included in the dataset used for cache population, preventing potential data misses.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request refactors the Redis cache pre-warming logic. The initialization is moved into a dedicated ExamQuotaLoadService and an ApplicationRunner (ExamQuotaCacheInitializer), which allows for parallel execution of loading tasks to improve application startup time. Additionally, a bug in the exam application counting query is fixed by using a RIGHT JOIN. The changes are well-structured and improve both performance and correctness. I have a critical suggestion to prevent a potential NullPointerException during cache loading and a medium-severity suggestion to improve logging in the cache initializer for better observability.
| .stream() | ||
| .collect(Collectors.toMap( | ||
| ExamJpaEntity::getId, | ||
| e -> e.getCapacity().longValue() | ||
| )); |
There was a problem hiding this comment.
The code assumes that exam.getCapacity() will never be null. However, the capacity field in ExamJpaEntity is of type Integer, which is nullable. If any ExamJpaEntity returned from the repository has a null capacity, calling .longValue() on it will result in a NullPointerException. This would cause the cache initialization to fail and prevent the application from starting. It's safer to handle this possibility, for example by filtering out entities with a null capacity before collecting them into a map.
| .stream() | |
| .collect(Collectors.toMap( | |
| ExamJpaEntity::getId, | |
| e -> e.getCapacity().longValue() | |
| )); | |
| .stream() | |
| .filter(e -> e.getCapacity() != null) | |
| .collect(Collectors.toMap( | |
| ExamJpaEntity::getId, | |
| e -> e.getCapacity().longValue() | |
| )); |
| CompletableFuture<Void> maxCapFuture = loadService.loadMaxCapacities(); | ||
| CompletableFuture<Void> currAppFuture = loadService.loadCurrentApplications(); | ||
|
|
||
| CompletableFuture.allOf(maxCapFuture, currAppFuture).join(); |
There was a problem hiding this comment.
While CompletableFuture.allOf(...).join() correctly waits for initialization and propagates exceptions, it would be beneficial for maintainability and debugging to wrap this call in a try-catch block. This allows for explicit logging of success or failure of the cache initialization process. A clear log message upon failure can significantly speed up diagnosing startup issues.
For example:
// Add @Slf4j to the class
try {
CompletableFuture.allOf(maxCapFuture, currAppFuture).join();
log.info("Exam quota cache initialized successfully.");
} catch (Exception e) {
log.error("Failed to initialize exam quota cache.", e);
throw e; // Re-throw to ensure application startup fails
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/main/java/life/mosu/mosuserver/application/exam/cache/ExamQuotaLoadService.java (1)
33-33: Consider using asynchronous execution properly.Returning
CompletableFuture.completedFuture(null)defeats the purpose of@Asyncannotation since all work is completed synchronously before returning the future.Consider restructuring to make the cache loading truly asynchronous:
- examQuotaCacheManager.loadMaxCapacities(maxCapacities); - return CompletableFuture.completedFuture(null); + return CompletableFuture.runAsync(() -> + examQuotaCacheManager.loadMaxCapacities(maxCapacities));Alternatively, if the current approach is intentional, consider removing the
@Asyncannotation and returningvoid.Also applies to: 46-46
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/java/life/mosu/mosuserver/application/exam/cache/ExamQuotaCacheManager.java(2 hunks)src/main/java/life/mosu/mosuserver/application/exam/cache/ExamQuotaLoadService.java(1 hunks)src/main/java/life/mosu/mosuserver/application/exam/initializer/ExamQuotaCacheInitializer.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/exam/entity/ExamJpaRepository.java(1 hunks)src/main/java/life/mosu/mosuserver/infra/persistence/redis/KeyValueCacheManager.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-deploy
🔇 Additional comments (3)
src/main/java/life/mosu/mosuserver/infra/persistence/redis/KeyValueCacheManager.java (1)
12-16: LGTM! Clean removal of abstract cache initialization.The removal of the abstract
initCache()method properly decouples cache initialization from cache management, allowing for more flexible initialization patterns through dedicated services.src/main/java/life/mosu/mosuserver/domain/exam/entity/ExamJpaRepository.java (1)
41-41: Good change to include all exams in the result set.Changing from
JOINtoRIGHT JOINensures that all exams are included in the count, even those with zero applications. This supports comprehensive cache pre-warming and reduces potential cache misses.src/main/java/life/mosu/mosuserver/application/exam/cache/ExamQuotaCacheManager.java (1)
91-97: LGTM! Clean separation of cache loading from cache management.The new
loadMaxCapacitiesandloadCurrentApplicationsmethods provide a clean interface for bulk cache population while delegating to existing setter methods. This maintains consistency and reuses existing validation/error handling logic.
| @Async | ||
| @Transactional(readOnly = true) | ||
| public CompletableFuture<Void> loadMaxCapacities() { | ||
| var maxCapacities = examJpaRepository.findByExamDateAfter(LocalDate.now()) | ||
| .stream() | ||
| .collect(Collectors.toMap( | ||
| ExamJpaEntity::getId, | ||
| e -> e.getCapacity().longValue() | ||
| )); | ||
| examQuotaCacheManager.loadMaxCapacities(maxCapacities); | ||
| return CompletableFuture.completedFuture(null); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for database operations.
Both methods lack error handling. Database failures could cause application startup to fail without clear error messages.
Consider adding try-catch blocks with appropriate logging:
@Async
@Transactional(readOnly = true)
public CompletableFuture<Void> loadMaxCapacities() {
+ try {
var maxCapacities = examJpaRepository.findByExamDateAfter(LocalDate.now())
.stream()
.filter(e -> e.getCapacity() != null)
.collect(Collectors.toMap(
ExamJpaEntity::getId,
e -> e.getCapacity().longValue()
));
examQuotaCacheManager.loadMaxCapacities(maxCapacities);
+ log.info("Successfully loaded {} max capacities to cache", maxCapacities.size());
+ } catch (Exception e) {
+ log.error("Failed to load max capacities to cache", e);
+ throw e;
+ }
return CompletableFuture.completedFuture(null);
}Also applies to: 36-47
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/application/exam/cache/ExamQuotaLoadService.java
around lines 23 to 34, the loadMaxCapacities method lacks error handling for
database operations, which could cause silent failures. Wrap the database call
and subsequent processing in a try-catch block, catch any exceptions, and log an
appropriate error message to ensure failures are visible and handled gracefully.
Apply the same pattern to the method spanning lines 36 to 47.
| var maxCapacities = examJpaRepository.findByExamDateAfter(LocalDate.now()) | ||
| .stream() | ||
| .collect(Collectors.toMap( | ||
| ExamJpaEntity::getId, | ||
| e -> e.getCapacity().longValue() | ||
| )); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null safety check for exam capacity.
The conversion e.getCapacity().longValue() could throw a NullPointerException if the capacity is null.
var maxCapacities = examJpaRepository.findByExamDateAfter(LocalDate.now())
.stream()
+ .filter(e -> e.getCapacity() != null)
.collect(Collectors.toMap(
ExamJpaEntity::getId,
e -> e.getCapacity().longValue()
));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var maxCapacities = examJpaRepository.findByExamDateAfter(LocalDate.now()) | |
| .stream() | |
| .collect(Collectors.toMap( | |
| ExamJpaEntity::getId, | |
| e -> e.getCapacity().longValue() | |
| )); | |
| var maxCapacities = examJpaRepository.findByExamDateAfter(LocalDate.now()) | |
| .stream() | |
| .filter(e -> e.getCapacity() != null) | |
| .collect(Collectors.toMap( | |
| ExamJpaEntity::getId, | |
| e -> e.getCapacity().longValue() | |
| )); |
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/application/exam/cache/ExamQuotaLoadService.java
around lines 26 to 31, the code calls e.getCapacity().longValue() without
checking if getCapacity() returns null, which can cause a NullPointerException.
To fix this, add a null check for e.getCapacity() before calling longValue(),
and provide a default value (e.g., 0L) if it is null to ensure null safety.
| public void run(ApplicationArguments args) { | ||
| CompletableFuture<Void> maxCapFuture = loadService.loadMaxCapacities(); | ||
| CompletableFuture<Void> currAppFuture = loadService.loadCurrentApplications(); | ||
|
|
||
| CompletableFuture.allOf(maxCapFuture, currAppFuture).join(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding timeout and error handling for startup robustness.
The current implementation blocks indefinitely on join() and lacks error handling, which could cause application startup failures or hangs.
Consider adding timeout and graceful error handling:
@Override
public void run(ApplicationArguments args) {
+ try {
CompletableFuture<Void> maxCapFuture = loadService.loadMaxCapacities();
CompletableFuture<Void> currAppFuture = loadService.loadCurrentApplications();
- CompletableFuture.allOf(maxCapFuture, currAppFuture).join();
+ CompletableFuture.allOf(maxCapFuture, currAppFuture)
+ .orTimeout(30, TimeUnit.SECONDS)
+ .join();
+ log.info("Cache initialization completed successfully");
+ } catch (Exception e) {
+ log.error("Cache initialization failed, application may have degraded performance", e);
+ // Consider whether to fail fast or continue with empty cache
+ }
}You'll need to add these imports:
import java.util.concurrent.TimeUnit;
import lombok.extern.slf4j.Slf4j;🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/application/exam/initializer/ExamQuotaCacheInitializer.java
around lines 20 to 25, the current use of CompletableFuture.allOf(...).join()
blocks indefinitely without timeout or error handling, risking startup hangs.
Modify the code to use a timeout with get(timeout, TimeUnit) instead of join(),
and wrap the call in a try-catch block to handle TimeoutException,
InterruptedException, and ExecutionException. Log any exceptions using a logger
(add @Slf4j annotation to the class) and ensure the application can handle these
errors gracefully during startup.
…ve exam by ID after a specific date
1 participant
✨ 구현한 기능
📢 논의하고 싶은 내용
🎸 기타
Summary by CodeRabbit
New Features
Improvements
Refactor