Skip to content
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

[SPARK-12708][UI] Sorting task error in Stages Page when yarn mode. #10663

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions core/src/main/scala/org/apache/spark/ui/UIUtils.scala
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

package org.apache.spark.ui

import java.net.URLDecoder
import java.text.SimpleDateFormat
import java.util.{Date, Locale}

Expand Down Expand Up @@ -451,4 +452,19 @@ private[spark] object UIUtils extends Logging {
<span class="description-input">{desc}</span>
}
}

/**
* Decode URLParameter if URL is encoded by YARN-WebAppProxyServlet.
* Due to YARN-2844: WebAppProxyServlet cannot handle urls which contain encoded characters
* Therefore we need to decode it until we get the real URLParameter.
*/
def decodeURLParameter(urlParam: String): String = {
var param = urlParam
var decodedParam = URLDecoder.decode(param, "UTF-8")
while (param != decodedParam) {
param = decodedParam
decodedParam = URLDecoder.decode(param, "UTF-8")
}
param
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@

package org.apache.spark.ui.exec

import java.net.URLDecoder
import javax.servlet.http.HttpServletRequest

import scala.util.Try
Expand All @@ -30,19 +29,7 @@ private[ui] class ExecutorThreadDumpPage(parent: ExecutorsTab) extends WebUIPage
private val sc = parent.sc

def render(request: HttpServletRequest): Seq[Node] = {
val executorId = Option(request.getParameter("executorId")).map {
executorId =>
// Due to YARN-2844, "<driver>" in the url will be encoded to "%25253Cdriver%25253E" when
// running in yarn-cluster mode. `request.getParameter("executorId")` will return
// "%253Cdriver%253E". Therefore we need to decode it until we get the real id.
var id = executorId
var decodedId = URLDecoder.decode(id, "UTF-8")
while (id != decodedId) {
id = decodedId
decodedId = URLDecoder.decode(id, "UTF-8")
}
id
}.getOrElse {
val executorId = Option(request.getParameter("executorId")).getOrElse {
Copy link
Member

Choose a reason for hiding this comment

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

decodeURLParameter is not invoked.

We should say like as follows right?

Option(...).map { executorId =>
  UIUtils.decodeURLParameter(executorId)
}.getOrElse {
  ...
}

throw new IllegalArgumentException(s"Missing executorId parameter")
}
val time = System.currentTimeMillis()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ private[ui] class PoolPage(parent: StagesTab) extends WebUIPage("pool") {

def render(request: HttpServletRequest): Seq[Node] = {
listener.synchronized {
val poolName = request.getParameter("poolname")
val poolName = UIUtils.decodeURLParameter(request.getParameter("poolname"))
Copy link
Member

Choose a reason for hiding this comment

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

NullPointerException if poolname is not set. Could you use Option.map.getOrElse like other place?

require(poolName != null && poolName.nonEmpty, "Missing poolname parameter")

val poolToActiveStages = listener.poolToActiveStages
Expand Down
5 changes: 4 additions & 1 deletion core/src/main/scala/org/apache/spark/ui/jobs/PoolTable.scala
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@

package org.apache.spark.ui.jobs

import java.net.URLEncoder

import scala.collection.mutable.HashMap
import scala.xml.Node

Expand Down Expand Up @@ -59,7 +61,8 @@ private[ui] class PoolTable(pools: Seq[Schedulable], parent: StagesTab) {
case None => 0
}
val href = "%s/stages/pool?poolname=%s"
.format(UIUtils.prependBaseUri(parent.basePath), p.name)
.format(UIUtils.prependBaseUri(parent.basePath),
URLEncoder.encode(p.name, "UTF-8"))
Copy link
Member

Choose a reason for hiding this comment

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

This two lines can be fit into one line.

<tr>
<td>
<a href={href}>{p.name}</a>
Expand Down
4 changes: 3 additions & 1 deletion core/src/main/scala/org/apache/spark/ui/jobs/StagePage.scala
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,9 @@ private[ui] class StagePage(parent: StagesTab) extends WebUIPage("stage") {
val parameterTaskPrevPageSize = request.getParameter("task.prevPageSize")

val taskPage = Option(parameterTaskPage).map(_.toInt).getOrElse(1)
val taskSortColumn = Option(parameterTaskSortColumn).getOrElse("Index")
val taskSortColumn = Option(parameterTaskSortColumn).map { sortColumn =>
UIUtils.decodeURLParameter(sortColumn)
}.getOrElse("Index")
val taskSortDesc = Option(parameterTaskSortDesc).map(_.toBoolean).getOrElse(false)
val taskPageSize = Option(parameterTaskPageSize).map(_.toInt).getOrElse(100)
val taskPrevPageSize = Option(parameterTaskPrevPageSize).map(_.toInt).getOrElse(taskPageSize)
Expand Down
14 changes: 14 additions & 0 deletions core/src/test/scala/org/apache/spark/ui/UIUtilsSuite.scala
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,20 @@ class UIUtilsSuite extends SparkFunSuite {
s"\nRunning progress bar should round down\n\nExpected:\n$expected\nGenerated:\n$generated")
}

test("decodeURLParameter (SPARK-12708: Sorting task error in Stages Page when yarn mode.)") {
val encoded1 = "%252F"
val decoded1 = "/"
val encoded2 = "%253Cdriver%253E"
val decoded2 = "<driver>"

assert(decoded1.sameElements(decodeURLParameter(encoded1)))
assert(decoded2.sameElements(decodeURLParameter(encoded2)))
Copy link
Member

Choose a reason for hiding this comment

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

Why don't you just compare by === instead of sameElements?


// verify that no affect to decoded URL.
assert(decoded1.sameElements(decodeURLParameter(decoded1)))
assert(decoded2.sameElements(decodeURLParameter(decoded2)))
}

private def verify(
desc: String, expected: Elem, errorMsg: String = "", baseUrl: String = ""): Unit = {
val generated = makeDescription(desc, baseUrl)
Expand Down