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

Ignore Nashorn's class loader for performance reasons #7116

Merged
merged 1 commit into from
Nov 11, 2022

Conversation

dboreham
Copy link
Contributor

@dboreham dboreham commented Nov 9, 2022

See this discussion thread for background: #6985

@dboreham dboreham requested a review from a team November 9, 2022 04:39
Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

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

thx @dboreham!

@laurit had an additional comment that would be nice to include if you are able #6985 (reply in thread):

If we are going to exclude it we should also exclude the same class loader from older jdks that bundle nashorn, it should have a different package name there.

@dboreham
Copy link
Contributor Author

dboreham commented Nov 9, 2022

thx @dboreham!

@laurit had an additional comment that would be nice to include if you are able #6985 (reply in thread):

If we are going to exclude it we should also exclude the same class loader from older jdks that bundle nashorn, it should have a different package name there.

Oh, I spaced that out. I will grab the stack trace from an older JDK to find the class name.

@dboreham
Copy link
Contributor Author

thx @dboreham!
@laurit had an additional comment that would be nice to include if you are able #6985 (reply in thread):

If we are going to exclude it we should also exclude the same class loader from older jdks that bundle nashorn, it should have a different package name there.

Oh, I spaced that out. I will grab the stack trace from an older JDK to find the class name.

It turns out the fix is not required for JDK8: #6985 (comment)

@trask trask merged commit c009c79 into open-telemetry:main Nov 11, 2022
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.

3 participants