-
Notifications
You must be signed in to change notification settings - Fork 4.7k
HIVE-28856: Remove Jetty-Runner #5720
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
Conversation
packaging/src/license/licenses.xml
Outdated
<dependency> | ||
<groupId>org.apache.tomcat</groupId> | ||
<artifactId>tomcat-servlet-api</artifactId> | ||
<version>9.0.82</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's better to create a variable for versions so that we can reuse them elsewhere. You can probably add
<tomcat.version>9.0.82</tomcat.version>
in the root pom's properties.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added variable
packaging/src/license/licenses.xml
Outdated
<dependency> | ||
<groupId>org.eclipse.jdt</groupId> | ||
<artifactId>ecj</artifactId> | ||
<version>3.26.0</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Create a property variable for this too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added variable
service/pom.xml
Outdated
</exclusions> | ||
<groupId>org.apache.tomcat</groupId> | ||
<artifactId>tomcat-jasper</artifactId> | ||
<version>9.0.82</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace this with {tomcat.version}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
replaced
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the "How was this patch tested?" section, can you please include the tests that were run on your local machine?
It is easier for reviewers who are not familiar with the specifics to verify the tests if the steps are listed.
pom.xml
Outdated
<groupId>org.eclipse.jetty</groupId> | ||
<artifactId>jetty-runner</artifactId> | ||
<version>${jetty.version}</version> | ||
<groupId>org.apache.tomcat</groupId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we should be using apache-jsp
instead. that would simplify the upgrades of jetty dependencies
<groupId>org.eclipse.jetty</groupId>
<artifactId>apache-jsp</artifactId>
<version>${jetty.version}</version>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I thought of it but as per apache-jsp from org.eclipse.jetty has test dependencies with 3 cve for the current jetty version so i thought of not taking risk of adding it although it is under test scope and also as apache-jsp from org.mortbay.jasper was not alone enough to prevent compilation failures so i also dropped for apache-jsp from org.eclipse.jetty
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also on using apache-jsp it is giving ant build error on service pom due to absance of org.apache.jasper.JspC so that's why only apache-jsp won't be enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ramitg254, try
<jetty.version>9.4.57.v20241219</jetty.version>
<dependency>
<groupId>org.eclipse.jetty</groupId>
<artifactId>apache-jsp</artifactId>
<version>${jetty.version}</version>
</dependency>
that removes a number of CVEs from org.eclipse.jetty
deps:
Direct vulnerabilities:
[CVE-2024-8184](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2024-8184)
[CVE-2023-26049](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2023-26049)
[CVE-2023-26048](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2023-26048)
Vulnerabilities from dependencies:
[CVE-2023-40167](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2023-40167)
[CVE-2022-2047](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2022-2047)
ref: #5599 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mvn clean install -DskipTests -T4 -Pdist,itests -Dmaven.javadoc.skip=true -Drat.skip=true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
made the changes
I have upgraded to 9.0.98 of tomcat-jasper because from version 10 and onwards there was migration from javax.servlet.* to jakarta.servlet.* and we are still using javax.servlet.* so it won't build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, pending tests
@ramitg254, have you tried to start HS2 in Docker and check the WebUI? |
Yeah, I am able to start hs2 in docker and see the webui of it |
|
What changes were proposed in this pull request?
Removal of jetty-runner dependency
Why are the changes needed?
Not jetty-runner as a whole was needed only tomcat-jasper was required
Does this PR introduce any user-facing change?
No
Is the change a dependency upgrade?
No
How was this patch tested?
on local machine, I tested it for successful mvn build and testing it with hive-precommit-tests .