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

Upgrade guava version to support higher JDK version #3541

Merged
merged 7 commits into from
Sep 30, 2019
Merged

Upgrade guava version to support higher JDK version #3541

merged 7 commits into from
Sep 30, 2019

Conversation

kezhenxu94
Copy link
Member

Please answer these questions before submitting pull request

  • Why submit this pull request?

  • Bug fix

  • New feature provided

  • Improve performance

  • Related issues

#3540 and #3477

ClassPath classpath = ClassPath.from(this.getClass().getClassLoader());
ImmutableSet<ClassPath.ClassInfo> classes = classpath.getTopLevelClassesRecursive("org.apache.skywalking");
for (ClassPath.ClassInfo classInfo : classes) {

the classpath.getTopLevelClassesRecursive("org.apache.skywalking") returns empty set under JDK 8+, and it turns out to be an issue of Guava, see google/guava#3249 (comment) , this patch upgrades the Guava version to 23.1, and I've verified locally the exception disappear and OAP starts successfully

@kezhenxu94 kezhenxu94 added this to the 6.5.0 milestone Sep 28, 2019
@kezhenxu94 kezhenxu94 mentioned this pull request Sep 28, 2019
4 tasks
Copy link
Member

@wu-sheng wu-sheng left a comment

Choose a reason for hiding this comment

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

Are you sure this could make oap running 9 and 9+?

And license needs to be upgraded. Why CI check didn't find version change?

@wu-sheng
Copy link
Member

Maybe CI found the issue.

@kezhenxu94
Copy link
Member Author

I've replayed the e2e tests under JDK 1.9 manually (which won't report check status back to this PR), let's wait and see the result, if it passes, I think it should generally work on 1.9 and above, https://builds.apache.org/job/skywalking-e2e-2/763/console

@wu-sheng wu-sheng added feature New feature backend OAP backend related. labels Sep 28, 2019
@wu-sheng
Copy link
Member

If you see it passed, reply me here. I am using phone only, today.

@kezhenxu94
Copy link
Member Author

@wu-sheng it passes under JDK 9, see the selected/highlighted words below the screenshot
image

@wu-sheng
Copy link
Member

Cool. How about higher? 11 for example? I am thinking about we need JRE documentation update.

@kezhenxu94
Copy link
Member Author

Cool. How about higher? 11 for example? I am thinking about we need JRE documentation update.

I tested this afternoon locally with OpenJDK 12, it works fine too, so it's highly possible that it also works on JDK 10 and JDK11, do we need to replay this on the infra Jenkins?

@wu-sheng
Copy link
Member

The infra is setting up our VM. Please submit an issue to track the higher jdk tests.

In this PR, at least, we should update document.

@kezhenxu94
Copy link
Member Author

Oops, I've made a mistake that the container in which our e2e runs is based on OpenJDK8, the JDK version in Jenkinsfile-E2E is just for compile time only, so we'll need some real tests

@wu-sheng
Copy link
Member

OK, so we haven't verified in 9+?

@kezhenxu94
Copy link
Member Author

OK, so we haven't verified in 9+?

I verified JDK12 locally, others are not verified yet

@kezhenxu94
Copy link
Member Author

We may need much more resources if we want to verify every commit on those versions

@wu-sheng
Copy link
Member

OK. I think 9 and 11 are LTS versions, right? We could focus on that.

@wu-sheng
Copy link
Member

/run ci

@wu-sheng wu-sheng merged commit 179914d into apache:master Sep 30, 2019
@kezhenxu94 kezhenxu94 deleted the fix/3540 branch September 30, 2019 05:32
@kezhenxu94 kezhenxu94 mentioned this pull request Oct 7, 2019
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend OAP backend related. feature New feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants