HyukjinKwon commented on code in PR #59089:
URL: https://github.com/apache/spark/pull/59089#discussion_r4172835138
##########
core/src/main/scala/org/apache/spark/ui/JettyUtils.scala:
##########
@@ -315,6 +318,15 @@ private[spark] object JettyUtils extends Logging {
val newHeader = createProxyLocationHeader(headerValue, clientRequest,
serverResponse.getRequest().getURI())
if (newHeader != null) {
+ if (normalizedReverseProxyUrl.nonEmpty) {
Review Comment:
**Non-blocking (P2):** The reverse-proxy rewriting added here (the
`normalizedReverseProxyUrl` prepend into the forwarded `X-Forwarded-Context`,
and the proxied redirect-`Location` rewrite below) is the PR's core mechanism,
but no test drives a `/proxy/<id>` request through this handler -- the new
`MasterWebUISuite` test exercises the `/app/kill` redirect, a different path. A
regression that dropped or mangled the `reverseProxyUrl` prefix in the
forwarded context or the rewritten Location would pass CI. Worth adding a test
that proxies a request with `reverseProxy` enabled and asserts both the
prefixed `X-Forwarded-Context` and the rewritten `Location`.
**Verification:**
- **Regression:** Dropping or mangling the reverseProxyUrl prefix in the
forwarded context or the Location rewrite fails the test.
##########
core/src/main/scala/org/apache/spark/SparkContext.scala:
##########
@@ -2729,6 +2751,13 @@ class SparkContext(config: SparkConf) extends Logging {
log"${MDC(LogKeys.TOTAL_TIME, System.currentTimeMillis() - startTime)}
ms)")
}
+ private def isAppSpecificProxyBase(proxyBase: String): Boolean = {
+ val clean = proxyBase.stripSuffix("/")
Review Comment:
**Nit (P3):** `isAppSpecificProxyBase` recognizes `/proxy/application_`,
`/proxy/app-`, and `/proxy/spark-`, but local-mode app ids are `local-<ts>`, so
a pre-existing `/proxy/local-<id>` base is not treated as app-specific. The
`avoid inheriting previous YARN application proxy path` test confirms the
intended guard for `application_`; the same inheritance scenario with a
`local-` base would instead be kept as a deployment prefix and compounded into
`/proxy/local-<id>/proxy/<newId>` (and restored stale on stop). Adding
`/proxy/local-` here would close the asymmetry.
**Verification:**
- **Regression:** A test with a pre-set /proxy/local-<id> base asserts the
new context derives /proxy/<newId> (no compounding), mirroring the
YARN-inheritance test.
##########
core/src/main/scala/org/apache/spark/SparkContext.scala:
##########
@@ -2719,6 +2733,14 @@ class SparkContext(config: SparkConf) extends Logging {
if (_statusStore != null) {
_statusStore.close()
}
+ if (_proxyBaseSetByContext) {
Review Comment:
**Nit (P3):** The restore branch here (`case Some(oldBase) if
!isAppSpecificProxyBase(oldBase) => System.setProperty(...)`) has no test: the
`fallback spark.ui.proxyBase when UI_REVERSE_PROXY_URL is empty` case asserts
the derived base after construction, then clears the property in `finally`
without asserting that `stop()` set it back to the saved base. A regression
that failed to restore it would not fail any test.
**Verification:**
- **Regression:** A regression that fails to restore the saved base on
stop() fails the test.
##########
core/src/main/scala/org/apache/spark/deploy/master/Master.scala:
##########
@@ -1394,11 +1394,24 @@ private[deploy] class Master(
desc.copy(command = desc.command.copy(arguments = arguments, javaOpts =
javaOpts))
}
+ private def maybeAddReverseProxyConfig(desc: DriverDescription):
DriverDescription = {
Review Comment:
**Nit (P3):** Only the positive injection path of
`maybeAddReverseProxyConfig` is asserted. The `!reverseProxy` early return
(javaOpts must be unchanged when reverse proxy is off), the `None` case
(empty/absent URL), and the dedup
`.filter(!_.startsWith("-Dspark.ui.reverseProxyUrl="))` each have distinct
observable effects and no test -- e.g. injecting the opt when reverse proxy is
off, or appending a duplicate, would pass. Worth adding negative and dedup
cases.
**Verification:**
- **Regression:** Injecting the opt when reverse proxy is off, or appending
a duplicate opt, fails a test.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
