[SPARK-47086][BUILD][CORE][SQL][UI] Migrate from Jetty 11 to Jetty 12 #45500
Conversation
github-actions
bot
added
SQL
STRUCTURED STREAMING
WEB UI
BUILD
SPARK SHELL
YARN
CORE
DSTREAM
labels
|
This is a draft MR and I'm still working on it. |
|
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. If I understand correctly the initial intent of 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 I have checked on the Jetty upgrade guide, which the guide suggests that all I wonder if you guys have an idea on this / or if any visible alternative can be implemented instead? |
| import org.apache.spark.scheduler._ | ||
| import org.apache.spark.util.{SparkTestUtils, Utils} | ||
|
|
||
|
|
There was a problem hiding this comment.
Please remove this kind of empty lines to minimize your PR.
| val handlers = new HandlerList() | ||
| handlers.setHandlers(Array[Handler](resHandler, new DefaultHandler())) | ||
| resHandler.setBaseResource(ResourceFactory.of(resHandler).newResource(resBaseDir)) | ||
| val handlers = new Handler.Sequence; |
There was a problem hiding this comment.
Please follow the Scala style. We don't need ; at the end.
| // 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) |
There was a problem hiding this comment.
Please clean up this leftover.
| override def filterServerResponseHeader( | ||
| clientRequest: HttpServletRequest, | ||
| serverResponse: Response, | ||
| serverResponse: org.eclipse.jetty.client.Response, |
There was a problem hiding this comment.
Please use import statement. If there is a conflict, you can use Scala's import renaming feature like JMap usage.
| if (headerName.equalsIgnoreCase("location")) { | ||
| val newHeader = createProxyLocationHeader(headerValue, clientRequest, | ||
| serverResponse.getRequest().getURI()) | ||
| serverResponse.getRequest.getURI) |
There was a problem hiding this comment.
Do we need this change really? Otherwise, please revert all this kind of style change.
| // val proxyUri = _proxyUri.stripSuffix("/") | ||
| // | ||
| // } | ||
|
|
There was a problem hiding this comment.
Please clean up this left-over.
| // } | ||
| // super.sendRedirect(newTarget) | ||
| // } | ||
| // } |
There was a problem hiding this comment.
Please remove this leftover.
| } | ||
| isBindCollision(e.getCause) | ||
| case e: MultiException => | ||
| e.getThrowables.asScala.exists(isBindCollision) |
There was a problem hiding this comment.
This looks like a nice removal.
| 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 '/'") { |
There was a problem hiding this 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?
| 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); |
There was a problem hiding this comment.
This looks like a mistake. Please revert this file's changes.
|
Thank you for sharing your AS-IS status, @HiuKwok . For the following, it's a little surprising to me.
|
|
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. |
|
@dongjoon-hyun Sure thing, let me revisit this and do it accordingly. |
|
Left a comment on the Jira ticket and I'm closing this MR for now. |
|
Thank you for all of your contributions here, @HiuKwok ! |
2 participants
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?
Does this PR introduce any user-facing change?
How was this patch tested?
Was this patch authored or co-authored using generative AI tooling?