Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

INTERNAL: Log info level when null value is present to ArcusCache.putIfAbsent() method. #88

Closed
wants to merge 1 commit into from

Conversation

uhm0311
Copy link
Collaborator

@uhm0311 uhm0311 commented Jun 14, 2024

🔗 Related Issue

  • putValue()에서는 값이 null인 경우 info 레벨 로그를 찍고 아무런 연산을 수행하지 않습니다.
  • 반면 putIfAbsent()에서는 값이 null인 경우 IllegalArgumentException을 발생시키고 있습니다.

⌨️ What I did

  • null 값을 저장하려고 할 때, putIfAbsent()에서도 putValue()에서와 같은 동작을 하도록 변경합니다.
  • 다른 옵션으로는 값이 null일 때 어차피 arcus-java-client에서 Exception이 발생하므로 putIfAbsent() 내에서 미리 null 체크를 하지 않는 방법이 있습니다.
    • 이 경우 wantToGetException 값에 따라 Exception이 전파되거나 혹은 로깅만 합니다.
  • 혹은 null 체크를 수행하여 IllegalArgumentException을 발생시키는 구문을 try-catch 내에 넣어 wantToGetException 필드를 적용시킬 수 있습니다.

@uhm0311 uhm0311 requested review from jhpark816 and oliviarla June 14, 2024 09:26
@uhm0311 uhm0311 force-pushed the uhm0311/develop2 branch from 07cf311 to 241a379 Compare June 14, 2024 09:36
Copy link
Contributor

@jhpark816 jhpark816 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

리뷰 완료

@@ -217,7 +217,8 @@ public ValueWrapper putIfAbsent(Object key, Object value) {
logger.debug("trying to add key: {}", arcusKey);

if (value == null) {
throw new IllegalArgumentException("arcus cannot add NULL value. key: " + arcusKey);
logger.info("arcus cannot putIfAbsent NULL value. key: {}", arcusKey);
return null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

null 리턴하면, putIfAbsent() 수행이 성공했다는 의미가 될 것 같습니다.
문제가 없나요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

반환 값 변경했습니다.

@uhm0311 uhm0311 force-pushed the uhm0311/develop2 branch from 241a379 to dde0095 Compare June 18, 2024 13:17
@jhpark816 jhpark816 requested a review from oliviarla June 19, 2024 00:27
@@ -215,7 +216,8 @@ public ValueWrapper putIfAbsent(Object key, Object value) {
logger.debug("trying to add key: {}", arcusKey);

if (value == null) {
throw new IllegalArgumentException("arcus cannot add NULL value. key: " + arcusKey);
logger.info("arcus cannot putIfAbsent NULL value. key: {}", arcusKey);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

redis, couchbase에서는 value 인자에 null이 입력되었고 null 저장을 허용하지 않으면, 아래와 같이 get 메서드를 호출합니다.
저희는 아직 allowNullValue 속성이 없으므로 일단은 get 메서드를 호출하도록 변경하는게 어떤가요?

@Override
public ValueWrapper putIfAbsent(final Object key, final Object value) {
  if (!isAllowNullValues() && value == null) {
    return get(key);
  }
  // ...
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

putIfAbsent()는 어떤 경우에 호출되는 지 확인 되었나요?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cache 객체를 CacheManager의 getCache(String name) 메서드를 통해 얻어 아래와 같이 사용할 수 있습니다. 내부적으로 AOP 처리를 위해 호출되는 경우는 없습니다.

Cache cache = cacheManager.getCache("myCache");
cache.putIfAbsent(key, value);

@oliviarla
Copy link
Collaborator

@uhm0311
여기는 close해도 되나요?

@uhm0311
Copy link
Collaborator Author

uhm0311 commented Jun 19, 2024

@oliviarla

통합한 PR이 반영되면 닫겠습니다.

@uhm0311
Copy link
Collaborator Author

uhm0311 commented Jun 19, 2024

#90

위 PR이 반영되어 닫습니다.

@uhm0311 uhm0311 closed this Jun 19, 2024
@uhm0311 uhm0311 deleted the uhm0311/develop2 branch June 20, 2024 01:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants