Skip to content

[#1387] improvement: compatibility with jdk8 when call JavaUtils.newConcurrentMap - #1389

Merged
zuston merged 5 commits into
apache:masterfrom
lifeSo:feature_jdk8_1382
Dec 21, 2023
Merged

[#1387] improvement: compatibility with jdk8 when call JavaUtils.newConcurrentMap#1389
zuston merged 5 commits into
apache:masterfrom
lifeSo:feature_jdk8_1382

Conversation

@lifeSo

@lifeSo lifeSo commented Dec 20, 2023

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?
compatibility with jdk8 when call JavaUtils.newConcurrentMap

Why are the changes needed?
Fix: #1387

Does this PR introduce any user-facing change?
No.

How was this patch tested?

@lifeSo

lifeSo commented Dec 20, 2023

Copy link
Copy Markdown
Contributor Author

Fix The error:

image

@codecov-commenter

codecov-commenter commented Dec 20, 2023

Copy link
Copy Markdown

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (ecbf2e7) 53.22% compared to head (3f97769) 54.11%.
Report is 10 commits behind head on master.

Files Patch % Lines
...java/org/apache/uniffle/common/util/JavaUtils.java 20.00% 1 Missing and 3 partials ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #1389      +/-   ##
============================================
+ Coverage     53.22%   54.11%   +0.89%     
- Complexity     2715     2720       +5     
============================================
  Files           418      399      -19     
  Lines         23932    21628    -2304     
  Branches       2043     2050       +7     
============================================
- Hits          12738    11705    -1033     
+ Misses        10408     9205    -1203     
+ Partials        786      718      -68     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

private static final String JAVA_9 = "JAVA_9";

public static boolean isJavaVersionAtLeastJava9() {
if (Enums.getIfPresent(JavaVersion.class, JAVA_9).isPresent()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why we should two checkings rather than 1?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

image

for fix this error .

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Enums.getIfPresent(JavaVersion.class, JAVA_9).isPresent()

is check is "JAVA_9" in enum JavaVersion.

SystemUtils.isJavaVersionAtLeast(JavaVersion.JAVA_9)
is check is system support jdk9

public static boolean isJavaVersionAtLeastJava9() {
if (Enums.getIfPresent(JavaVersion.class, JAVA_9).isPresent()
&& SystemUtils.isJavaVersionAtLeast(JavaVersion.JAVA_9)) {
logger.info("Jdk version is at least jdk9");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The log is not necessary

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok, I removed it.

logger.info("Jdk version is at least jdk9");
return true;
} else {
logger.info("Jdk version is jdk8");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ditto

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok, I removed it.

@zuston zuston left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@roryqi roryqi changed the title [1387] [Bug] compatibility with jdk8 when call JavaUtils.newConcurrentMap [1387] improvement: compatibility with jdk9 when call JavaUtils.newConcurrentMap Dec 20, 2023
@roryqi roryqi changed the title [1387] improvement: compatibility with jdk9 when call JavaUtils.newConcurrentMap [1387] improvement: compatibility with jdk8 when call JavaUtils.newConcurrentMap Dec 20, 2023
@zuston
zuston merged commit f9c1393 into apache:master Dec 21, 2023
@zuston

zuston commented Dec 21, 2023

Copy link
Copy Markdown
Member

Thanks @lifeSo Merged

@zuston zuston changed the title [1387] improvement: compatibility with jdk8 when call JavaUtils.newConcurrentMap [#1387] improvement: compatibility with jdk8 when call JavaUtils.newConcurrentMap Dec 21, 2023
@lifeSo
lifeSo deleted the feature_jdk8_1382 branch December 21, 2023 16:22
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.

[Bug] compatibility with jdk8 when call JavaUtils.newConcurrentMap

3 participants