You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "HiuKwok (via GitHub)" <gi...@apache.org> on 2024/03/13 13:55:23 UTC
[PR] [SPARK-47086][BUILD][CORE][SQL][UI] Migrate from Jetty 11 to Jetty 12 [spark]
HiuKwok opened a new pull request, #45500:
URL: https://github.com/apache/spark/pull/45500
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'common/utils/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
This is a draft MR to upgrade Jetty deps from 11 to 12.
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Spark versions or within the unreleased branches such as master.
If no, write 'No'.
-->
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
If benchmark tests were added, please run the benchmarks in GitHub Actions for the consistent environment, and the instructions could accord to: https://spark.apache.org/developer-tools.html#github-workflow-benchmarks.
-->
### Was this patch authored or co-authored using generative AI tooling?
<!--
If generative AI tooling has been used in the process of authoring this patch, please include the
phrase: 'Generated-by: ' followed by the name of the tool and its version.
If no, write 'No'.
Please refer to the [ASF Generative Tooling Guidance](https://www.apache.org/legal/generative-tooling.html) for details.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47086][BUILD][CORE][SQL][UI] Migrate from Jetty 11 to Jetty 12 [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #45500:
URL: https://github.com/apache/spark/pull/45500#issuecomment-2011251140
Thank you for all of your contributions here, @HiuKwok !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47086][BUILD][CORE][SQL][UI] Migrate from Jetty 11 to Jetty 12 [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #45500:
URL: https://github.com/apache/spark/pull/45500#discussion_r1523607793
##########
core/src/main/scala/org/apache/spark/ui/JettyUtils.scala:
##########
@@ -588,36 +590,37 @@ private[spark] case class ServerInfo(
* a servlet context without the trailing slash (e.g. "/jobs") - Jetty will send a redirect to the
* same URL, but with a trailing slash.
*/
-private class ProxyRedirectHandler(_proxyUri: String) extends HandlerWrapper {
+private class ProxyRedirectHandler(_proxyUri: String) extends Handler.Wrapper {
private val proxyUri = _proxyUri.stripSuffix("/")
override def handle(
- target: String,
- baseRequest: Request,
- request: HttpServletRequest,
- response: HttpServletResponse): Unit = {
- super.handle(target, baseRequest, request, new ResponseWrapper(request, response))
- }
-
- private class ResponseWrapper(
- req: HttpServletRequest,
- res: HttpServletResponse)
- extends HttpServletResponseWrapper(res) {
-
- override def sendRedirect(location: String): Unit = {
- val newTarget = if (location != null) {
- val target = new URI(location)
- // The target path should already be encoded, so don't re-encode it, just the
- // proxy address part.
- val proxyBase = UIUtils.uiRoot(req)
- val proxyPrefix = if (proxyBase.nonEmpty) s"$proxyUri$proxyBase" else proxyUri
- s"${res.encodeURL(proxyPrefix)}${target.getPath()}"
- } else {
- null
- }
- super.sendRedirect(newTarget)
- }
+ request: Request,
+ response: org.eclipse.jetty.server.Response,
+ callback: Callback): Boolean = {
+ // Todo: Fix the proxy redirect behaviour.
+// super.handle(request, new ResponseWrapper(request, response), callback)
+ super.handle(request, response, callback)
}
+//
+// private class ResponseWrapper(
+// req: Request,
+// res: Response)
+// extends Response.Wrapper(req, res) {
+//
+// override def sendRedirect(location: String): Unit = {
+// val newTarget = if (location != null) {
+// val target = new URI(location)
+// // The target path should already be encoded, so don't re-encode it, just the
+// // proxy address part.
+// val proxyBase = UIUtils.uiRoot(req)
+// val proxyPrefix = if (proxyBase.nonEmpty) s"$proxyUri$proxyBase" else proxyUri
+// s"${res.encodeURL(proxyPrefix)}${target.getPath()}"
+// } else {
+// null
+// }
+// super.sendRedirect(newTarget)
+// }
+// }
Review Comment:
Please remove this leftover.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47086][BUILD][CORE][SQL][UI] Migrate from Jetty 11 to Jetty 12 [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #45500:
URL: https://github.com/apache/spark/pull/45500#discussion_r1523611719
##########
core/src/test/scala/org/apache/spark/ui/UISuite.scala:
##########
@@ -398,8 +386,8 @@ class UISuite extends SparkFunSuite {
}
}
- test("SPARK-45522: Jetty 10 and above shouuld return status code 302 with correct redirect url" +
- " when request URL ends with a context path without trailing '/'") {
+ test("SPARK-34449: Jetty 9.4.35.v20201120 and later no longer return status code 302 " +
+ " and handle internally when request URL ends with a context path without trailing '/'") {
Review Comment:
I'm not sure why do we change this in this PR. Could you spin-off this in order to merge before this PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47086][BUILD][CORE][SQL][UI] Migrate from Jetty 11 to Jetty 12 [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #45500:
URL: https://github.com/apache/spark/pull/45500#discussion_r1523608885
##########
core/src/main/scala/org/apache/spark/util/Utils.scala:
##########
@@ -2198,8 +2197,6 @@ private[spark] object Utils
return true
}
isBindCollision(e.getCause)
- case e: MultiException =>
- e.getThrowables.asScala.exists(isBindCollision)
Review Comment:
This looks like a nice removal.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47086][BUILD][CORE][SQL][UI] Migrate from Jetty 11 to Jetty 12 [spark]
Posted by "HiuKwok (via GitHub)" <gi...@apache.org>.
HiuKwok commented on PR #45500:
URL: https://github.com/apache/spark/pull/45500#issuecomment-2009192048
@dongjoon-hyun Sure thing, let me revisit this and do it accordingly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47086][BUILD][CORE][SQL][UI] Migrate from Jetty 11 to Jetty 12 [spark]
Posted by "HiuKwok (via GitHub)" <gi...@apache.org>.
HiuKwok closed pull request #45500: [SPARK-47086][BUILD][CORE][SQL][UI] Migrate from Jetty 11 to Jetty 12
URL: https://github.com/apache/spark/pull/45500
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47086][BUILD][CORE][SQL][UI] Migrate from Jetty 11 to Jetty 12 [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #45500:
URL: https://github.com/apache/spark/pull/45500#discussion_r1523602207
##########
core/src/main/scala/org/apache/spark/TestUtils.scala:
##########
@@ -335,9 +336,9 @@ private[spark] object TestUtils extends SparkTestUtils {
// 0 as port means choosing randomly from the available ports
val server = new Server(new InetSocketAddress(Utils.localCanonicalHostName(), 0))
val resHandler = new ResourceHandler()
- resHandler.setResourceBase(resBaseDir)
- val handlers = new HandlerList()
- handlers.setHandlers(Array[Handler](resHandler, new DefaultHandler()))
+ resHandler.setBaseResource(ResourceFactory.of(resHandler).newResource(resBaseDir))
+ val handlers = new Handler.Sequence;
Review Comment:
Please follow the Scala style. We don't need `;` at the end.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47086][BUILD][CORE][SQL][UI] Migrate from Jetty 11 to Jetty 12 [spark]
Posted by "HiuKwok (via GitHub)" <gi...@apache.org>.
HiuKwok commented on PR #45500:
URL: https://github.com/apache/spark/pull/45500#issuecomment-2011184474
Left a comment on the Jira ticket and I'm closing this MR for now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47086][BUILD][CORE][SQL][UI] Migrate from Jetty 11 to Jetty 12 [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #45500:
URL: https://github.com/apache/spark/pull/45500#discussion_r1523613261
##########
sql/hive-thriftserver/src/main/java/org/apache/hive/service/cli/thrift/ThriftBinaryCLIService.java:
##########
@@ -92,9 +92,9 @@ protected void initializeServer() {
// Server args
int maxMessageSize = hiveConf.getIntVar(HiveConf.ConfVars.HIVE_SERVER2_THRIFT_MAX_MESSAGE_SIZE);
int requestTimeout = (int) hiveConf.getTimeVar(
- HiveConf.ConfVars.HIVE_SERVER2_THRIFT_LOGIN_TIMEOUT, TimeUnit.SECONDS);
+ HiveConf.ConfVars.HIVE_SERVER2_THRIFT_LOGIN_TIMEOUT, TimeUnit.SECONDS);
Review Comment:
This looks like a mistake. Please revert this file's changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47086][BUILD][CORE][SQL][UI] Migrate from Jetty 11 to Jetty 12 [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #45500:
URL: https://github.com/apache/spark/pull/45500#discussion_r1523603371
##########
core/src/main/scala/org/apache/spark/ui/JettyUtils.scala:
##########
@@ -149,6 +152,7 @@ private[spark] object JettyUtils extends Logging {
// Make sure we don't end up with "//" in the middle
val newUrl = new URL(new URL(request.getRequestURL.toString), prefixedDestPath).toString
response.sendRedirect(newUrl)
+// Response.sendRedirect(request, response, callback, location)
Review Comment:
Please clean up this leftover.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47086][BUILD][CORE][SQL][UI] Migrate from Jetty 11 to Jetty 12 [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #45500:
URL: https://github.com/apache/spark/pull/45500#issuecomment-2004112318
Hi, @HiuKwok . If Jetty 12 is missing the URL rewrite feature and we cannot use it, it's okay to add some comments and stop this migration effort.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47086][BUILD][CORE][SQL][UI] Migrate from Jetty 11 to Jetty 12 [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #45500:
URL: https://github.com/apache/spark/pull/45500#discussion_r1523601523
##########
core/src/main/scala/org/apache/spark/TestUtils.scala:
##########
@@ -46,15 +46,16 @@ import org.apache.logging.log4j.core.config.builder.api.ConfigurationBuilderFact
import org.eclipse.jetty.server.Handler
import org.eclipse.jetty.server.Server
import org.eclipse.jetty.server.handler.DefaultHandler
-import org.eclipse.jetty.server.handler.HandlerList
import org.eclipse.jetty.server.handler.ResourceHandler
+import org.eclipse.jetty.util.resource.ResourceFactory
import org.json4s.JsonAST.JValue
import org.json4s.jackson.JsonMethods.{compact, render}
import org.apache.spark.executor.TaskMetrics
import org.apache.spark.scheduler._
import org.apache.spark.util.{SparkTestUtils, Utils}
+
Review Comment:
Please remove this kind of empty lines to minimize your PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47086][BUILD][CORE][SQL][UI] Migrate from Jetty 11 to Jetty 12 [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #45500:
URL: https://github.com/apache/spark/pull/45500#discussion_r1523604616
##########
core/src/main/scala/org/apache/spark/ui/JettyUtils.scala:
##########
@@ -209,12 +213,12 @@ private[spark] object JettyUtils extends Logging {
override def filterServerResponseHeader(
clientRequest: HttpServletRequest,
- serverResponse: Response,
+ serverResponse: org.eclipse.jetty.client.Response,
Review Comment:
Please use `import` statement. If there is a conflict, you can use Scala's import renaming feature like `JMap` usage.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47086][BUILD][CORE][SQL][UI] Migrate from Jetty 11 to Jetty 12 [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #45500:
URL: https://github.com/apache/spark/pull/45500#discussion_r1523605466
##########
core/src/main/scala/org/apache/spark/ui/JettyUtils.scala:
##########
@@ -209,12 +213,12 @@ private[spark] object JettyUtils extends Logging {
override def filterServerResponseHeader(
clientRequest: HttpServletRequest,
- serverResponse: Response,
+ serverResponse: org.eclipse.jetty.client.Response,
headerName: String,
headerValue: String): String = {
if (headerName.equalsIgnoreCase("location")) {
val newHeader = createProxyLocationHeader(headerValue, clientRequest,
- serverResponse.getRequest().getURI())
+ serverResponse.getRequest.getURI)
Review Comment:
Do we need this change really? Otherwise, please revert all this kind of style change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47086][BUILD][CORE][SQL][UI] Migrate from Jetty 11 to Jetty 12 [spark]
Posted by "HiuKwok (via GitHub)" <gi...@apache.org>.
HiuKwok commented on PR #45500:
URL: https://github.com/apache/spark/pull/45500#issuecomment-1994466598
This is a draft MR and I'm still working on it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47086][BUILD][CORE][SQL][UI] Migrate from Jetty 11 to Jetty 12 [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #45500:
URL: https://github.com/apache/spark/pull/45500#discussion_r1523607249
##########
core/src/main/scala/org/apache/spark/ui/JettyUtils.scala:
##########
@@ -579,6 +575,12 @@ private[spark] case class ServerInfo(
}
+// private def getRedirectUrl(location: String): Unit = {
+//
+// val proxyUri = _proxyUri.stripSuffix("/")
+//
+// }
+
Review Comment:
Please clean up this left-over.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47086][BUILD][CORE][SQL][UI] Migrate from Jetty 11 to Jetty 12 [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #45500:
URL: https://github.com/apache/spark/pull/45500#issuecomment-1994982261
Thank you for sharing your AS-IS status, @HiuKwok .
For the following, it's a little surprising to me.
> However in this case we no longer have to control URL rewrite, because this is a static method from Jetty lib.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47086][BUILD][CORE][SQL][UI] Migrate from Jetty 11 to Jetty 12 [spark]
Posted by "HiuKwok (via GitHub)" <gi...@apache.org>.
HiuKwok commented on PR #45500:
URL: https://github.com/apache/spark/pull/45500#issuecomment-1994496507
@dongjoon-hyun @LuciferYang
During the past few weeks, I managed to re-write / update, all Jetty-related classes, things look fine in most of the Java / Scala classes.
However, due to the new handler class structure that Jetty 12 introduced, I'm not sure is that feasible to replicate what `ProxyRedirectHandler` is performing now.
If I understand correctly the initial intent of `ProxyRedirectHandler` is to override the redirect behaviour, in the case that Jetty decides to redirect the given request, BEFORE the request reaches any of the servlets.
However in Jetty 12, all Jetty handlers are switched to use the Jetty Request and Response wrapper object, hence it's no longer possible to override the redirect behaviour via the `sendRedirect` method call.
I have checked on the Jetty upgrade guide, which the guide suggests that all `sendDirect()` should be rewritten with ` Response.sendRedirect(request, response, callback, location)`.
However in this case we no longer have to control URL rewrite, because this is a static method from Jetty lib.
I wonder if you guys have an idea on this / or if any visible alternative can be implemented instead?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org