Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
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
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,18 @@ class HistoryServer(
}

val appId = parts(1)
val attemptId = if (parts.length >= 3) Some(parts(2)) else None
var shouldAppendAttemptId = false
val attemptId = if (parts.length >= 3) {
Some(parts(2))
} else {
val lastAttemptId = provider.getApplicationInfo(appId).flatMap(_.attempts.head.attemptId)
if (lastAttemptId.isDefined) {
shouldAppendAttemptId = true
lastAttemptId
} else {
None
}
}

// Since we may have applications with multiple attempts mixed with applications with a
// single attempt, we need to try both. Try the single-attempt route first, and if an
Expand All @@ -97,7 +108,17 @@ class HistoryServer(
// the app's UI, and all we need to do is redirect the user to the same URI that was
// requested, and the proper data should be served at that point.
// Also, make sure that the redirect url contains the query string present in the request.
val requestURI = req.getRequestURI + Option(req.getQueryString).map("?" + _).getOrElse("")
val attemptPart = if (shouldAppendAttemptId) {
if (!req.getRequestURI.endsWith("/")) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Simpler:

val redirect = if (shouldAppendAttemptId) {
  req.getRequestURI().stripSuffix("/") + "/" + attemptId.get
} else {
  req.getRequestURI()
}
val query = Option(req.getQueryString).map("?" + _).getOrElse("")
res.sendRedirect(...)

"/" + attemptId.get
} else {
attemptId.get
}
} else {
""
}
val requestURI = req.getRequestURI +
attemptPart + Option(req.getQueryString).map("?" + _).getOrElse("")
res.sendRedirect(res.encodeRedirectURL(requestURI))
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -632,6 +632,49 @@ class HistoryServerSuite extends SparkFunSuite with BeforeAndAfter with Matchers
}
}

test("SPARK-28564: Access history application defaults to the last attempt id") {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hi, @cxzl25 .
Since you created a JIRA issue with Improvement type, we don't use JIRA ID in the test case name.
That's because we don't add JIRA ID prefix for all new features.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ok, I remember now. thanks for your reminder


def getRedirectUrl(url: URL): (Int, String) = {
val connection = url.openConnection().asInstanceOf[HttpURLConnection]
connection.setRequestMethod("GET")
connection.setUseCaches(false)
connection.setDefaultUseCaches(false)
connection.setInstanceFollowRedirects(false)
connection.connect()
val code = connection.getResponseCode()
val location = connection.getHeaderField("Location")
(code, location)
}

def buildPageAttemptUrl(appId: String, attemptId: Option[Int]): URL = {
attemptId match {
case Some(id) =>
new URL(s"http://localhost:$port/history/$appId/$id")
case None =>
new URL(s"http://localhost:$port/history/$appId")
}
}

val oneAttemptAppId = "local-1430917381534"
HistoryServerSuite.getUrl(buildPageAttemptUrl(oneAttemptAppId, None))

val multiAttemptAppid = "local-1430917381535"
val lastAttemptId = Some(2)
val lastAttemptUrl = buildPageAttemptUrl(multiAttemptAppid, lastAttemptId)
Seq(None, Some(1), Some(2)).foreach { attemptId =>
val url = buildPageAttemptUrl(multiAttemptAppid, attemptId)
val (code, location) = getRedirectUrl(url)
assert(code === 302, s"Unexpected status code $code for $url")
attemptId match {
case None =>
assert(location.stripSuffix("/") === lastAttemptUrl.toString, s"Unexpected url")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The "unexpected url" message is unnecessary (also below).

case _ =>
assert(location.stripSuffix("/") === url.toString, s"Unexpected url")
}
HistoryServerSuite.getUrl(new URL(location))
}
}

def getContentAndCode(path: String, port: Int = port): (Int, Option[String], Option[String]) = {
HistoryServerSuite.getContentAndCode(new URL(s"http://localhost:$port/api/v1/$path"))
}
Expand Down