학생 수 캐싱 관리에 하한 상한 설정 처리 로직 추가 (with. Lua) #135
Conversation
…ota with error handling
…ication count management
…xamQuotaCacheManager
WalkthroughThis update introduces atomic increment and decrement operations for exam quota management using Redis Lua scripts. New operator classes and interfaces are added, a configuration class wires the scripts and operators, and the cache manager is refactored to use these atomic operations with enhanced error handling. Supporting enums, scripts, and exception codes are also introduced or updated. Changes
Sequence Diagram(s)sequenceDiagram
participant Service as ExamService
participant Manager as ExamQuotaCacheManager
participant Operator as AtomicExamQuotaIncrement/DecrementOperator
participant Redis as Redis (Lua Script)
Service->>Manager: increaseCurrentApplications(schoolName)
Manager->>Operator: execute(schoolName)
Operator->>Redis: Run increment_exam_quota.lua
Redis-->>Operator: Result (success/failure)
Operator-->>Manager: (throws on failure)
Manager-->>Service: (throws CACHE_UPDATE_FAIL on failure)
sequenceDiagram
participant Service as ExamService
participant Manager as ExamQuotaCacheManager
participant Operator as AtomicExamQuotaDecrementOperator
participant Redis as Redis (Lua Script)
Service->>Manager: decreaseCurrentApplications(schoolName)
Manager->>Operator: execute(schoolName)
Operator->>Redis: Run decrement_exam_quota.lua
Redis-->>Operator: Result (success/failure)
Operator-->>Manager: (throws on failure)
Manager-->>Service: (throws CACHE_UPDATE_FAIL on failure)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ 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!
This pull request significantly enhances the student quota caching mechanism by introducing atomic operations with enforced lower and upper bounds. It leverages Redis Lua scripting for transactional integrity and refactors the cache management layer to support a more robust and extensible atomic operator pattern.
Highlights
- Atomic Student Count Management: Implemented atomic increment and decrement operations for student counts in the cache using Redis Lua scripts. This ensures that concurrent updates to student quotas are handled safely and consistently.
- Quota Boundary Enforcement: Introduced logic within the Lua scripts to enforce lower and upper limits on student counts. The
decrement_exam_quota.luascript prevents the count from going below zero, andincrement_exam_quota.luaprevents it from exceeding the maximum capacity. - Refactored Cache Operations: The
ExamQuotaCacheManagerhas been refactored to utilize a newCacheAtomicOperatorpattern. This allows for more flexible and extensible management of atomic cache operations, moving away from directcacheWritercalls for increment/decrement. - Lua Script Configuration: Added a new
LuaScriptConfigto centralize the loading and configuration of Redis Lua scripts, making them easily injectable and manageable within the Spring application context.
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 introduces atomic operations for Redis using Lua scripts to manage exam application quotas. The code is well-structured with separate operators and a configuration class. The review includes suggestions to improve robustness, maintainability, and clarity by fixing misleading variable names, improving type safety, reducing duplication, and eliminating magic strings.
| private final DefaultRedisScript<Long> decrementScript; | ||
|
|
||
| public AtomicExamQuotaIncrementOperator( | ||
| RedisTemplate<String, Long> redisTemplate, | ||
| @Qualifier("incrementExamQuotaScript") DefaultRedisScript<Long> decrementScript | ||
| ) { | ||
| this.redisTemplate = redisTemplate; | ||
| this.decrementScript = decrementScript; | ||
| } |
There was a problem hiding this comment.
The field and constructor parameter decrementScript are misnamed in this increment operator class. This is confusing and error-prone as it suggests a decrement operation. Rename it to incrementScript to accurately reflect its purpose.
| private final DefaultRedisScript<Long> decrementScript; | |
| public AtomicExamQuotaIncrementOperator( | |
| RedisTemplate<String, Long> redisTemplate, | |
| @Qualifier("incrementExamQuotaScript") DefaultRedisScript<Long> decrementScript | |
| ) { | |
| this.redisTemplate = redisTemplate; | |
| this.decrementScript = decrementScript; | |
| } | |
| private final DefaultRedisScript<Long> incrementScript; | |
| public AtomicExamQuotaIncrementOperator( | |
| RedisTemplate<String, Long> redisTemplate, | |
| @Qualifier("incrementExamQuotaScript") DefaultRedisScript<Long> incrementScript | |
| ) { | |
| this.redisTemplate = redisTemplate; | |
| this.incrementScript = incrementScript; | |
| } |
|
|
||
| @Override | ||
| public void execute(String key) { | ||
| Long result = redisTemplate.execute(decrementScript, List.of( |
There was a problem hiding this comment.
Following the renaming of the decrementScript field to incrementScript, this usage should also be updated to avoid confusion and ensure the correct script is being executed.
| Long result = redisTemplate.execute(decrementScript, List.of( | |
| Long result = redisTemplate.execute(incrementScript, List.of( |
| VoidCacheAtomicOperator<String, Long> increment = (VoidCacheAtomicOperator<String, Long>) cacheAtomicOperatorMap.get( | ||
| "increment"); |
There was a problem hiding this comment.
The direct cast to VoidCacheAtomicOperator is unsafe and can lead to a ClassCastException at runtime if the map is misconfigured. It's safer to perform a type check before casting. This same issue exists in decreaseCurrentApplications on line 85.
| try { | ||
| decrement.execute(schoolName); | ||
| } catch (Exception e) { | ||
| log.error("Cache increment failed", e); |
There was a problem hiding this comment.
This log message appears to be a copy-paste error from the increaseCurrentApplications method. It should state that the "decrement" operation failed to avoid confusion during debugging.
| log.error("Cache increment failed", e); | |
| log.error("Cache decrement failed", e); |
| public DefaultRedisScript<Long> decrementExamQuotaScript() { | ||
| DefaultRedisScript<Long> script = new DefaultRedisScript<>(); | ||
| script.setResultType(Long.class); | ||
| try { | ||
| String lua = new String(decrementScript.getInputStream().readAllBytes(), StandardCharsets.UTF_8); | ||
| script.setScriptText(lua); | ||
| } catch (IOException e) { | ||
| throw new RuntimeException("Failed to load decrement_exam_quota.lua", e); | ||
| } | ||
| return script; | ||
| } | ||
|
|
||
| @Bean | ||
| @Qualifier("incrementExamQuotaScript") | ||
| public DefaultRedisScript<Long> incrementExamQuotaScript() { | ||
| DefaultRedisScript<Long> script = new DefaultRedisScript<>(); | ||
| script.setResultType(Long.class); | ||
| try { | ||
| String lua = new String(incrementScript.getInputStream().readAllBytes(), StandardCharsets.UTF_8); | ||
| script.setScriptText(lua); | ||
| } catch (IOException e) { | ||
| throw new RuntimeException("Failed to load increment_exam_quota.lua", e); | ||
| } | ||
| return script; | ||
| } |
There was a problem hiding this comment.
The logic for creating a DefaultRedisScript from a Resource is duplicated in decrementExamQuotaScript() and incrementExamQuotaScript(). Extract this into a private helper method to reduce code duplication and improve maintainability.
| "increment", incrementOp, | ||
| "decrement", decrementOp |
There was a problem hiding this comment.
The keys "increment" and "decrement" are hardcoded strings. This is brittle and can lead to runtime errors if there's a typo here or in ExamQuotaCacheManager where they are used. Use the getActionName() method from the operators themselves to define the keys, ensuring consistency.
| package life.mosu.mosuserver.infra.persistence.redis.support; | ||
|
|
||
| import java.util.List; | ||
| import lombok.RequiredArgsConstructor; | ||
| import org.springframework.data.redis.core.RedisTemplate; | ||
| import org.springframework.data.redis.core.script.DefaultRedisScript; | ||
| import org.springframework.stereotype.Component; | ||
|
|
||
| @Component | ||
| @RequiredArgsConstructor | ||
| public class LuaScriptExecutor<V> { | ||
|
|
||
| private final RedisTemplate<String, V> redisTemplate; | ||
|
|
||
| public Long execute(DefaultRedisScript<Long> script, List<String> keys, List<?> args) { | ||
| return redisTemplate.execute(script, keys, args); | ||
| } | ||
| } |
There was a problem hiding this comment.
This new class LuaScriptExecutor appears to be unused in the pull request, as the atomic operators implement their own script execution logic using RedisTemplate directly. If this class is dead code, remove it to avoid clutter.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (12)
src/main/java/life/mosu/mosuserver/infra/persistence/redis/operator/CacheAtomicOperator.java (1)
3-6: Consider adding documentation for method clarity.The distinction between
getName()andgetActionName()is not immediately clear from the interface. Consider adding JavaDoc comments to clarify their intended purposes.+/** + * Interface for atomic cache operations with identifiable names and actions. + */ public interface CacheAtomicOperator<K,V> { + /** + * @return the name of this cache operator + */ String getName(); + /** + * @return the action name performed by this operator + */ String getActionName(); }src/main/java/life/mosu/mosuserver/infra/config/LuaScriptConfig.java (4)
20-24: Consider externalizing script paths to application properties.Hard-coded script paths reduce flexibility. Consider moving these to application.yml for easier configuration management.
+# In application.yml +lua: + scripts: + decrement-exam-quota: classpath:scripts/decrement_exam_quota.lua + increment-exam-quota: classpath:scripts/increment_exam_quota.lua- @Value("classpath:scripts/decrement_exam_quota.lua") + @Value("${lua.scripts.decrement-exam-quota}") private Resource decrementScript; - @Value("classpath:scripts/increment_exam_quota.lua") + @Value("${lua.scripts.increment-exam-quota}") private Resource incrementScript;
31-36: Improve exception handling to preserve original cause.Wrapping
IOExceptioninRuntimeExceptionis appropriate for configuration failures, but consider using a more specific exception type and ensure the original cause is preserved.try { String lua = new String(decrementScript.getInputStream().readAllBytes(), StandardCharsets.UTF_8); script.setScriptText(lua); } catch (IOException e) { - throw new RuntimeException("Failed to load decrement_exam_quota.lua", e); + throw new IllegalStateException("Failed to load decrement_exam_quota.lua script", e); }
45-50: Apply consistent exception handling.Same improvement needed for increment script loading.
} catch (IOException e) { - throw new RuntimeException("Failed to load increment_exam_quota.lua", e); + throw new IllegalStateException("Failed to load increment_exam_quota.lua script", e); }
56-64: Simplify generic wildcard usage.The wildcard generic can be simplified for better readability.
- public Map<String, ? extends CacheAtomicOperator<String, Long>> examCacheAtomicOperatorMap( + public Map<String, CacheAtomicOperator<String, Long>> examCacheAtomicOperatorMap(src/main/java/life/mosu/mosuserver/global/exception/ErrorCode.java (1)
120-120: Consider HTTP status choice for cache failuresThe new
CACHE_UPDATE_FAILerror code is well-defined. However, consider whetherCONFLICT(409) is the most appropriate status for cache update failures.
INTERNAL_SERVER_ERROR(500) might be more suitable since cache update failures are typically internal system issues rather than client conflicts.CONFLICTis usually reserved for situations like resource conflicts or optimistic locking failures.-CACHE_UPDATE_FAIL(HttpStatus.CONFLICT, "캐시 업데이트에 실패하였습니다."),; +CACHE_UPDATE_FAIL(HttpStatus.INTERNAL_SERVER_ERROR, "캐시 업데이트에 실패하였습니다."),;src/main/resources/scripts/decrement_exam_quota.lua (1)
1-8: Review the validation logic and consider edge cases.The Lua script implementation is generally sound, but there are a few concerns:
Redundant GET operation: Line 1 performs a GET followed by DECR on line 8. The DECR command can handle non-existent keys by treating them as 0, so the nil check might be unnecessary.
Edge case handling: The current logic allows decrementing from 1 to 0, but prevents decrementing from 0. Consider if this aligns with business requirements.
Consider this simplified version that eliminates the redundant GET:
-local current = tonumber(redis.call('GET', KEYS[1])) -if current == nil then - return redis.error_reply("Current value is nil") -end -if current <= 0 then - return redis.error_reply("Current value is already zero or negative") -end -return redis.call('DECR', KEYS[1]) +local current = redis.call('GET', KEYS[1]) +if current == false then + return redis.error_reply("Key does not exist") +end +current = tonumber(current) +if current <= 0 then + return redis.error_reply("Current value is already zero or negative") +end +return redis.call('DECR', KEYS[1])src/main/java/life/mosu/mosuserver/infra/persistence/redis/support/LuaScriptExecutor.java (1)
11-17: Improve generic design and return type flexibility.The current design has some limitations:
- Inconsistent generics: The class is generic with type
Vbut the return type is hardcoded asLong.- Limited reusability: The hardcoded
Longreturn type restricts usage to scripts that return Long values.Consider making the class more flexible:
-public class LuaScriptExecutor<V> { +public class LuaScriptExecutor<V> { - public Long execute(DefaultRedisScript<Long> script, List<String> keys, List<?> args) { - return redisTemplate.execute(script, keys, args); + public <T> T execute(DefaultRedisScript<T> script, List<String> keys, List<?> args) { + return redisTemplate.execute(script, keys, args); }This makes the executor more generic and reusable for scripts with different return types.
src/main/java/life/mosu/mosuserver/application/exam/cache/AtomicExamQuotaIncrementOperator.java (1)
38-41: Improve error handling specificity.Consider using a more specific exception type or custom exception for better error handling and debugging.
if (result == null) { - throw new RuntimeException("Failed to execute increment Lua script"); + throw new CacheOperationException("Failed to execute increment Lua script for key: " + key); }src/main/java/life/mosu/mosuserver/application/exam/cache/AtomicExamQuotaDecrementOperator.java (1)
10-42: Well-implemented decrement operator with consistent design.The implementation is correct and consistent. The field naming, constructor injection, and key usage all align with the decrement operation purpose.
Consider the same error handling improvement suggested for the increment operator:
if (result == null) { - throw new RuntimeException("Failed to execute decrement Lua script"); + throw new CacheOperationException("Failed to execute decrement Lua script for key: " + key); }src/main/java/life/mosu/mosuserver/infra/persistence/redis/KeyValueCacheManager.java (1)
17-17: Consider consistent spacing in generic type declarationsThe generic type declaration has inconsistent spacing:
Map<String,? extends CacheAtomicOperator<K, V>>. Consider adding a space after the comma for consistency.- protected final Map<String,? extends CacheAtomicOperator<K, V>> cacheAtomicOperatorMap; + protected final Map<String, ? extends CacheAtomicOperator<K, V>> cacheAtomicOperatorMap;src/main/java/life/mosu/mosuserver/application/exam/cache/ExamQuotaCacheManager.java (1)
25-25: Consider method-level @transactional instead of class-levelHaving
@Transactionalat the class level makes all public methods transactional, which may not be necessary for read operations likegetMaxCapacityandgetCurrentApplications. Consider applying it only to methods that modify data.Remove class-level
@Transactionaland apply it to specific methods that need transactions:
setMaxCapacitysetCurrentApplicationsincreaseCurrentApplicationsdecreaseCurrentApplicationsdeleteMaxCapacitydeleteCurrentApplicationsinitCache
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
src/main/java/life/mosu/mosuserver/application/exam/ExamService.java(1 hunks)src/main/java/life/mosu/mosuserver/application/exam/cache/AtomicExamQuotaDecrementOperator.java(1 hunks)src/main/java/life/mosu/mosuserver/application/exam/cache/AtomicExamQuotaIncrementOperator.java(1 hunks)src/main/java/life/mosu/mosuserver/application/exam/cache/ExamQuotaCacheManager.java(3 hunks)src/main/java/life/mosu/mosuserver/application/exam/cache/ExamQuotaPrefix.java(1 hunks)src/main/java/life/mosu/mosuserver/application/exam/resolver/ExamQuotaEventResolver.java(1 hunks)src/main/java/life/mosu/mosuserver/global/exception/ErrorCode.java(1 hunks)src/main/java/life/mosu/mosuserver/infra/config/LuaScriptConfig.java(1 hunks)src/main/java/life/mosu/mosuserver/infra/persistence/redis/CacheEvictor.java(0 hunks)src/main/java/life/mosu/mosuserver/infra/persistence/redis/DefaultRedisCacheEvictor.java(1 hunks)src/main/java/life/mosu/mosuserver/infra/persistence/redis/DefaultRedisCacheLoader.java(1 hunks)src/main/java/life/mosu/mosuserver/infra/persistence/redis/DefaultRedisCacheReader.java(1 hunks)src/main/java/life/mosu/mosuserver/infra/persistence/redis/DefaultRedisCacheWriter.java(1 hunks)src/main/java/life/mosu/mosuserver/infra/persistence/redis/KeyValueCacheManager.java(1 hunks)src/main/java/life/mosu/mosuserver/infra/persistence/redis/operator/CacheAtomicOperator.java(1 hunks)src/main/java/life/mosu/mosuserver/infra/persistence/redis/operator/CacheEvictor.java(1 hunks)src/main/java/life/mosu/mosuserver/infra/persistence/redis/operator/CacheLoader.java(1 hunks)src/main/java/life/mosu/mosuserver/infra/persistence/redis/operator/CacheReader.java(1 hunks)src/main/java/life/mosu/mosuserver/infra/persistence/redis/operator/CacheWriter.java(1 hunks)src/main/java/life/mosu/mosuserver/infra/persistence/redis/operator/VoidCacheAtomicOperator.java(1 hunks)src/main/java/life/mosu/mosuserver/infra/persistence/redis/support/LuaScriptExecutor.java(1 hunks)src/main/resources/scripts/decrement_exam_quota.lua(1 hunks)src/main/resources/scripts/increment_exam_quota.lua(1 hunks)
💤 Files with no reviewable changes (1)
- src/main/java/life/mosu/mosuserver/infra/persistence/redis/CacheEvictor.java
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/main/java/life/mosu/mosuserver/application/exam/cache/AtomicExamQuotaIncrementOperator.java (1)
src/main/java/life/mosu/mosuserver/application/exam/cache/AtomicExamQuotaDecrementOperator.java (1)
Component(10-42)
🔇 Additional comments (21)
src/main/java/life/mosu/mosuserver/infra/persistence/redis/operator/CacheWriter.java (2)
1-1: Good package reorganization for better modularity.The move to the
operatorsubpackage improves code organization by grouping cache operation contracts together.
3-8: Well-designed cache writer interface.The interface provides a clean contract for cache writing operations with appropriate generic typing and clear method names.
src/main/java/life/mosu/mosuserver/infra/persistence/redis/DefaultRedisCacheWriter.java (2)
3-3: Correct import update for reorganized interface.The import correctly references the moved
CacheWriterinterface from the new operator package.
10-10: Good explicit interface implementation.Making the interface implementation explicit improves code clarity and ensures compile-time contract verification.
src/main/java/life/mosu/mosuserver/infra/persistence/redis/operator/VoidCacheAtomicOperator.java (1)
3-5: Well-designed atomic operation interface.Good use of interface composition by extending
CacheAtomicOperatorand adding the execution contract. The "Void" naming clearly indicates no return value.src/main/java/life/mosu/mosuserver/infra/config/LuaScriptConfig.java (1)
26-52: Lua script files verified in classpathThe Lua scripts
src/main/resources/scripts/decrement_exam_quota.luaandsrc/main/resources/scripts/increment_exam_quota.luaare present, so the beans will load without issues. No further action required.src/main/java/life/mosu/mosuserver/application/exam/ExamService.java (1)
4-4: Import path update looks correctThe new package path aligns with the cache refactor and keeps component-scanning intact (still under
life.mosu.mosuserver). No further action needed.src/main/java/life/mosu/mosuserver/infra/persistence/redis/operator/CacheReader.java (1)
1-1: Package relocation is fineThe move to
.operatorcleanly groups cache operator abstractions; no issues spotted.src/main/java/life/mosu/mosuserver/infra/persistence/redis/operator/CacheLoader.java (1)
1-1: Consistent packagingRelocation keeps the operator interfaces co-located. Implementation classes compile without change.
src/main/java/life/mosu/mosuserver/application/exam/resolver/ExamQuotaEventResolver.java (1)
4-4: Correct import – verify beans after refactorThe import now matches the new cache package. Just ensure Spring’s context picks up
ExamQuotaCacheManagerafter the move (it should, given the base package).src/main/java/life/mosu/mosuserver/infra/persistence/redis/DefaultRedisCacheReader.java (2)
4-4: LGTM: Clean interface reorganizationThe import update aligns with the refactoring that moves cache operation interfaces into the
operatorpackage for better organization.
11-11: LGTM: Explicit interface implementationMaking the interface implementation explicit improves code clarity and ensures proper contract adherence.
src/main/java/life/mosu/mosuserver/infra/persistence/redis/DefaultRedisCacheLoader.java (2)
4-4: LGTM: Consistent with interface reorganizationThe import change is consistent with moving cache operation interfaces to the
operatorpackage.
11-11: LGTM: Explicit interface implementation maintains contractThe explicit interface implementation ensures type safety and clear contract definition.
src/main/java/life/mosu/mosuserver/infra/persistence/redis/operator/CacheEvictor.java (1)
1-5: LGTM: Clean and focused interface designThe
CacheEvictor<K>interface follows good design principles:
- Single responsibility (cache eviction only)
- Generic type parameter for flexibility
- Clear and simple contract
This aligns well with the operator package reorganization.
src/main/resources/scripts/increment_exam_quota.lua (2)
8-10: LGTM: Proper bounds checkingThe upper bound validation correctly prevents exceeding the maximum capacity. The error message is clear and appropriate.
12-12: LGTM: Atomic increment operationUsing
INCRensures the increment operation is atomic and returns the new value correctly.src/main/java/life/mosu/mosuserver/application/exam/cache/ExamQuotaPrefix.java (1)
8-17: Well-designed enum with clean API.The implementation is clean and follows good practices. The
with()method provides a convenient way to construct cache keys.Consider adding null safety if needed:
public String with(String schoolName) { + if (schoolName == null || schoolName.trim().isEmpty()) { + throw new IllegalArgumentException("School name cannot be null or empty"); + } return prefix + schoolName; }src/main/java/life/mosu/mosuserver/infra/persistence/redis/KeyValueCacheManager.java (1)
13-24: Well-structured enhancement for atomic operationsThe addition of the
cacheAtomicOperatorMapfield and the generic type parameter change fromTtoVare well-implemented. The design maintains backward compatibility while extending functionality for atomic operations.src/main/java/life/mosu/mosuserver/application/exam/cache/ExamQuotaCacheManager.java (2)
1-1: Package restructuring looks goodMoving the class to the
cachesubpackage improves code organization by grouping cache-related classes together.
95-107: Well-implemented cache initializationThe
initCachemethod correctly loads both max capacity and current application counts from the database. The use of method references and forEach makes the code clean and readable.
| @Component | ||
| public class AtomicExamQuotaIncrementOperator implements VoidCacheAtomicOperator<String, Long> { | ||
| private final RedisTemplate<String, Long> redisTemplate; | ||
| private final DefaultRedisScript<Long> decrementScript; |
There was a problem hiding this comment.
Critical naming error: Field name doesn't match its purpose.
The field is named decrementScript but this is an increment operator. This is likely a copy-paste error from the decrement operator.
- private final DefaultRedisScript<Long> decrementScript;
+ private final DefaultRedisScript<Long> incrementScript;📝 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.
| private final DefaultRedisScript<Long> decrementScript; | |
| private final DefaultRedisScript<Long> incrementScript; |
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/application/exam/cache/AtomicExamQuotaIncrementOperator.java
at line 13, the field named decrementScript is incorrectly named as it
represents an increment operation. Rename the field to incrementScript to
accurately reflect its purpose and avoid confusion.
| @Qualifier("incrementExamQuotaScript") DefaultRedisScript<Long> decrementScript | ||
| ) { | ||
| this.redisTemplate = redisTemplate; | ||
| this.decrementScript = decrementScript; | ||
| } |
There was a problem hiding this comment.
Fix parameter name consistency.
The constructor parameter name should match the corrected field name.
public AtomicExamQuotaIncrementOperator(
RedisTemplate<String, Long> redisTemplate,
- @Qualifier("incrementExamQuotaScript") DefaultRedisScript<Long> decrementScript
+ @Qualifier("incrementExamQuotaScript") DefaultRedisScript<Long> incrementScript
) {
this.redisTemplate = redisTemplate;
- this.decrementScript = decrementScript;
+ this.incrementScript = incrementScript;
}📝 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.
| @Qualifier("incrementExamQuotaScript") DefaultRedisScript<Long> decrementScript | |
| ) { | |
| this.redisTemplate = redisTemplate; | |
| this.decrementScript = decrementScript; | |
| } | |
| public AtomicExamQuotaIncrementOperator( | |
| RedisTemplate<String, Long> redisTemplate, | |
| @Qualifier("incrementExamQuotaScript") DefaultRedisScript<Long> incrementScript | |
| ) { | |
| this.redisTemplate = redisTemplate; | |
| this.incrementScript = incrementScript; | |
| } |
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/application/exam/cache/AtomicExamQuotaIncrementOperator.java
around lines 17 to 21, the constructor parameter name 'decrementScript' does not
match the intended field name. Rename the constructor parameter to
'incrementExamQuotaScript' to maintain consistency with the field and the
qualifier annotation.
| Long result = redisTemplate.execute(decrementScript, List.of( | ||
| ExamQuotaPrefix.CURRENT_APPLICATIONS.with(key), | ||
| ExamQuotaPrefix.MAX_CAPACITY.with(key) | ||
| )); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Update script variable reference and verify key usage pattern.
The execute method should use the corrected field name. Also, note that the increment operator uses 2 keys while the decrement operator uses only 1 key - verify this is intentional based on the Lua script requirements.
- Long result = redisTemplate.execute(decrementScript, List.of(
+ Long result = redisTemplate.execute(incrementScript, List.of(
ExamQuotaPrefix.CURRENT_APPLICATIONS.with(key),
ExamQuotaPrefix.MAX_CAPACITY.with(key)
));Please verify that the increment Lua script requires both current applications and max capacity keys, while the decrement script only needs the current applications key.
🏁 Script executed:
#!/bin/bash
set -e
# Locate AtomicExamQuotaIncrementOperator class
inc_file=$(rg --files-with-matches "class AtomicExamQuotaIncrementOperator" -g "*.java")
echo "Increment operator file: $inc_file"
echo "---- File Content (first 200 lines) ----"
sed -n '1,200p' "$inc_file"
echo
# Show script field definitions in increment operator
echo "---- Script Fields in Increment Operator ----"
rg -n "Script" -C3 "$inc_file"
echo
# Show redisTemplate.execute calls in increment operator
echo "---- execute() Calls in Increment Operator ----"
rg -n "redisTemplate.execute" -C2 "$inc_file"
echo
# Locate AtomicExamQuotaDecrementOperator class
dec_file=$(rg --files-with-matches "class AtomicExamQuotaDecrementOperator" -g "*.java")
echo "Decrement operator file: $dec_file"
echo "---- File Content (first 200 lines) ----"
sed -n '1,200p' "$dec_file"
echo
# Show script field definitions in decrement operator
echo "---- Script Fields in Decrement Operator ----"
rg -n "Script" -C3 "$dec_file"
echo
# Show redisTemplate.execute calls in decrement operator
echo "---- execute() Calls in Decrement Operator ----"
rg -n "redisTemplate.execute" -C2 "$dec_file"
echo
# List Lua script files in the repository
echo "---- Lua Scripts ----"
find . -type f -iname "*.lua"
echo
# Show snippet of increment Lua script(s)
echo "---- Increment Lua Script Snippet ----"
find . -type f -iname "*increment*.lua" -exec sed -n '1,50p' {} \;
echo
# Show snippet of decrement Lua script(s)
echo "---- Decrement Lua Script Snippet ----"
find . -type f -iname "*decrement*.lua" -exec sed -n '1,50p' {} \;Length of output: 9161
Rename decrementScript to incrementScript in AtomicExamQuotaIncrementOperator and correct its usage
The increment operator is wired with the incrementExamQuotaScript bean but still named and invoked as decrementScript. Rename the field, constructor parameter, and execution call to incrementScript. The Lua scripts confirm that:
increment_exam_quota.luarequires 2 keys (current applications and max capacity).decrement_exam_quota.luarequires 1 key (current applications).
Locations to update in src/main/java/life/mosu/mosuserver/application/exam/cache/AtomicExamQuotaIncrementOperator.java:
- private final DefaultRedisScript<Long> decrementScript;
+ private final DefaultRedisScript<Long> incrementScript;
- public AtomicExamQuotaIncrementOperator(
- RedisTemplate<String, Long> redisTemplate,
- @Qualifier("incrementExamQuotaScript") DefaultRedisScript<Long> decrementScript
- ) {
- this.redisTemplate = redisTemplate;
- this.decrementScript = decrementScript;
+ public AtomicExamQuotaIncrementOperator(
+ RedisTemplate<String, Long> redisTemplate,
+ @Qualifier("incrementExamQuotaScript") DefaultRedisScript<Long> incrementScript
+ ) {
+ this.redisTemplate = redisTemplate;
+ this.incrementScript = incrementScript;
}
@Override
public void execute(String key) {
- Long result = redisTemplate.execute(decrementScript, List.of(
+ Long result = redisTemplate.execute(incrementScript, List.of(
ExamQuotaPrefix.CURRENT_APPLICATIONS.with(key),
ExamQuotaPrefix.MAX_CAPACITY.with(key)
));📝 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.
| Long result = redisTemplate.execute(decrementScript, List.of( | |
| ExamQuotaPrefix.CURRENT_APPLICATIONS.with(key), | |
| ExamQuotaPrefix.MAX_CAPACITY.with(key) | |
| )); | |
| // Field declaration | |
| private final DefaultRedisScript<Long> incrementScript; | |
| // Constructor | |
| public AtomicExamQuotaIncrementOperator( | |
| RedisTemplate<String, Long> redisTemplate, | |
| @Qualifier("incrementExamQuotaScript") DefaultRedisScript<Long> incrementScript | |
| ) { | |
| this.redisTemplate = redisTemplate; | |
| this.incrementScript = incrementScript; | |
| } | |
| @Override | |
| public void execute(String key) { | |
| Long result = redisTemplate.execute(incrementScript, List.of( | |
| ExamQuotaPrefix.CURRENT_APPLICATIONS.with(key), | |
| ExamQuotaPrefix.MAX_CAPACITY.with(key) | |
| )); | |
| // …handle result if needed | |
| } |
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/application/exam/cache/AtomicExamQuotaIncrementOperator.java
around lines 34 to 37, rename the field, constructor parameter, and all
references from decrementScript to incrementScript to correctly reflect the
increment operation. Also, update the redisTemplate.execute call to use
incrementScript instead of decrementScript, ensuring it passes the two required
keys (current applications and max capacity) as per the increment_exam_quota.lua
script.
| public void increaseCurrentApplications(String schoolName) { | ||
| String key = ExamQuotaPrefix.CURRENT_APPLICATIONS.with(schoolName); | ||
| cacheWriter.increase(key); | ||
| VoidCacheAtomicOperator<String, Long> increment = (VoidCacheAtomicOperator<String, Long>) cacheAtomicOperatorMap.get( | ||
| "increment"); | ||
| try { | ||
| increment.execute(schoolName); | ||
| } catch (Exception e) { | ||
| log.error("Cache increment failed", e); | ||
| throw new CustomRuntimeException(ErrorCode.CACHE_UPDATE_FAIL); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety and use constants for operator keys
Several improvements needed:
- Use constants instead of hardcoded strings
- Add type safety check before casting
- The error handling is appropriate
Consider defining constants for operator keys:
private static final String INCREMENT_OPERATOR = "increment";
private static final String DECREMENT_OPERATOR = "decrement";Then use them:
- VoidCacheAtomicOperator<String, Long> increment = (VoidCacheAtomicOperator<String, Long>) cacheAtomicOperatorMap.get(
- "increment");
+ CacheAtomicOperator<String, Long> operator = cacheAtomicOperatorMap.get(INCREMENT_OPERATOR);
+ if (!(operator instanceof VoidCacheAtomicOperator)) {
+ throw new IllegalStateException("Invalid operator type for increment");
+ }
+ VoidCacheAtomicOperator<String, Long> increment = (VoidCacheAtomicOperator<String, Long>) operator;🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/application/exam/cache/ExamQuotaCacheManager.java
around lines 73 to 82, replace the hardcoded string "increment" with a defined
constant like INCREMENT_OPERATOR to improve maintainability. Before casting the
retrieved operator from cacheAtomicOperatorMap, add a type check to ensure it is
an instance of VoidCacheAtomicOperator<String, Long> to avoid
ClassCastException. Keep the existing error handling as is.
| public void decreaseCurrentApplications(String schoolName) { | ||
| String key = ExamQuotaPrefix.CURRENT_APPLICATIONS.with(schoolName); | ||
| cacheWriter.decrease(key); | ||
| VoidCacheAtomicOperator<String, Long> decrement = (VoidCacheAtomicOperator<String, Long>) cacheAtomicOperatorMap.get( | ||
| "decrement"); | ||
| try { | ||
| decrement.execute(schoolName); | ||
| } catch (Exception e) { | ||
| log.error("Cache increment failed", e); | ||
| throw new CustomRuntimeException(ErrorCode.CACHE_UPDATE_FAIL); | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix error message and apply same improvements as increment method
The error log message is incorrect - it says "Cache increment failed" but should say "Cache decrement failed".
- log.error("Cache increment failed", e);
+ log.error("Cache decrement failed", e);Also apply the same type safety improvements suggested for the increment method.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/application/exam/cache/ExamQuotaCacheManager.java
around lines 84 to 93, update the error log message to "Cache decrement failed"
to correctly reflect the operation. Additionally, improve type safety by
avoiding unchecked casts for the decrement operator, similar to the increment
method's improvements, ensuring proper generic type handling and null checks
before usage.
| @@ -1,5 +1,6 @@ | |||
| package life.mosu.mosuserver.infra.persistence.redis; | |||
|
|
|||
| import life.mosu.mosuserver.infra.persistence.redis.operator.CacheEvictor; | |||
There was a problem hiding this comment.
Empty evict implementation – cache entry is never removed
DefaultRedisCacheEvictor#evict is still a no-op, so callers will believe a key was evicted while it remains in Redis.
@@
@Override
public void evict(String key) {
- }
+ if (key == null || key.isBlank()) {
+ return;
+ }
+ redisTemplate.delete(key);
+ }Implementing the delete prevents stale data and aligns with the purpose of the operator.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/infra/persistence/redis/DefaultRedisCacheEvictor.java
at line 3, the evict method is currently empty and does not remove cache entries
from Redis. To fix this, implement the evict method to actually delete the
specified key from Redis, ensuring that cache entries are properly removed and
preventing stale data from persisting.
| local current = tonumber(redis.call('GET', KEYS[1])) | ||
| local max_capacity = tonumber(redis.call('GET', KEYS[2])) | ||
|
|
||
| if current == nil or max_capacity == nil then | ||
| return redis.error_reply("Current or Max Capacity is nil") | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle initialization case for missing current count
The script returns an error when the current count key doesn't exist (nil), but for quota management, it might be more appropriate to treat a missing current count as 0 for the first increment operation.
Consider this approach:
local current = tonumber(redis.call('GET', KEYS[1])) or 0
local max_capacity = tonumber(redis.call('GET', KEYS[2]))
if max_capacity == nil then
return redis.error_reply("Max Capacity is nil")
endThis would allow the first increment to succeed when the current count key doesn't exist yet.
🤖 Prompt for AI Agents
In src/main/resources/scripts/increment_exam_quota.lua lines 1 to 6, the script
currently returns an error if the current count key is nil, but it should treat
a missing current count as 0 to allow the first increment. Modify the code to
set current to 0 if the GET call returns nil by using
"tonumber(redis.call('GET', KEYS[1])) or 0" and only return an error if
max_capacity is nil. This change enables proper initialization of the current
count for quota management.
| local current = tonumber(redis.call('GET', KEYS[1])) | ||
| local max_capacity = tonumber(redis.call('GET', KEYS[2])) | ||
|
|
||
| if current == nil or max_capacity == nil then | ||
| return redis.error_reply("Current or Max Capacity is nil") | ||
| end | ||
|
|
||
| if current >= max_capacity then | ||
| return redis.error_reply("Current value has reached the maximum capacity") | ||
| end | ||
|
|
||
| return redis.call('INCR', KEYS[1]) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify error handling for non-numeric values
The script uses tonumber() which returns nil for non-numeric strings. This will trigger the error condition, which is good for data integrity. However, consider adding more specific error messages to distinguish between missing keys and invalid data types.
🏁 Script executed:
#!/bin/bash
# Verify how the Lua script is used and if initialization is handled elsewhere
ast-grep --pattern 'AtomicExamQuotaIncrementOperator' -A 10Length of output: 3493
🏁 Script executed:
#!/bin/bash
# Locate and display the bean definition for incrementExamQuotaScript in LuaScriptConfig.java
rg -n "incrementExamQuotaScript" -A5 src/main/java/life/mosu/mosuserver/infra/config/LuaScriptConfig.javaLength of output: 509
Differentiate missing vs invalid values in Lua script
Splitting the nil‐check into two branches makes it clear whether the key was never set or contained non‐numeric data. For example:
-local current = tonumber(redis.call('GET', KEYS[1]))
-local max_capacity = tonumber(redis.call('GET', KEYS[2]))
-
-if current == nil or max_capacity == nil then
- return redis.error_reply("Current or Max Capacity is nil")
-end
+local raw_current = redis.call('GET', KEYS[1])
+local current = tonumber(raw_current)
+local raw_max = redis.call('GET', KEYS[2])
+local max_capacity = tonumber(raw_max)
+
+if raw_current == false then
+ return redis.error_reply("Missing current capacity for key: " .. KEYS[1])
+elseif current == nil then
+ return redis.error_reply("Invalid current capacity (not a number): " .. tostring(raw_current))
+end
+
+if raw_max == false then
+ return redis.error_reply("Missing max capacity for key: " .. KEYS[2])
+elseif max_capacity == nil then
+ return redis.error_reply("Invalid max capacity (not a number): " .. tostring(raw_max))
+end• This ensures you can immediately tell if a key was unset vs contained bad data.
• The rest of the logic (overflow check and INCR) remains unchanged.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/main/resources/scripts/increment_exam_quota.lua lines 1 to 12, separate
the nil check for current and max_capacity into two distinct checks to
differentiate between keys that are unset and keys that contain non-numeric
data. First, check if the redis call returns nil for each key and return an
error indicating the key is missing. Then, check if the values are not valid
numbers and return an error indicating invalid data. Keep the overflow check and
increment logic unchanged.
1 participant
✨ 구현한 기능
Summary by CodeRabbit
New Features
Refactor
Chores