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

Fix tomcat 9.0.52 #3789

Merged
merged 1 commit into from
Aug 7, 2021
Merged

Fix tomcat 9.0.52 #3789

merged 1 commit into from
Aug 7, 2021

Conversation

laurit
Copy link
Contributor

@laurit laurit commented Aug 6, 2021

In tomcat 9.0.52 async servlet tests don't pass without instrumenting org.apache.tomcat.util.threads.ThreadPoolExecutor

Comment on lines +23 to +24
testLibrary("org.apache.tomcat.embed:tomcat-embed-core:8.0.41")
testLibrary("org.apache.tomcat.embed:tomcat-embed-jasper:8.0.41")
Copy link
Member

@trask trask Aug 7, 2021

Choose a reason for hiding this comment

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

is this part of the comment still relevant?

Suggested change
testLibrary("org.apache.tomcat.embed:tomcat-embed-core:8.0.41")
testLibrary("org.apache.tomcat.embed:tomcat-embed-jasper:8.0.41")
// Tests need at least version 8.0.41 to have necessary classes to configure the embedded tomcat...
// ... but not newer that version 10, because its servlet 5.
testLibrary("org.apache.tomcat.embed:tomcat-embed-core:8.0.41")
testLibrary("org.apache.tomcat.embed:tomcat-embed-jasper:8.0.41")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my opinion this comment isn't relevant. It was previously using 9+ for both regular build and latest deps which is bad because it is really confusing when regular build starts failing without any code changes. 8.0.41 is the version used in servlet-3.0 tests. I didn't try to figure out whether this is really the earliest version that works, only checked that 8.0.1 didn't work. The part of version 10 is kind of obvious, servlet-3.0 uses the same construct and doesn't have a comment. Jasper dependency was added to get rid of a non fatal stack trace in test output.

@trask trask mentioned this pull request Aug 7, 2021
@trask trask merged commit 9df0686 into open-telemetry:main Aug 7, 2021
@trask trask mentioned this pull request Aug 7, 2021
@trask
Copy link
Member

trask commented Aug 8, 2021

btw thanks for finding/fixing, this was quite confusing failure 👍

@laurit laurit deleted the tomcat9052 branch August 24, 2021 07:46
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.

2 participants