FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Support forkjoinpool plugin in JDK11 by 786991884 · Pull Request #656 · apache/skywalking-java · GitHub

/ skywalking-java Public

Support forkjoinpool plugin in JDK11 - #656

Merged
wu-sheng merged 6 commits into
apache:mainfrom
786991884:main
Dec 18, 2023
ConversationCommits (6)ChecksFiles changed
Merged

Support forkjoinpool plugin in JDK11#656
wu-sheng merged 6 commits into
apache:mainfrom
786991884:main

Conversation

786991884 commented Dec 7, 2023

Copy link
Copy Markdown
Contributor

wu-sheng commented Dec 7, 2023

Copy link
Copy Markdown
Member

You need to add the test scenario to run in JDK 11, otherwise, this is only a manual test

wu-sheng requested review from wu-sheng and xu1009 December 7, 2023 15:23
wu-sheng added enhancement New feature or request plugin labels Dec 7, 2023

wu-sheng commented Dec 7, 2023

Copy link
Copy Markdown
Member

Also, you removed the pull request template, we need that, and you missed the update in the change log.

wu-sheng commented Dec 9, 2023

Copy link
Copy Markdown
Member

You need to add the test scenario to run in JDK 11, otherwise, this is only a manual test

This is still missed.

wu-sheng commented Dec 11, 2023

Copy link
Copy Markdown
Member

Why do you open a new test3 with so many cases?

wu-sheng reviewed Dec 11, 2023
View reviewed changes
Comment thread .github/workflows/plugins-jdk11-test.3.yaml
strategy:
matrix:
case:
- jdk11-forkjoinpool-scenario

wu-sheng Dec 11, 2023

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Do we need a new case? I think this is rt.jar(jdk level) change, even the codes are compiled as 1.8, the runtime method is still going to be forced to run in 11, right?

wu-sheng Dec 11, 2023

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Could we not copy the test scenario, but directly write the existing test in jdk11. Was that failing before and running now?

786991884 Dec 12, 2023

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

yes, you are right. I will optimize it

wu-sheng reviewed Dec 11, 2023
View reviewed changes
Comment thread ...apache/skywalking/apm/plugin/jdk/forkjoinpool/define/ForkJoinWorkerQueueInstrumentation.java

private static final String FORK_JOIN_WORKER_QUEUE_RUN_TASK_METHOD = "runTask";

private static final String FORK_JOIN_WORKER_QUEUE_RUN_TASK_METHOD_JDK11 = "topLevelExec";

wu-sheng Dec 11, 2023

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Please add comments about why there are two methods to intercept.

wu-sheng commented Dec 11, 2023

Copy link
Copy Markdown
Member

From test logs, your interceptor doesn't run as expected. Still, the old method existed and being intercepted.

wu-sheng commented Dec 12, 2023

Copy link
Copy Markdown
Member

Tests passed, could you check this? #656 (comment)

wu-sheng commented Dec 13, 2023

Copy link
Copy Markdown
Member

Is there any update here?

wu-sheng commented Dec 15, 2023

Copy link
Copy Markdown
Member

@786991884 Do you have any update?

wu-sheng added the stale label Dec 18, 2023
wu-sheng removed the stale label Dec 18, 2023
wu-sheng added this to the 9.2.0 milestone Dec 18, 2023

wu-sheng commented Dec 18, 2023

Copy link
Copy Markdown
Member

@786991884 If you will be back, I hope this extra test can be removed by reusing the existing one.

I am going to merge this first because from what I saw, this fix is important.

wu-sheng merged commit c5d62cb into apache:main Dec 18, 2023
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

wu-sheng wu-sheng approved these changes
xu1009 Awaiting requested review from xu1009

Assignees

No one assigned

Labels

enhancement New feature or request plugin

Projects

None yet

Milestone

9.2.0

Development

Successfully merging this pull request may close these issues.

[Bug] api-jdk-forkjoinpool-plugin not effective in jdk11

2 participants

Back | FazBrowse Home | New Git URL