INTERNAL: Adjust `@NonNullApi` and `@NonNullFields` using package-info. by uhm0311 · Pull Request #97 · naver/arcus-spring

uhm0311 · 2024-08-12T06:36:36Z

🔗 Related Issue

⌨️ What I did

package-info.java 파일을 추가하여 전역으로 non-null 설정을 추가합니다.
nullable 설정이 필요한 경우 @Nullable을 붙입니다.
@Nullable 설정을 어디까지 붙일지가 애매하여 논의가 필요합니다.

uhm0311 · 2024-08-12T06:37:42Z

src/main/java/com/navercorp/arcus/spring/ArcusClientFactoryBean.java

+  @Nullable
  private ArcusClientPool client;
+  @Nullable
  private String url;
+  @Nullable
  private String serviceCode;


생성자에서 null이 아닌 값을 넣어주지 않기 때문에 처음 객체를 생성하면 null이 들어갑니다.
그래서 @Nullable을 붙였습니다.

uhm0311 · 2024-08-12T06:43:06Z

src/main/java/com/navercorp/arcus/spring/ArcusClientFactoryBean.java

+
+    ConnectionFactoryBuilder cfb = new ConnectionFactoryBuilder()
+            .setFrontCacheExpireTime(frontCacheExpireTime)
+            .setTimeoutExceptionThreshold(timeoutExceptionThreshold)
+            .setFrontCacheCopyOnRead(frontCacheCopyOnRead)
+            .setFrontCacheCopyOnWrite(frontCacheCopyOnWrite)
+            .setMaxReconnectDelay(maxReconnectDelay);
+
+    if (maxFrontCacheElements > 0) {
+      cfb.setMaxFrontCacheElements(maxFrontCacheElements);
+    }
+    if (globalTranscoder != null) {
+      cfb.setTranscoder(globalTranscoder);
+    }
+
+    client = ArcusClient.createArcusClientPool(url, serviceCode, cfb, poolSize);


ZK 접속 주소와 서비스 코드가 null이 아님을 검증하기 위해 ArcusClientPool 객체를 여기서 생성합니다.
기존과 같이 getObject() 메소드에서 생성하되, 프로퍼티의 검증 코드를 중복으로 복사하는 방법이 있습니다.
getObject() 메소드 내에서 afterPropertiesSet()을 호출하는 경우 IDE가 url과 serviceCode의 null 여부 검증을 인식하지 못해서 검증 코드를 복사해야 합니다.

Assert.notNull(this.url, "Url property must be provided."); Assert.notNull(this.serviceCode, "ServiceCode property must be provided."); Assert.isTrue(this.poolSize > 0, "PoolSize property must be larger than 0."); Assert.isTrue(this.timeoutExceptionThreshold > 0, "TimeoutExceptionThreshold must be larger than 0."); Assert.isTrue(this.maxReconnectDelay > 0, "MaxReconnectDelay must be larger than 0.");

uhm0311 · 2024-08-12T06:45:56Z

src/main/java/com/navercorp/arcus/spring/cache/ArcusCache.java

  private int frontExpireSeconds;
  private long timeoutMilliSeconds = DEFAULT_TIMEOUT_MILLISECONDS;
-  private ArcusClientPool arcusClient;
+  private ArcusClientPool arcusClient = new ArcusClientPoolPlaceholder();


arcusClient 필드가 가장 @Nullable을 붙일지 말지 애매한 필드입니다.
본 변경사항은 @Nullable을 붙이는 대신 Placeholder 개념의 초기값을 넣어주는 것입니다.

이 필드에 @Nullable을 붙이면 IDE는 캐시 연산을 위해 이 필드에 참조할 때마다 null 여부를 체크해야 한다고 합니다.
이 필드에 @Nullable을 붙이지 않으면 처음에 null이 들어가고 있다고 알립니다.
ArcusCacheManager를 사용하지 않는 경우 객체 생성 시 처음에는 null이 들어가기 때문입니다.
단, ArcusCacheManager를 사용하는 경우 생성자에서 null이 아닌 값을 할당합니다.

여기에 대한 제 생각은 다음과 같습니다. @jhpark816님도 의견 주시면 좋을 것 같습니다.

Placeholder 대신 @Nullable 을 붙입니다. IDE를 사용하지 않는 배포 환경에는 영향이 없기 때문에 문제가 될 것 같지 않습니다.

인자가 없는 public 생성자를 deprecate하고 필수 인자를 받는 생성자를 제공합니다.

인자가 없는 public 생성자가 제거되는 시점에 필수 인자에 대한 @Nullable 도 제거합니다.

src/main/java/com/navercorp/arcus/spring/cache/ArcusCache.java

src/main/java/com/navercorp/arcus/spring/cache/ArcusCacheConfiguration.java

uhm0311 · 2024-08-12T06:49:49Z

src/main/java/com/navercorp/arcus/spring/concurrent/DefaultKeyLockProvider.java

-      return 0;
-    }
-    return key.hashCode() & (mutexes.length - 1);
+    return Objects.hashCode(key) & (mutexes.length - 1);


key 매개변수는 개념적으로 null이 아니어야 합니다.
하지만 null 체크하는 if문을 제거하면, 기존에 null을 넣던 곳에서 NullPointerExcpetion이 발생합니다.
Objects.hashCode() 메소드는 내부적으로 null 체크를 해주어 null인 경우 0을 반환하고 그렇지 않으면 Object.hashCode() 값을 반환하므로 null인 경우와 그렇지 않은 경우 모든 동작이 동일합니다.

uhm0311 · 2024-08-12T06:58:33Z

src/main/java/com/navercorp/arcus/spring/cache/ArcusCache.java

+    this.name = this.getNamePlaceholder();
+    this.serviceId = this.getServiceIdPlaceholder();


이 두 필드는 내부 로직적으로 null이 아닌 값으로 동작합니다
하지만 ArcusCacheManager를 사용하지 않는다면 처음에는 null이 들어갑니다.
@Nullable을 붙이는 대신 Placeholder 개념의 초기값을 넣어줍니다.

uhm0311 · 2024-08-12T06:59:56Z

src/main/java/com/navercorp/arcus/spring/cache/ArcusCacheConfiguration.java

+  public ArcusCacheConfiguration() {
+    this.serviceId = this.getServiceIdPlaceholder();
+  }
+


ArcusCache 생성자에서 serviceId에 Placeholder 개념의 초기값을 넣어주는 것과 동일합니다.

oliviarla · 2024-08-12T08:17:39Z

@uhm0311 @jhpark816
ArcusClientFactoryBean 클래스를 Deprecate 시키는 방법은 어떤가요?
사용 방법 문서에는 ArcusCacheManager를 빈으로 등록하라고 설명하고 있고 하위 호환성을 제외하고는 이 클래스가 필요한지 잘 모르겠습니다.
또한 이 클래스로 인해 PlaceHolder라는 요소를 끼워넣고 Nullable하지 않아야 하는 곳에 Nullable을 붙이게 되다보니, 나중에 코드를 볼때 헷갈릴 것 같습니다.

uhm0311 · 2024-08-12T08:18:56Z

FactoryBean이 아니어도 Placeholder를 두는 곳은 있습니다.

kiheyunkim · 2024-08-30T15:47:53Z

지난 PR변경사항들과 JDK8버전으로 올라간 develop 브랜치상 기록을 봤을 때 org.springframework.lang.Nullable이 사용가능하고 또
이번 PR에서 해당 어노테이션 사용으로 변경되었으므로, #61 PR에서 @Nullable 만을 위해 추가됐던 아래의 모듈도 함께 제거 되어도 될 것 같습니다.

<dependency>
            <groupId>com.google.code.findbugs</groupId>
            <artifactId>jsr305</artifactId>
            <version>${com.google.code.findbugs.version}</version>
           <scope>provided</scope>
 </dependency>

oliviarla · 2024-09-04T03:22:41Z

@kiheyunkim
안녕하세요, 리뷰 의견 주셔서 감사합니다.

Arcus Spring의 경우 Compile Warning이 발생하면 컴파일이 되지 않도록 설정해두고 개발하고 있습니다.
그런데 jsr305에 대한 의존성을 제거하면 unknown enum constant javax.annotation.meta.When.MAYBE 컴파일 워닝이 발생하기 때문에 제거할 수 없는 상황입니다.

Spring 문서에서도 이에 관련한 내용을 다루고 있습니다.
https://github.com/spring-projects/spring-framework/blob/main/framework-docs/modules/ROOT/pages/core/null-safety.adoc#jsr-305-meta-annotations

jhpark816 · 2024-09-05T03:02:09Z

@uhm0311 @oliviarla
본 PR은 현 시점에서 받지 않고 보류해 두겠습니다.
NonNull, Nullable 관련 annotation을 의미적으로 잘 붙일 수 있도록
기존 코드를 먼저 리팩토링한 후, 본 PR을 다시 적용하면 좋겠습니다.

INTERNAL: Adjust @NonNullApi and @NonNullFields using package-info.

b00a75f

uhm0311 force-pushed the uhm0311/jdk branch from 07edc8b to b00a75f Compare August 12, 2024 06:57

uhm0311 commented Aug 12, 2024

View reviewed changes

uhm0311 requested review from brido4125 and oliviarla August 12, 2024 07:01

		this.name = this.getNamePlaceholder();
		this.serviceId = this.getServiceIdPlaceholder();

Comments

Conversation

uhm0311 commented Aug 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Related Issue

⌨️ What I did

Uh oh!

uhm0311 Aug 12, 2024

Choose a reason for hiding this comment

Uh oh!

uhm0311 Aug 12, 2024

Choose a reason for hiding this comment

Uh oh!

uhm0311 Aug 12, 2024

Choose a reason for hiding this comment

Uh oh!

oliviarla Sep 4, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

uhm0311 Aug 12, 2024

Choose a reason for hiding this comment

Uh oh!

uhm0311 Aug 12, 2024

Choose a reason for hiding this comment

Uh oh!

uhm0311 Aug 12, 2024

Choose a reason for hiding this comment

Uh oh!

oliviarla commented Aug 12, 2024

Uh oh!

uhm0311 commented Aug 12, 2024

Uh oh!

kiheyunkim commented Aug 30, 2024

Uh oh!

oliviarla commented Sep 4, 2024

Uh oh!

jhpark816 commented Sep 5, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

uhm0311 commented Aug 12, 2024 •

edited

Loading