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: Adjust @NonNullApi and @NonNullFields using package-info. #97

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

uhm0311
Copy link
Collaborator

@uhm0311 uhm0311 commented Aug 12, 2024

๐Ÿ”— Related Issue

โŒจ๏ธ What I did

  • package-info.java ํŒŒ์ผ์„ ์ถ”๊ฐ€ํ•˜์—ฌ ์ „์—ญ์œผ๋กœ non-null ์„ค์ •์„ ์ถ”๊ฐ€ํ•ฉ๋‹ˆ๋‹ค.
  • nullable ์„ค์ •์ด ํ•„์š”ํ•œ ๊ฒฝ์šฐ @Nullable์„ ๋ถ™์ž…๋‹ˆ๋‹ค.
  • @Nullable ์„ค์ •์„ ์–ด๋””๊นŒ์ง€ ๋ถ™์ผ์ง€๊ฐ€ ์• ๋งคํ•˜์—ฌ ๋…ผ์˜๊ฐ€ ํ•„์š”ํ•ฉ๋‹ˆ๋‹ค.

Comment on lines +36 to 41
@Nullable
private ArcusClientPool client;
@Nullable
private String url;
@Nullable
private String serviceCode;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

์ƒ์„ฑ์ž์—์„œ null์ด ์•„๋‹Œ ๊ฐ’์„ ๋„ฃ์–ด์ฃผ์ง€ ์•Š๊ธฐ ๋•Œ๋ฌธ์— ์ฒ˜์Œ ๊ฐ์ฒด๋ฅผ ์ƒ์„ฑํ•˜๋ฉด null์ด ๋“ค์–ด๊ฐ‘๋‹ˆ๋‹ค.
๊ทธ๋ž˜์„œ @Nullable์„ ๋ถ™์˜€์Šต๋‹ˆ๋‹ค.

Comment on lines +125 to +141

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);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.");

private String prefix;
private String serviceId;
private int expireSeconds;
private int frontExpireSeconds;
private long timeoutMilliSeconds = DEFAULT_TIMEOUT_MILLISECONDS;
private ArcusClientPool arcusClient;
private ArcusClientPool arcusClient = new ArcusClientPoolPlaceholder();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

arcusClient ํ•„๋“œ๊ฐ€ ๊ฐ€์žฅ @Nullable์„ ๋ถ™์ผ์ง€ ๋ง์ง€ ์• ๋งคํ•œ ํ•„๋“œ์ž…๋‹ˆ๋‹ค.
๋ณธ ๋ณ€๊ฒฝ์‚ฌํ•ญ์€ @Nullable์„ ๋ถ™์ด๋Š” ๋Œ€์‹  Placeholder ๊ฐœ๋…์˜ ์ดˆ๊ธฐ๊ฐ’์„ ๋„ฃ์–ด์ฃผ๋Š” ๊ฒƒ์ž…๋‹ˆ๋‹ค.

์ด ํ•„๋“œ์— @Nullable์„ ๋ถ™์ด๋ฉด IDE๋Š” ์บ์‹œ ์—ฐ์‚ฐ์„ ์œ„ํ•ด ์ด ํ•„๋“œ์— ์ฐธ์กฐํ•  ๋•Œ๋งˆ๋‹ค null ์—ฌ๋ถ€๋ฅผ ์ฒดํฌํ•ด์•ผ ํ•œ๋‹ค๊ณ  ํ•ฉ๋‹ˆ๋‹ค.
์ด ํ•„๋“œ์— @Nullable์„ ๋ถ™์ด์ง€ ์•Š์œผ๋ฉด ์ฒ˜์Œ์— null์ด ๋“ค์–ด๊ฐ€๊ณ  ์žˆ๋‹ค๊ณ  ์•Œ๋ฆฝ๋‹ˆ๋‹ค.
ArcusCacheManager๋ฅผ ์‚ฌ์šฉํ•˜์ง€ ์•Š๋Š” ๊ฒฝ์šฐ ๊ฐ์ฒด ์ƒ์„ฑ ์‹œ ์ฒ˜์Œ์—๋Š” null์ด ๋“ค์–ด๊ฐ€๊ธฐ ๋•Œ๋ฌธ์ž…๋‹ˆ๋‹ค.
๋‹จ, ArcusCacheManager๋ฅผ ์‚ฌ์šฉํ•˜๋Š” ๊ฒฝ์šฐ ์ƒ์„ฑ์ž์—์„œ null์ด ์•„๋‹Œ ๊ฐ’์„ ํ• ๋‹นํ•ฉ๋‹ˆ๋‹ค.

Copy link
Collaborator

Choose a reason for hiding this comment

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

์—ฌ๊ธฐ์— ๋Œ€ํ•œ ์ œ ์ƒ๊ฐ์€ ๋‹ค์Œ๊ณผ ๊ฐ™์Šต๋‹ˆ๋‹ค. @jhpark816๋‹˜๋„ ์˜๊ฒฌ ์ฃผ์‹œ๋ฉด ์ข‹์„ ๊ฒƒ ๊ฐ™์Šต๋‹ˆ๋‹ค.

  • Placeholder ๋Œ€์‹  @Nullable ์„ ๋ถ™์ž…๋‹ˆ๋‹ค. IDE๋ฅผ ์‚ฌ์šฉํ•˜์ง€ ์•Š๋Š” ๋ฐฐํฌ ํ™˜๊ฒฝ์—๋Š” ์˜ํ–ฅ์ด ์—†๊ธฐ ๋•Œ๋ฌธ์— ๋ฌธ์ œ๊ฐ€ ๋  ๊ฒƒ ๊ฐ™์ง€ ์•Š์Šต๋‹ˆ๋‹ค.
  • ์ธ์ž๊ฐ€ ์—†๋Š” public ์ƒ์„ฑ์ž๋ฅผ deprecateํ•˜๊ณ  ํ•„์ˆ˜ ์ธ์ž๋ฅผ ๋ฐ›๋Š” ์ƒ์„ฑ์ž๋ฅผ ์ œ๊ณตํ•ฉ๋‹ˆ๋‹ค.
  • ์ธ์ž๊ฐ€ ์—†๋Š” public ์ƒ์„ฑ์ž๊ฐ€ ์ œ๊ฑฐ๋˜๋Š” ์‹œ์ ์— ํ•„์ˆ˜ ์ธ์ž์— ๋Œ€ํ•œ @Nullable ๋„ ์ œ๊ฑฐํ•ฉ๋‹ˆ๋‹ค.

return 0;
}
return key.hashCode() & (mutexes.length - 1);
return Objects.hashCode(key) & (mutexes.length - 1);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

key ๋งค๊ฐœ๋ณ€์ˆ˜๋Š” ๊ฐœ๋…์ ์œผ๋กœ null์ด ์•„๋‹ˆ์–ด์•ผ ํ•ฉ๋‹ˆ๋‹ค.
ํ•˜์ง€๋งŒ null ์ฒดํฌํ•˜๋Š” if๋ฌธ์„ ์ œ๊ฑฐํ•˜๋ฉด, ๊ธฐ์กด์— null์„ ๋„ฃ๋˜ ๊ณณ์—์„œ NullPointerExcpetion์ด ๋ฐœ์ƒํ•ฉ๋‹ˆ๋‹ค.
Objects.hashCode() ๋ฉ”์†Œ๋“œ๋Š” ๋‚ด๋ถ€์ ์œผ๋กœ null ์ฒดํฌ๋ฅผ ํ•ด์ฃผ์–ด null์ธ ๊ฒฝ์šฐ 0์„ ๋ฐ˜ํ™˜ํ•˜๊ณ  ๊ทธ๋ ‡์ง€ ์•Š์œผ๋ฉด Object.hashCode() ๊ฐ’์„ ๋ฐ˜ํ™˜ํ•˜๋ฏ€๋กœ null์ธ ๊ฒฝ์šฐ์™€ ๊ทธ๋ ‡์ง€ ์•Š์€ ๊ฒฝ์šฐ ๋ชจ๋“  ๋™์ž‘์ด ๋™์ผํ•ฉ๋‹ˆ๋‹ค.

Comment on lines +107 to +108
this.name = this.getNamePlaceholder();
this.serviceId = this.getServiceIdPlaceholder();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

์ด ๋‘ ํ•„๋“œ๋Š” ๋‚ด๋ถ€ ๋กœ์ง์ ์œผ๋กœ null์ด ์•„๋‹Œ ๊ฐ’์œผ๋กœ ๋™์ž‘ํ•ฉ๋‹ˆ๋‹ค
ํ•˜์ง€๋งŒ ArcusCacheManager๋ฅผ ์‚ฌ์šฉํ•˜์ง€ ์•Š๋Š”๋‹ค๋ฉด ์ฒ˜์Œ์—๋Š” null์ด ๋“ค์–ด๊ฐ‘๋‹ˆ๋‹ค.
@Nullable์„ ๋ถ™์ด๋Š” ๋Œ€์‹  Placeholder ๊ฐœ๋…์˜ ์ดˆ๊ธฐ๊ฐ’์„ ๋„ฃ์–ด์ค๋‹ˆ๋‹ค.

Comment on lines +46 to +49
public ArcusCacheConfiguration() {
this.serviceId = this.getServiceIdPlaceholder();
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ArcusCache ์ƒ์„ฑ์ž์—์„œ serviceId์— Placeholder ๊ฐœ๋…์˜ ์ดˆ๊ธฐ๊ฐ’์„ ๋„ฃ์–ด์ฃผ๋Š” ๊ฒƒ๊ณผ ๋™์ผํ•ฉ๋‹ˆ๋‹ค.

@oliviarla
Copy link
Collaborator

@uhm0311 @jhpark816
ArcusClientFactoryBean ํด๋ž˜์Šค๋ฅผ Deprecate ์‹œํ‚ค๋Š” ๋ฐฉ๋ฒ•์€ ์–ด๋–ค๊ฐ€์š”?
์‚ฌ์šฉ ๋ฐฉ๋ฒ• ๋ฌธ์„œ์—๋Š” ArcusCacheManager๋ฅผ ๋นˆ์œผ๋กœ ๋“ฑ๋กํ•˜๋ผ๊ณ  ์„ค๋ช…ํ•˜๊ณ  ์žˆ๊ณ  ํ•˜์œ„ ํ˜ธํ™˜์„ฑ์„ ์ œ์™ธํ•˜๊ณ ๋Š” ์ด ํด๋ž˜์Šค๊ฐ€ ํ•„์š”ํ•œ์ง€ ์ž˜ ๋ชจ๋ฅด๊ฒ ์Šต๋‹ˆ๋‹ค.
๋˜ํ•œ ์ด ํด๋ž˜์Šค๋กœ ์ธํ•ด PlaceHolder๋ผ๋Š” ์š”์†Œ๋ฅผ ๋ผ์›Œ๋„ฃ๊ณ  Nullableํ•˜์ง€ ์•Š์•„์•ผ ํ•˜๋Š” ๊ณณ์— Nullable์„ ๋ถ™์ด๊ฒŒ ๋˜๋‹ค๋ณด๋‹ˆ, ๋‚˜์ค‘์— ์ฝ”๋“œ๋ฅผ ๋ณผ๋•Œ ํ—ท๊ฐˆ๋ฆด ๊ฒƒ ๊ฐ™์Šต๋‹ˆ๋‹ค.

@uhm0311
Copy link
Collaborator Author

uhm0311 commented Aug 12, 2024

FactoryBean์ด ์•„๋‹ˆ์–ด๋„ Placeholder๋ฅผ ๋‘๋Š” ๊ณณ์€ ์žˆ์Šต๋‹ˆ๋‹ค.

@kiheyunkim
Copy link
Contributor

์ง€๋‚œ 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
Copy link
Collaborator

@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
Copy link
Contributor

@uhm0311 @oliviarla
๋ณธ PR์€ ํ˜„ ์‹œ์ ์—์„œ ๋ฐ›์ง€ ์•Š๊ณ  ๋ณด๋ฅ˜ํ•ด ๋‘๊ฒ ์Šต๋‹ˆ๋‹ค.
NonNull, Nullable ๊ด€๋ จ annotation์„ ์˜๋ฏธ์ ์œผ๋กœ ์ž˜ ๋ถ™์ผ ์ˆ˜ ์žˆ๋„๋ก
๊ธฐ์กด ์ฝ”๋“œ๋ฅผ ๋จผ์ € ๋ฆฌํŒฉํ† ๋งํ•œ ํ›„, ๋ณธ PR์„ ๋‹ค์‹œ ์ ์šฉํ•˜๋ฉด ์ข‹๊ฒ ์Šต๋‹ˆ๋‹ค.

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.

4 participants