Skip to content

Conversation

@sarutak
Copy link
Member

@sarutak sarutak commented Nov 18, 2025

What changes were proposed in this pull request?

This PR proposes to upgrade Jetty to 12.1.4, Jersey to 3.1.11 and Servlet to 6.0 which need to be upgraded together.
Because Jetty 12 is significantly changed from 11.x, this PR mainly considers following things to upgrade.

Why are the changes needed?

Jetty 11 is already EOF and starting Jan 1, 2026, no more release will be published.
jetty/jetty.project#13918

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Existing tests with some tweaks.

Was this patch authored or co-authored using generative AI tooling?

No.

override def setStatus(status: Int): Unit = this.status = status

override def setStatus(sc: Int, sm: String): Unit = {}

Copy link
Member Author

Choose a reason for hiding this comment

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

if (queryString == null) {
return null;
}
Map<String, String[]> params = jakarta.servlet.http.HttpUtils.parseQueryString( queryString );
Copy link
Member Author

Choose a reason for hiding this comment

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

HttpUtils was deprecated in Servlet 5.0 and removed in 6.0.
https://jakarta.ee/specifications/servlet/5.0/apidocs/jakarta/servlet/http/httputils

Copy link
Member

Choose a reason for hiding this comment

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

Thank you, @sarutak . To minimize the PR, could you spin off this HttpUtils-related change PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

@dongjoon-hyun
You mean, merge this part first in another PR and then merge this PR right?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I mean handling HttpUtils-deprecation first in the existing dependencies of master branch. This will help eventually this PR by reducing the complexity.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I will open another one.

dongjoon-hyun pushed a commit that referenced this pull request Nov 18, 2025
…pServlet.java` with `HttpServletRequest.getQueryString`

### What changes were proposed in this pull request?
This PR aims to replace `HtmlUtils.parseQueryString` in	`ThriftHttpServlet.java` with `HttpServletRequest.getQueryString`

### Why are the changes needed?
`HttpUtils` was deprecated in Servlet 5.0 and removed in 6.0.
https://jakarta.ee/specifications/servlet/5.0/apidocs/jakarta/servlet/http/httputils

This change is necessary for upgrading Jetty to 12 in #53116

### Does this PR introduce _any_ user-facing change?
No.

### How was this patch tested?
GA.

### Was this patch authored or co-authored using generative AI tooling?
No.

Closes #53119 from sarutak/replace-http-utils.

Authored-by: Kousuke Saruta <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
}

test("SPARK-45522: Jetty 10 and above shouuld return status code 302 with correct redirect url" +
test("SPARK-47086: Jetty 12 and above should return status code 301 with correct redirect url" +
Copy link
Member

Choose a reason for hiding this comment

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

Oh, is this changed from 302 (Temporary Redirect) to 301 (Permanent Redirect)?

Copy link
Member Author

Choose a reason for hiding this comment

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

jersey-container-servlet/3.0.18//jersey-container-servlet-3.0.18.jar
jersey-hk2/3.0.18//jersey-hk2-3.0.18.jar
jersey-server/3.0.18//jersey-server-3.0.18.jar
jersey-client/3.1.11//jersey-client-3.1.11.jar
Copy link
Member

Choose a reason for hiding this comment

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

Could you mention jersey version in the PR title because this is also important, @sarutak ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure.

@sarutak sarutak changed the title [SPARK-47086][SPARK-48022][BUILD][CORE][WEBUI] Upgrade Jetty to 12.1.4 [SPARK-47086][SPARK-48022][BUILD][CORE][WEBUI] Upgrade Jetty to 12.1.4 and Jersey to 3.1.11 Nov 19, 2025
@sarutak sarutak changed the title [SPARK-47086][SPARK-48022][BUILD][CORE][WEBUI] Upgrade Jetty to 12.1.4 and Jersey to 3.1.11 [SPARK-47086][SPARK-48022][BUILD][CORE][WEBUI] Upgrade Jetty to 12.1.4, Jersey to 3.1.11 and Servlet to 6.0 Nov 19, 2025
@LuciferYang
Copy link
Contributor

As far as I recall, when @HiuKwok previously attempted to migrate to Jetty 12, a problem with the sendRedirect capability was identified, and the records from that time are as follows:

Has this issue been resolved now?

@pan3793
Copy link
Member

pan3793 commented Nov 19, 2025

Jetty 11 is already EOF and starting Jan 1, 2026, no more release will be published.
jetty/jetty.project#13918

It sounds like a shocking news, @dongjoon-hyun, I understand 4.1 has been feature frozen, do you think we can have an exception for this one, otherwise, that means Spark 4.1 has no workaround during its 18 months lifecycle if Jetty 11 exposes new CVEs.

@sarutak
Copy link
Member Author

sarutak commented Nov 19, 2025

@LuciferYang

Has this issue been resolved now?

Yes. Otherwise, this test should fail because of the following reason:

  1. Access to https://proxy.example.com:443/prefix/ctx1 http://$localhost:${serverInfo.boundPort}/ctx1 where https://proxy.example.com:443/prefix/ is a proxy root and ctx1 is a context path root.
  2. Jetty returns status code 301 without rewriting location header. So the location header will be /ctx/ while https://proxy.example.com:443/prefix/ctx/ is expected where https://proxy.example.com:443/prefix is a proxy root.

Comment on lines +619 to +623
val redirectServer = if (server.contains(":") && !server.startsWith("[")) {
s"[$server]"
} else {
server
}
Copy link
Member

Choose a reason for hiding this comment

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

this can be simplified by using Utils.addBracketsIfNeeded(server)

Copy link
Member Author

Choose a reason for hiding this comment

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

This part is just brought from the original code. Unless we need to refactor this part within this PR, I'd like to complete upgrading first and then open a followup PR.

Copy link
Member

Choose a reason for hiding this comment

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

was not aware of that, it's fine then.

when(clientRequest.getScheme()).thenReturn("http")
when(clientRequest.getHeader("host")).thenReturn(s"$localhost:8080")
when(clientRequest.getPathInfo()).thenReturn("/proxy/worker-id/jobs")
when(clientRequest.getPathInfo()).thenReturn("/worker-id/jobs")
Copy link
Member

Choose a reason for hiding this comment

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

Why is this change needed?

Copy link
Member Author

@sarutak sarutak Nov 19, 2025

Choose a reason for hiding this comment

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

Good question. This change is relevant to this part.

/proxy is a context root so the returned value from HttpServletRequest#getPathInfo should not contain /proxy. In old versions of Jetty, the returned value can strangely contain a context root.
As of Jetty 12, Jetty specific Core API and the standard Servlet API are decoupled and getPathInfo doesn't return a path with a context root (/proxy in this case).

Copy link
Member

Choose a reason for hiding this comment

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

thanks for the explanation, the Jetty 12 behavior sounds more reasonable, do you think it is worth leaving some comment (at def createProxyLocationHeader, not here) to briefly explain such behavior changes?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm OK to leave a comment. I'll do it and your another suggestion together in a followup PR.

Copy link
Member

@pan3793 pan3793 left a comment

Choose a reason for hiding this comment

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

Have manually verified with YARN integration, proxy works as expected.

$ spark-sql
WARNING: Using incubator modules: jdk.incubator.vector
Setting default log level to "WARN".
To adjust logging level use sc.setLogLevel(newLevel). For SparkR, use setLogLevel(newLevel).
2025-11-20 06:50:36 WARN Client: Neither spark.yarn.jars nor spark.yarn.archive is set, falling back to uploading libraries under SPARK_HOME.
Spark Web UI available at http://hadoop-master1.orb.local:4040
Spark master: yarn, Application Id: application_1763621405849_0001
spark-sql (default)>

Access http://hadoop-master1.orb.local:4040 and it is correctly redirected to the YARN RM address.

Image

@sarutak
Copy link
Member Author

sarutak commented Nov 20, 2025

@pan3793 Thank you for verifying!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants