You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@paimon.apache.org by "legendtkl (via GitHub)" <gi...@apache.org> on 2023/03/24 18:09:16 UTC
[GitHub] [incubator-paimon] legendtkl opened a new pull request, #714: [WIP][core] support JDK 11
legendtkl opened a new pull request, #714:
URL: https://github.com/apache/incubator-paimon/pull/714
### Purpose
Support JDK11
issue ref: https://github.com/apache/incubator-paimon/issues/693
### Tests
regression it/e2e case
### API and Format
no
### Documentation
ye
### Note
all the java code change is because different behavior between jdk8 and jdk11:
1. `LocalDateTime.now()` return micro seconds in jdk11 (only return millisecond in jdk8), and the DataFileMeta serialize and deserialize both specify the TimeStamp(3). In jdk11, all the related cases will be broken.
2. Changes in `CachedRandomInputView.java` is to fix concurrency issues.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-paimon] s7monk commented on pull request #714: [WIP][core] support JDK 11
Posted by "s7monk (via GitHub)" <gi...@apache.org>.
s7monk commented on PR #714:
URL: https://github.com/apache/incubator-paimon/pull/714#issuecomment-1483715103
I also think it is too radical to completely abandon JDK8, because many companies and individual developers are still using JDK8
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-paimon] legendtkl commented on pull request #714: [core] support JDK 11
Posted by "legendtkl (via GitHub)" <gi...@apache.org>.
legendtkl commented on PR #714:
URL: https://github.com/apache/incubator-paimon/pull/714#issuecomment-1486258064
> Thanks @legendtkl for the contribution! I am very pleased to be able to officially announce support for JDK11! But one thing is that still JDK8 is currently the main force, as mentioned by the partners of the review above.
>
> Another thing is that I don't really want to cause too many tests, is it possible to provide the ability to test with JDK11, but by default it needs to be enabled manually?
@JingsongLi OK, let me have a check.
By the way, would you help review this PR either too? I have changed some code written by you. Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-paimon] SteNicholas commented on pull request #714: [core] support JDK 11
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on PR #714:
URL: https://github.com/apache/incubator-paimon/pull/714#issuecomment-1491237189
@legendtkl, as the description mentioned, is the changes of `CachedRandomInputView` related to the support of JDK 11?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-paimon] legendtkl closed pull request #714: [core] support JDK 11
Posted by "legendtkl (via GitHub)" <gi...@apache.org>.
legendtkl closed pull request #714: [core] support JDK 11
URL: https://github.com/apache/incubator-paimon/pull/714
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-paimon] legendtkl commented on a diff in pull request #714: [core] support JDK 11
Posted by "legendtkl (via GitHub)" <gi...@apache.org>.
legendtkl commented on code in PR #714:
URL: https://github.com/apache/incubator-paimon/pull/714#discussion_r1151320572
##########
paimon-spark/paimon-spark-2/pom.xml:
##########
@@ -84,6 +89,11 @@ under the License.
<version>${jackson.version}</version>
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>org.apache.xbean</groupId>
+ <artifactId>xbean-asm6-shaded</artifactId>
+ <version>${asm6.version}</version>
Review Comment:
@JingsongLi This package is to replace the package in `spark-sql_2.11`, so keep the scope same with `spark-sql_2.11`, namely default `compile`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-paimon] SteNicholas commented on pull request #714: [core] support JDK 11
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on PR #714:
URL: https://github.com/apache/incubator-paimon/pull/714#issuecomment-1489940669
@legendtkl, I left one minor comment above. Please resolve the conflicts and address above comment. PTAL.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-paimon] legendtkl commented on a diff in pull request #714: [WIP][core] support JDK 11
Posted by "legendtkl (via GitHub)" <gi...@apache.org>.
legendtkl commented on code in PR #714:
URL: https://github.com/apache/incubator-paimon/pull/714#discussion_r1148349990
##########
.github/workflows/e2e-tests-1.14.yml:
##########
@@ -32,10 +32,11 @@ jobs:
steps:
- name: Checkout code
uses: actions/checkout@v2
- - name: Set up JDK 1.8
- uses: actions/setup-java@v1
+ - name: Set up JDK 11
+ uses: actions/setup-java@v2
with:
- java-version: 1.8
+ java-version: '11'
Review Comment:
@s7monk ok, let's keep both now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-paimon] JingsongLi commented on a diff in pull request #714: [core] support JDK 11
Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.
JingsongLi commented on code in PR #714:
URL: https://github.com/apache/incubator-paimon/pull/714#discussion_r1151307139
##########
paimon-spark/paimon-spark-2/pom.xml:
##########
@@ -84,6 +89,11 @@ under the License.
<version>${jackson.version}</version>
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>org.apache.xbean</groupId>
+ <artifactId>xbean-asm6-shaded</artifactId>
+ <version>${asm6.version}</version>
Review Comment:
which scope? test or provided?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-paimon] legendtkl commented on a diff in pull request #714: [core] support JDK 11
Posted by "legendtkl (via GitHub)" <gi...@apache.org>.
legendtkl commented on code in PR #714:
URL: https://github.com/apache/incubator-paimon/pull/714#discussion_r1147900649
##########
.github/workflows/e2e-tests-1.14.yml:
##########
@@ -32,10 +32,11 @@ jobs:
steps:
- name: Checkout code
uses: actions/checkout@v2
- - name: Set up JDK 1.8
- uses: actions/setup-java@v1
+ - name: Set up JDK 11
+ uses: actions/setup-java@v2
with:
- java-version: 1.8
+ java-version: '11'
Review Comment:
@TyrantLucifer Yes, CI for both JDK8 and JDK11 will take a long time.
I think we can adopt JDK11 only lately, and no need to support JDK8 in the future.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-paimon] legendtkl closed pull request #714: [core] support JDK 11
Posted by "legendtkl (via GitHub)" <gi...@apache.org>.
legendtkl closed pull request #714: [core] support JDK 11
URL: https://github.com/apache/incubator-paimon/pull/714
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-paimon] SteNicholas merged pull request #714: [core] support JDK 11
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas merged PR #714:
URL: https://github.com/apache/incubator-paimon/pull/714
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-paimon] legendtkl commented on pull request #714: [core] support JDK 11
Posted by "legendtkl (via GitHub)" <gi...@apache.org>.
legendtkl commented on PR #714:
URL: https://github.com/apache/incubator-paimon/pull/714#issuecomment-1488124686
Hi, @JingsongLi , I have addressed the review comments, would you help review again? thanksl
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-paimon] legendtkl commented on a diff in pull request #714: [core] support JDK 11
Posted by "legendtkl (via GitHub)" <gi...@apache.org>.
legendtkl commented on code in PR #714:
URL: https://github.com/apache/incubator-paimon/pull/714#discussion_r1149221281
##########
paimon-common/src/main/java/org/apache/paimon/io/cache/CachedRandomInputView.java:
##########
@@ -55,7 +55,7 @@ public CachedRandomInputView(File file, CacheManager cacheManager)
this.file = new RandomAccessFile(file, "r");
this.fileLength = file.length();
this.cacheManager = cacheManager;
- this.segments = new HashMap<>();
+ this.segments = new ConcurrentHashMap<>();
Review Comment:
@SteNicholas ok
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-paimon] s7monk commented on a diff in pull request #714: [WIP][core] support JDK 11
Posted by "s7monk (via GitHub)" <gi...@apache.org>.
s7monk commented on code in PR #714:
URL: https://github.com/apache/incubator-paimon/pull/714#discussion_r1148309830
##########
.github/workflows/e2e-tests-1.14.yml:
##########
@@ -32,10 +32,11 @@ jobs:
steps:
- name: Checkout code
uses: actions/checkout@v2
- - name: Set up JDK 1.8
- uses: actions/setup-java@v1
+ - name: Set up JDK 11
+ uses: actions/setup-java@v2
with:
- java-version: 1.8
+ java-version: '11'
Review Comment:
I also think it is too radical to completely abandon JDK8, because many companies and individual developers are still using JDK8
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-paimon] legendtkl commented on pull request #714: [core] support JDK 11
Posted by "legendtkl (via GitHub)" <gi...@apache.org>.
legendtkl commented on PR #714:
URL: https://github.com/apache/incubator-paimon/pull/714#issuecomment-1490343968
> @legendtkl, I left one minor comment above. Please resolve the conflicts and address above comment. PTAL.
hi, @SteNicholas , I have updated the code, would you help review it again? cc @JingsongLi
Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-paimon] TyrantLucifer commented on a diff in pull request #714: [WIP][core] support JDK 11
Posted by "TyrantLucifer (via GitHub)" <gi...@apache.org>.
TyrantLucifer commented on code in PR #714:
URL: https://github.com/apache/incubator-paimon/pull/714#discussion_r1147910547
##########
.github/workflows/e2e-tests-1.14.yml:
##########
@@ -32,10 +32,11 @@ jobs:
steps:
- name: Checkout code
uses: actions/checkout@v2
- - name: Set up JDK 1.8
- uses: actions/setup-java@v1
+ - name: Set up JDK 11
+ uses: actions/setup-java@v2
with:
- java-version: 1.8
+ java-version: '11'
Review Comment:
Specifying multiple Java versions can result in two tasks running simultaneously, utilizing parallel processing. Moreover, in 2023, GitHub has increased action resources for Apache projects, so theoretically, this should not significantly increase the time required for continuous integration. Additionally, many users are still using Java 8. Therefore, completely abandoning Java 8 may be too drastic of a strategy. :joy:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-paimon] SteNicholas commented on a diff in pull request #714: [core] support JDK 11
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on code in PR #714:
URL: https://github.com/apache/incubator-paimon/pull/714#discussion_r1152945951
##########
paimon-common/src/main/java/org/apache/paimon/io/cache/CachedRandomInputView.java:
##########
@@ -55,7 +55,7 @@ public CachedRandomInputView(File file, CacheManager cacheManager)
this.file = new RandomAccessFile(file, "r");
this.fileLength = file.length();
this.cacheManager = cacheManager;
- this.segments = new HashMap<>();
+ this.segments = new ConcurrentHashMap<>();
Review Comment:
@legendtkl, after the @JingsongLi comments, does this need to change to `ConcurrentHashMap`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-paimon] legendtkl commented on pull request #714: [core] support JDK 11
Posted by "legendtkl (via GitHub)" <gi...@apache.org>.
legendtkl commented on PR #714:
URL: https://github.com/apache/incubator-paimon/pull/714#issuecomment-1484392096
hi, @SteNicholas
1. CI failure is because of the unstable UT described in this issue: https://github.com/apache/incubator-paimon/issues/657. I am not sure the failure frequency of occurrence, would you help re-run the CI? thanks.
2. sun.misc.Unsafe is still available in JDK11, but removed to jdk.unsupported. We can still compile this with JDK11. There is also an issue to track this: https://github.com/apache/incubator-paimon/issues/692 . To enable the compiler error in IDEA, plz make the option un-tick.
<img width="982" alt="image" src="https://user-images.githubusercontent.com/2370761/227825976-7abf4933-d859-4ccf-abcd-646552ffe263.png">
Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-paimon] legendtkl commented on pull request #714: [core] support JDK 11
Posted by "legendtkl (via GitHub)" <gi...@apache.org>.
legendtkl commented on PR #714:
URL: https://github.com/apache/incubator-paimon/pull/714#issuecomment-1487147374
> Thanks @legendtkl for the contribution! I am very pleased to be able to officially announce support for JDK11! But one thing is that still JDK8 is currently the main force, as mentioned by the partners of the review above.
>
> Another thing is that I don't really want to cause too many tests, is it possible to provide the ability to test with JDK11, but by default it needs to be enabled manually?
Hi, @JingsongLi , I have add the github action based PR's comment and update the workflows.
The JDK 11 workflows will not run by default. When we comment '/jdk11' in the PR, the workflow will start to run. ***But this feature will work only when the workflows files is on the default branch, namely master.*** That means, we can't verify this in this PR, and it will work only when this PR is merged.
I have tested it manually on my branch. When I comment '/jdk11', the jdk11 workflows start running, we can see it in the `Action` tab.
<img width="438" alt="image" src="https://user-images.githubusercontent.com/2370761/228291415-18d09fc4-9293-42c6-b9b1-03427818181a.png">
<img width="1822" alt="image" src="https://user-images.githubusercontent.com/2370761/228291225-de784154-4191-4d73-acca-55ce57f96773.png">
Ref: https://github.com/orgs/community/discussions/25389
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-paimon] JingsongLi commented on pull request #714: [core] support JDK 11
Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.
JingsongLi commented on PR #714:
URL: https://github.com/apache/incubator-paimon/pull/714#issuecomment-1486111157
Thanks @legendtkl for the contribution!
I am very pleased to be able to officially announce support for JDK11!
But one thing is that still JDK8 is currently the main force, as mentioned by the partners of the review above.
Another thing is that I don't really want to cause too many tests, is it possible to provide the ability to test with JDK11, but by default it needs to be enabled manually?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-paimon] legendtkl commented on a diff in pull request #714: [core] support JDK 11
Posted by "legendtkl (via GitHub)" <gi...@apache.org>.
legendtkl commented on code in PR #714:
URL: https://github.com/apache/incubator-paimon/pull/714#discussion_r1148746972
##########
paimon-common/src/main/java/org/apache/paimon/io/cache/CachedRandomInputView.java:
##########
@@ -55,7 +55,7 @@ public CachedRandomInputView(File file, CacheManager cacheManager)
this.file = new RandomAccessFile(file, "r");
this.fileLength = file.length();
this.cacheManager = cacheManager;
- this.segments = new HashMap<>();
+ this.segments = new ConcurrentHashMap<>();
Review Comment:
@FangYongs This is to fix ConcurrentModificationException threw by JDK11.
Generally speaking, it's part of JDK11 supported work.
> What's this change for? Maybe you should create an another issue and add test case for this change
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-paimon] legendtkl commented on a diff in pull request #714: [WIP][core] support JDK 11
Posted by "legendtkl (via GitHub)" <gi...@apache.org>.
legendtkl commented on code in PR #714:
URL: https://github.com/apache/incubator-paimon/pull/714#discussion_r1148268278
##########
.github/workflows/e2e-tests-1.14.yml:
##########
@@ -32,10 +32,11 @@ jobs:
steps:
- name: Checkout code
uses: actions/checkout@v2
- - name: Set up JDK 1.8
- uses: actions/setup-java@v1
+ - name: Set up JDK 11
+ uses: actions/setup-java@v2
with:
- java-version: 1.8
+ java-version: '11'
Review Comment:
@TyrantLucifer OK, let's keep them both temporarily.
But from a long-term view, supporting JDK11 without deprecating JDK8 is not a real upgrading. We have to do much meaningless work for the compatibility, and give up the JDK11 specified programing feature.
I will open a new ISSUE to discuss the JDK8 Deprecation after this PR is merged.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-paimon] legendtkl commented on a diff in pull request #714: [core] support JDK 11
Posted by "legendtkl (via GitHub)" <gi...@apache.org>.
legendtkl commented on code in PR #714:
URL: https://github.com/apache/incubator-paimon/pull/714#discussion_r1153125664
##########
paimon-common/src/main/java/org/apache/paimon/io/cache/CachedRandomInputView.java:
##########
@@ -55,7 +55,7 @@ public CachedRandomInputView(File file, CacheManager cacheManager)
this.file = new RandomAccessFile(file, "r");
this.fileLength = file.length();
this.cacheManager = cacheManager;
- this.segments = new HashMap<>();
+ this.segments = new ConcurrentHashMap<>();
Review Comment:
@SteNicholas yes, once we replace computeIfAbsent log, it's ok to use HashMap.
Let's keep the hashmap left.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-paimon] legendtkl closed pull request #714: [core] support JDK 11
Posted by "legendtkl (via GitHub)" <gi...@apache.org>.
legendtkl closed pull request #714: [core] support JDK 11
URL: https://github.com/apache/incubator-paimon/pull/714
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-paimon] SteNicholas commented on a diff in pull request #714: [core] support JDK 11
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on code in PR #714:
URL: https://github.com/apache/incubator-paimon/pull/714#discussion_r1148789529
##########
paimon-common/src/main/java/org/apache/paimon/io/cache/CachedRandomInputView.java:
##########
@@ -55,7 +55,7 @@ public CachedRandomInputView(File file, CacheManager cacheManager)
this.file = new RandomAccessFile(file, "r");
this.fileLength = file.length();
this.cacheManager = cacheManager;
- this.segments = new HashMap<>();
+ this.segments = new ConcurrentHashMap<>();
Review Comment:
@legendtkl, adds the test case to test this fix for `CachedRandomInputView` in JDK11?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-paimon] legendtkl commented on a diff in pull request #714: [core] support JDK 11
Posted by "legendtkl (via GitHub)" <gi...@apache.org>.
legendtkl commented on code in PR #714:
URL: https://github.com/apache/incubator-paimon/pull/714#discussion_r1151320889
##########
paimon-common/src/main/java/org/apache/paimon/io/cache/CachedRandomInputView.java:
##########
@@ -50,12 +52,24 @@ public class CachedRandomInputView extends AbstractPagedInputView
private int currentSegmentIndex;
+ /**
+ * The thread pool is involved to solve computeIfAbsent issue. From JDK 9, computeIfAbsent will
+ * throw ConcurrentModificationException if it is detected that the mapping function modifies
+ * this map during computation. So we add the thread pool to remove segments expired page. To
+ * guarantee the concurrency safety, we need to replace the HashMap with ConcurrentHashMap.
+ *
+ * <p>Note: if the cache expires many pages in a while, the single thread executor can't process
+ * it. That means, some remove actions are blocked, this will cause dirty data read. But this
+ * occurs with low probability.
+ */
+ ExecutorService executor = Executors.newSingleThreadExecutor();
Review Comment:
ok.
Actually, this is the first edition of this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-paimon] SteNicholas commented on pull request #714: [core] support JDK 11
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on PR #714:
URL: https://github.com/apache/incubator-paimon/pull/714#issuecomment-1484361880
@legendtkl, `sun.misc.Unsafe` is deprecated since JDK11. You need to take a look at this class.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-paimon] JingsongLi commented on a diff in pull request #714: [core] support JDK 11
Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.
JingsongLi commented on code in PR #714:
URL: https://github.com/apache/incubator-paimon/pull/714#discussion_r1151307886
##########
.github/workflows/e2e-tests-1.15.yml:
##########
@@ -32,10 +32,11 @@ jobs:
steps:
- name: Checkout code
uses: actions/checkout@v2
- - name: Set up JDK 1.8
- uses: actions/setup-java@v1
+ - name: Set up JDK 8
Review Comment:
Can we just have a env here? if (is jdk 11) Set up JDK 11 else Set up JDK 8?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-paimon] legendtkl commented on pull request #714: [core] support JDK 11
Posted by "legendtkl (via GitHub)" <gi...@apache.org>.
legendtkl commented on PR #714:
URL: https://github.com/apache/incubator-paimon/pull/714#issuecomment-1491243836
> @legendtkl, as the description mentioned, is the changes of `CachedRandomInputView` related to the support of JDK 11? BTW, where is the change for `LocalDateTime.now()`?
@SteNicholas
Two questions' answer:
1. yes. JDK11 will do more check in computeIfAbsent, now we replace it
2. you can refer paimon-core/src/main/java/org/apache/paimon/io/DataFileMeta.java . The main reason is that serialization and deserialization have limit the precesion to TimeStamp(3). (Serialization specify this by schema)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-paimon] TyrantLucifer commented on a diff in pull request #714: [core] support JDK 11
Posted by "TyrantLucifer (via GitHub)" <gi...@apache.org>.
TyrantLucifer commented on code in PR #714:
URL: https://github.com/apache/incubator-paimon/pull/714#discussion_r1147874984
##########
.github/workflows/e2e-tests-1.14.yml:
##########
@@ -32,10 +32,11 @@ jobs:
steps:
- name: Checkout code
uses: actions/checkout@v2
- - name: Set up JDK 1.8
- uses: actions/setup-java@v1
+ - name: Set up JDK 11
+ uses: actions/setup-java@v2
with:
- java-version: 1.8
+ java-version: '11'
Review Comment:
Hi @legendtkl , I have a question, if you change it from 8 to 11, it will not execute jdk 8 test, I think maybe you should use `strategy.metrix` to specify both versions
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-paimon] legendtkl closed pull request #714: [WIP][core] support JDK 11
Posted by "legendtkl (via GitHub)" <gi...@apache.org>.
legendtkl closed pull request #714: [WIP][core] support JDK 11
URL: https://github.com/apache/incubator-paimon/pull/714
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-paimon] FangYongs commented on a diff in pull request #714: [core] support JDK 11
Posted by "FangYongs (via GitHub)" <gi...@apache.org>.
FangYongs commented on code in PR #714:
URL: https://github.com/apache/incubator-paimon/pull/714#discussion_r1148737938
##########
paimon-common/src/main/java/org/apache/paimon/io/cache/CachedRandomInputView.java:
##########
@@ -55,7 +55,7 @@ public CachedRandomInputView(File file, CacheManager cacheManager)
this.file = new RandomAccessFile(file, "r");
this.fileLength = file.length();
this.cacheManager = cacheManager;
- this.segments = new HashMap<>();
+ this.segments = new ConcurrentHashMap<>();
Review Comment:
What's this change for? Maybe you should create an another issue and add test case for this change
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-paimon] JingsongLi commented on a diff in pull request #714: [core] support JDK 11
Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.
JingsongLi commented on code in PR #714:
URL: https://github.com/apache/incubator-paimon/pull/714#discussion_r1151306796
##########
paimon-common/src/main/java/org/apache/paimon/io/cache/CachedRandomInputView.java:
##########
@@ -50,12 +52,24 @@ public class CachedRandomInputView extends AbstractPagedInputView
private int currentSegmentIndex;
+ /**
+ * The thread pool is involved to solve computeIfAbsent issue. From JDK 9, computeIfAbsent will
+ * throw ConcurrentModificationException if it is detected that the mapping function modifies
+ * this map during computation. So we add the thread pool to remove segments expired page. To
+ * guarantee the concurrency safety, we need to replace the HashMap with ConcurrentHashMap.
+ *
+ * <p>Note: if the cache expires many pages in a while, the single thread executor can't process
+ * it. That means, some remove actions are blocked, this will cause dirty data read. But this
+ * occurs with low probability.
+ */
+ ExecutorService executor = Executors.newSingleThreadExecutor();
Review Comment:
We don't need to introduce this complex codes.
How about just modify `getCurrentPage`.
```
private MemorySegment getCurrentPage() {
MemorySegment segment = segments.get(currentSegmentIndex);
if (segment == null) {
segment = cacheManager.getPage(file, currentSegmentIndex, this::invalidPage);
segments.put(currentSegmentIndex, segment);
}
return segment;
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-paimon] JingsongLi commented on pull request #714: [core] support JDK 11
Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.
JingsongLi commented on PR #714:
URL: https://github.com/apache/incubator-paimon/pull/714#issuecomment-1487827668
@legendtkl Thanks, left comments.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org