[SPARK-16986][WEB-UI] Converter Started, Completed and Last Updated to client time zone in history page #19640

wangyum · 2017-11-02T08:45:13Z

What changes were proposed in this pull request?

This PR is converted the Started, Completed and Last Updated to client local time in the history page.

How was this patch tested?

Manual tests for Chrome, Firefox and Safari

Before modifying:

After modifying:

srowen · 2017-11-02T08:50:49Z

@wangyum this was a WontFix. See linked JIRAs for some of the discussion and implications.

wangyum · 2017-11-02T08:51:35Z

@srowen I have read the discussion about this issue by #15803. But users feel that the current readability is still bad.

srowen · 2017-11-02T09:32:16Z

This is a significant behavior change though. If anything we need to move towards GMT, not towards varying with whatever the server's timezone is.

SparkQA · 2017-11-02T11:22:13Z

Test build #83326 has finished for PR 19640 at commit 17bddef.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-11-03T07:05:01Z

Test build #83383 has finished for PR 19640 at commit c7339c8.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

wangyum · 2017-11-03T07:07:19Z

retest this please

SparkQA · 2017-11-03T10:32:05Z

Test build #83385 has finished for PR 19640 at commit c7339c8.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

wangyum · 2017-11-03T10:39:35Z

@srowen We can configure time zone byspark.history.timeZone now.

dbolshak · 2017-11-03T10:41:32Z

core/src/main/scala/org/apache/spark/deploy/SparkHadoopUtil.scala

@@ -426,6 +426,10 @@ class SparkHadoopUtil extends Logging {
    ugi.getAuthenticationMethod() == UserGroupInformation.AuthenticationMethod.PROXY
  }

+  def getTimeZone: TimeZone = {


I would write it like

def timeZone: TimeZone = TimeZone.getTimeZone(sparkConf.get("spark.history.timeZone", "GMT"))

dbolshak · 2017-11-03T10:44:37Z

core/src/main/scala/org/apache/spark/deploy/SparkHadoopUtil.scala

@@ -426,6 +426,10 @@ class SparkHadoopUtil extends Logging {
    ugi.getAuthenticationMethod() == UserGroupInformation.AuthenticationMethod.PROXY
  }

+  def getTimeZone: TimeZone = {
+    TimeZone.getTimeZone(sparkConf.get("spark.history.timeZone", "GMT"))


It's a bit confusing using spark.history.timeZone property in getTimeZone method of SparkHadoopUtil.

jiangxb1987 · 2017-11-03T15:32:23Z

cc @ueshin

dbolshak · 2017-11-06T14:12:02Z

core/src/main/scala/org/apache/spark/util/Utils.scala

@@ -2742,6 +2742,11 @@ private[spark] object Utils extends Logging {
    }
  }

+  def getTimeZone: TimeZone = {


Could you please image that this method will be used outside scope of the current issue?
What's about users who does not have spark history at all?

The name of method, the class and the package does not tell us that this method somehow related ONLY to history server, such API makes me easy to do wrong assumptions.

SparkQA · 2017-11-06T16:27:36Z

Test build #83480 has finished for PR 19640 at commit f36023a.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

jiangxb1987 · 2017-11-07T13:26:09Z

core/src/main/scala/org/apache/spark/util/Utils.scala

@@ -2742,6 +2742,11 @@ private[spark] object Utils extends Logging {
    }
  }

+  def getTimeZone: TimeZone = {
+    val sparkConf = new SparkConf(false).loadFromSystemProperties(true)


Can we make SparkConf as a input param instead of create a new instance for every function call?

SparkQA · 2017-11-07T19:57:51Z

Test build #83550 has finished for PR 19640 at commit 5291d16.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

jiangxb1987 · 2017-11-08T13:01:29Z

core/src/main/scala/org/apache/spark/deploy/SparkHadoopUtil.scala

@@ -426,6 +426,8 @@ class SparkHadoopUtil extends Logging {
    ugi.getAuthenticationMethod() == UserGroupInformation.AuthenticationMethod.PROXY
  }

+  def getHistoryServerTimeZone: String = sparkConf.get("spark.history.timeZone", "GMT")


Please add this config to internal/config.

vanzin · 2017-11-08T19:53:29Z

I'm not a fan of this change; this seems like it should be done client-side, so that the browser shows timestamps in its own time zone, and the server should only know about UTC. The API probably should be left alone - especially because with this change it becomes a little confusing, since you have a timestamp in whatever is the configured server tz, and the raw value which is in UTC.

SparkQA · 2017-11-08T19:58:21Z

Test build #83601 has finished for PR 19640 at commit d9994ea.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2017-11-10T15:50:11Z

Since a spark cluster may be used by people from different timezones, I think at server side we should just store seconds from unix epoch(time epoch), which is comparable though different timezones.

For the REST API, personally I think the server just need to send time epoch, but for ease of use, sending a timestamp string with GMT is acceptable.

For the UI part, I think it's more reasonable to interpret the time epoch using browser's local timezone, but we don't need a config.

wangyum · 2017-11-11T00:34:00Z

@cloud-fan For the UI part, how about this PR: #14577

cloud-fan · 2017-11-11T00:48:21Z

I think it makes sense, but the implementation is too hacky. We should extract the time epoch and convert it to timestamp string with local timezone.

SparkQA · 2017-11-11T18:57:21Z

Test build #83727 has finished for PR 19640 at commit 7b04e08.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2017-11-12T23:36:13Z

core/src/main/resources/org/apache/spark/ui/static/historypage.js

-  if (date <= 0) return "-";
-  else return date.split(".")[0].replace("T", " ");
+  if (date <= 0) {
+    return "-";


just for curious, what does a single - mean here? invalid value?

Yes, invalid value. Maybe we can remove this.

wangyum · 2017-11-14T10:10:08Z

How about this?

cloud-fan · 2017-11-14T10:22:54Z

how about Client local time zone? any better ideas?

BTW the last updated timestamp string at the beginning, which time zone it is?

wangyum · 2017-11-14T11:17:43Z

It is client local time zone:
https://github.com/apache/spark/blob/master/core/src/main/resources/org/apache/spark/ui/static/historypage-common.js#L22

But the format is 11/14/2017, 7:16:34 PM.

cloud-fan · 2017-11-14T11:31:34Z

maybe we can move formatDate to util and use it there.

wangyum · 2017-11-14T12:18:00Z

It looks like this:

cloud-fan · 2017-11-14T12:24:06Z

why it has PM postfix at the end?

wangyum · 2017-11-14T12:50:16Z

Generated by the toLocaleTimeString () method:

cloud-fan · 2017-11-14T13:21:56Z

sorry, I mean we should follow the format we use in completed, started...

wangyum · 2017-11-14T14:08:24Z

core/src/test/scala/org/apache/spark/deploy/history/HistoryServerSuite.scala

@@ -352,7 +352,7 @@ class HistoryServerSuite extends SparkFunSuite with BeforeAndAfter with Matchers
        .map(_.get)
        .filter(_.startsWith(url)).toList

-      // there are atleast some URL links that were generated via javascript,
+      // there are at least some URL links that were generated via javascript,


wangyum · 2017-11-14T14:11:12Z

core/src/main/resources/org/apache/spark/ui/static/utils.js

@@ -46,3 +46,25 @@ function formatBytes(bytes, type) {
    var i = Math.floor(Math.log(bytes) / Math.log(k));
    return parseFloat((bytes / Math.pow(k, i)).toFixed(dm)) + ' ' + sizes[i];
 }
+
+function padZeroes(num) {
+  return ("0" + num).slice(-2);


num.toString().padStart(2, "0") cause org.apache.spark.deploy.history.HistoryServerSuite test failed.

do you know why?

padStart with poor compatibility:

Feature Chrome Edge Firefox Internet Explorer Opera Safari

Basic support 57 15 48 No 44 10

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/padStart

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/slice

what does the test use?

TagNameQuery("a") gets result from WebBrowser, This browser doesn't support this function.

wangyum · 2017-11-14T14:11:46Z

core/src/main/resources/org/apache/spark/ui/static/historypage.js

@@ -37,11 +37,6 @@ function makeIdNumeric(id) {
  return resl;
 }

-function formatDate(date) {


Move to utils.js

cloud-fan · 2017-11-14T15:37:01Z

core/src/main/resources/org/apache/spark/ui/static/utils.js

+}
+
+function getTimeZone() {
+  return new Date().toString().match(/\(([A-Za-z\s].*)\)/)[1];


how about Intl.DateTimeFormat().resolvedOptions().timeZone

This timeZone seems incorrect. Safari gets Asia/Shanghai, but Chrome gets America/Chicago on the same computer.

How about just include this change to release notes.

did you have proxy on your chrome? It works fine at my Chrome.

cloud-fan · 2017-11-14T15:38:20Z

core/src/main/scala/org/apache/spark/deploy/history/HistoryPage.scala

@@ -56,6 +56,10 @@ private[history] class HistoryPage(parent: HistoryServer) extends WebUIPage("")
            }

            {
+            <p>Client local time zone: <span id="time-zone"></span></p>


any more feedback on the naming?

SparkQA · 2017-11-14T16:38:30Z

Test build #83848 has finished for PR 19640 at commit 4a78965.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

wangyum · 2017-11-14T23:25:47Z

retest this please.

SparkQA · 2017-11-15T02:41:30Z

Test build #83868 has finished for PR 19640 at commit 4a78965.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2017-11-15T16:54:18Z

core/src/main/resources/org/apache/spark/ui/static/utils.js

+  try {
+    return Intl.DateTimeFormat().resolvedOptions().timeZone;
+  } catch(ex) {
+    return new Date().toString().match(/\((.*)\)/)[1];


if you really want to keep this code, please add comment to explain this regex.

wangyum · 2017-11-15T17:33:34Z

core/src/main/resources/org/apache/spark/ui/static/utils.js

+}
+
+function getTimeZone() {
+  try {


Some old browser doesn't support this function. so I surround with try catch.

Feature Chrome Edge Firefox Internet Explorer Opera Safari

Basic support 24 12 29 11 15 10

computed timeZone 35 14 53 No 30 10

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/DateTimeFormat/resolvedOptions

cc @srowen does Spark have a guarantee about which version of which browser to support?

Spark does not have a list of supported browsers, from my experience most of us just test new UI changes on whatever browsers we have (In my case the latest Safari, Chrome and Firefox ESR). Though I'd like to think we don't have any IE users, we can't make that assumption while IE 11 is still supported by MS.

SparkQA · 2017-11-15T20:04:30Z

Test build #83900 has finished for PR 19640 at commit 40ec67e.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-11-15T20:43:45Z

Test build #83902 has finished for PR 19640 at commit 08f5e68.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

ajbozarth · 2017-11-15T21:16:53Z

I'm still not 100% convinced this is the best change, but the implementation LGTM.

cloud-fan · 2017-11-15T22:16:48Z

It's a pure UI change so no compatibility issues. The new UI LGTM

vanzin · 2017-12-12T18:06:54Z

JS is not really my thing, but other have reviewed and seem ok. I'm fine with this being done client-side.

Merging to master.

Replace GMT with history server side TimeZone.

17bddef

Support configure time zone.

c7339c8

dbolshak reviewed Nov 3, 2017

View reviewed changes

Move getTimeZone from SparkHadoopUtil to Util

f36023a

wangyum changed the title ~~[SPARK-16986][WEB-UI] Replace GMT with history server side TimeZone.~~ [SPARK-16986][CORE][WEB-UI] Support configure history server time zone Nov 6, 2017

dbolshak reviewed Nov 6, 2017

View reviewed changes

jiangxb1987 reviewed Nov 7, 2017

View reviewed changes

getTimeZone -> getHistoryServerTimeZone

5291d16

jiangxb1987 reviewed Nov 8, 2017

View reviewed changes

Add "spark.history.timeZone" to internal/config.

d9994ea

Only convert the UI part to client time zone

7b04e08

wangyum changed the title ~~[SPARK-16986][CORE][WEB-UI] Support configure history server time zone~~ [SPARK-16986][WEB-UI] Converter Started, Completed and Last Updated to client time zone in history page Nov 11, 2017

cloud-fan reviewed Nov 12, 2017

View reviewed changes

Show local time zone

cb1db11

Fix HistoryServerSuite test error

4a78965

wangyum commented Nov 14, 2017

View reviewed changes

cloud-fan reviewed Nov 14, 2017

View reviewed changes

Intl.DateTimeFormat().resolvedOptions().timeZone

40ec67e

cloud-fan reviewed Nov 15, 2017

View reviewed changes

Intl.DateTimeFormat().resolvedOptions().timeZone

08f5e68

wangyum commented Nov 15, 2017

View reviewed changes

asfgit closed this in d500773 Dec 12, 2017

[SPARK-16986][WEB-UI] Converter Started, Completed and Last Updated to client time zone in history page #19640

[SPARK-16986][WEB-UI] Converter Started, Completed and Last Updated to client time zone in history page #19640

Conversation

wangyum commented Nov 2, 2017 • edited

What changes were proposed in this pull request?

How was this patch tested?

Before modifying:

After modifying:

srowen commented Nov 2, 2017

wangyum commented Nov 2, 2017

srowen commented Nov 2, 2017

SparkQA commented Nov 2, 2017

SparkQA commented Nov 3, 2017

wangyum commented Nov 3, 2017

SparkQA commented Nov 3, 2017

wangyum commented Nov 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiangxb1987 commented Nov 3, 2017

Choose a reason for hiding this comment

SparkQA commented Nov 6, 2017

Choose a reason for hiding this comment

SparkQA commented Nov 7, 2017

Choose a reason for hiding this comment

vanzin commented Nov 8, 2017

SparkQA commented Nov 8, 2017

cloud-fan commented Nov 10, 2017

wangyum commented Nov 11, 2017

cloud-fan commented Nov 11, 2017

SparkQA commented Nov 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangyum commented Nov 14, 2017

cloud-fan commented Nov 14, 2017

wangyum commented Nov 14, 2017

cloud-fan commented Nov 14, 2017

wangyum commented Nov 14, 2017

cloud-fan commented Nov 14, 2017

wangyum commented Nov 14, 2017

cloud-fan commented Nov 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Nov 14, 2017

wangyum commented Nov 14, 2017

SparkQA commented Nov 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Nov 15, 2017

SparkQA commented Nov 15, 2017

ajbozarth commented Nov 15, 2017

cloud-fan commented Nov 15, 2017

vanzin commented Dec 12, 2017

wangyum commented Nov 2, 2017 •

edited