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-15373][WEB UI] Spark UI should show consistent timezones. #13158

Closed
wants to merge 5 commits into from
Closed

[SPARK-15373][WEB UI] Spark UI should show consistent timezones. #13158

wants to merge 5 commits into from

Conversation

dongjoon-hyun
Copy link
Member

@dongjoon-hyun dongjoon-hyun commented May 18, 2016

What changes were proposed in this pull request?

Currently, SparkUI shows two timezones in a single page when the timezone of browser is different from the server JVM timezone. The following is an example on Databricks CE which uses 'Etc/UTC' timezone.

  • The time of submitted column of list and pop-up description shows 2016/05/18 00:03:07
  • The time of timeline chart shows 2016/05/17 17:03:07.

Different Timezone

This PR fixes the timeline chart to use the same timezone by the followings.

  • Upgrade vis from 3.9.0(2015-01-16) to 4.16.1(2016-04-18)
  • Override moment of vis to get offset
  • Update AllJobsPage, JobPage, and StagePage.

How was this patch tested?

Manual. Run the following command and see the Spark UI's event timelines.

$ SPARK_SUBMIT_OPTS="-Dscala.usejavacp=true -Duser.timezone=Etc/UTC" bin/spark-submit --class org.apache.spark.repl.Main
...
scala> sql("select 1").head

@dongjoon-hyun
Copy link
Member Author

cc @davies

@SparkQA
Copy link

SparkQA commented May 18, 2016

Test build #58729 has finished for PR 13158 at commit d1bdb32.

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

@davies
Copy link
Contributor

davies commented May 18, 2016

cc @andrewor14 @zsxwing

@@ -172,6 +172,8 @@ private[ui] class AllJobsPage(parent: JobsTab) extends WebUIPage("") {

val jobEventJsonAsStrSeq = makeJobEvent(jobs)
val executorEventJsonAsStrSeq = makeExecutorEvent(executors)
val offset = TimeZone.getTimeZone(System.getProperty("user.timezone"))
Copy link
Member

Choose a reason for hiding this comment

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

I think this is what TimeZone.getDefault does, and it takes care of a few other details

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for review, @srowen . I'll replace that.

@dongjoon-hyun
Copy link
Member Author

@srowen . I updated the PR to use TimeZone.getDefault(). Thank you again.

@@ -172,6 +172,7 @@ private[ui] class AllJobsPage(parent: JobsTab) extends WebUIPage("") {

val jobEventJsonAsStrSeq = makeJobEvent(jobs)
val executorEventJsonAsStrSeq = makeExecutorEvent(executors)
val offset = TimeZone.getDefault().getOffset(System.currentTimeMillis()) / 1000 / 60
Copy link
Member

Choose a reason for hiding this comment

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

How about getRawOffset here? I think that's what you are looking for?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ur, I think you might miss my previous comment about this . Or, did I really miss something?

Since getRawOffset does not consider Daylight Saving Time, I needed to use the above.

Copy link
Member Author

Choose a reason for hiding this comment

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

For example, the second one is correct.

scala> java.util.TimeZone.getTimeZone("America/New_York").getRawOffset()
res0: Int = -18000000

scala> java.util.TimeZone.getTimeZone("America/New_York").getOffset(System.currentTimeMillis)
res1: Int = -14400000

Copy link
Member

Choose a reason for hiding this comment

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

Oh I did miss that yet. THis LGTM

Copy link
Member

Choose a reason for hiding this comment

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

nit: could you add a new method for this line in UIUtils and reuse it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure. That would be great. I'll refactor them like this.

  def getTimeZoneOffset() : Int =
    TimeZone.getDefault().getOffset(System.currentTimeMillis()) / 1000 / 60

@SparkQA
Copy link

SparkQA commented May 18, 2016

Test build #58767 has finished for PR 13158 at commit 38525b3.

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

@dongjoon-hyun
Copy link
Member Author

retest this please

@SparkQA
Copy link

SparkQA commented May 18, 2016

Test build #58789 has finished for PR 13158 at commit 38525b3.

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

@dongjoon-hyun
Copy link
Member Author

retest this please

@zsxwing
Copy link
Member

zsxwing commented May 18, 2016

@dongjoon-hyun could you revert the changes to the file permission?

@dongjoon-hyun
Copy link
Member Author

Oh, did I do that? Sorry, but which file did I do that?

@dongjoon-hyun
Copy link
Member Author

I see. The js files. I'll fix them. Thank you.

@zsxwing
Copy link
Member

zsxwing commented May 18, 2016

@dongjoon-hyun looks pretty good. Let me try locally.

@dongjoon-hyun
Copy link
Member Author

Oh, wait a moment. I'm testing on refactored method. I'll update the PR very soon again.

@dongjoon-hyun
Copy link
Member Author

I'm done. You can test locally now with the up-to-date code.
Thank you for review and local testing, @zsxwing !

@zsxwing
Copy link
Member

zsxwing commented May 18, 2016

LGTM pending tests

@SparkQA
Copy link

SparkQA commented May 18, 2016

Test build #58800 has finished for PR 13158 at commit 38525b3.

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

@SparkQA
Copy link

SparkQA commented May 18, 2016

Test build #58804 has finished for PR 13158 at commit 206171a.

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

@SparkQA
Copy link

SparkQA commented May 18, 2016

Test build #58801 has finished for PR 13158 at commit e170041.

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

@SparkQA
Copy link

SparkQA commented May 18, 2016

Test build #58803 has finished for PR 13158 at commit 0fa7d5b.

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

@SparkQA
Copy link

SparkQA commented May 18, 2016

Test build #2993 has finished for PR 13158 at commit 206171a.

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

@dongjoon-hyun
Copy link
Member Author

Hi, @zsxwing .
Finally, it passes the Jenkins test. :)

@srowen
Copy link
Member

srowen commented May 18, 2016

Merged to master/2.0

asfgit pushed a commit that referenced this pull request May 18, 2016
## What changes were proposed in this pull request?

Currently, SparkUI shows two timezones in a single page when the timezone of browser is different from the server JVM timezone. The following is an example on Databricks CE which uses 'Etc/UTC' timezone.

- The time of `submitted` column of list and pop-up description shows `2016/05/18 00:03:07`
- The time of `timeline chart` shows `2016/05/17 17:03:07`.

![Different Timezone](https://issues.apache.org/jira/secure/attachment/12804553/12804553_timezone.png)

This PR fixes the **timeline chart** to use the same timezone by the followings.
- Upgrade `vis` from 3.9.0(2015-01-16)  to 4.16.1(2016-04-18)
- Override `moment` of `vis` to get `offset`
- Update `AllJobsPage`, `JobPage`, and `StagePage`.

## How was this patch tested?

Manual. Run the following command and see the Spark UI's event timelines.

```
$ SPARK_SUBMIT_OPTS="-Dscala.usejavacp=true -Duser.timezone=Etc/UTC" bin/spark-submit --class org.apache.spark.repl.Main
...
scala> sql("select 1").head
```

Author: Dongjoon Hyun <dongjoon@apache.org>

Closes #13158 from dongjoon-hyun/SPARK-15373.

(cherry picked from commit cc6a47d)
Signed-off-by: Sean Owen <sowen@cloudera.com>
@dongjoon-hyun
Copy link
Member Author

Oh, thank you, @srowen !

@asfgit asfgit closed this in cc6a47d May 18, 2016
@dongjoon-hyun dongjoon-hyun deleted the SPARK-15373 branch July 20, 2016 07:35
asfgit pushed a commit that referenced this pull request Sep 2, 2016
## What changes were proposed in this pull request?

SPARK-15373 (#13158) updated the version of vis.js to 4.16.1. As of 4.0.0, some class was renamed like 'timeline to vis-timeline' but that ticket didn't care and now style is broken.

In this PR, I've restored the style by modifying `timeline-view.css` and `timeline-view.js`.

## How was this patch tested?

manual tests.

(If this patch involves UI changes, please attach a screenshot; otherwise, remove this)

* Before
<img width="1258" alt="2016-09-01 1 38 31" src="https://cloud.githubusercontent.com/assets/4736016/18141311/fddf1bac-6ff3-11e6-935f-28b389073b39.png">

* After
<img width="1256" alt="2016-09-01 3 30 19" src="https://cloud.githubusercontent.com/assets/4736016/18141394/49af65dc-6ff4-11e6-8640-70e20300f3c3.png">

Author: Kousuke Saruta <sarutak@oss.nttdata.co.jp>

Closes #14900 from sarutak/SPARK-17342.

(cherry picked from commit 2ab8dbd)
Signed-off-by: Sean Owen <sowen@cloudera.com>
ghost pushed a commit to dbtsai/spark that referenced this pull request Sep 2, 2016
## What changes were proposed in this pull request?

SPARK-15373 (apache#13158) updated the version of vis.js to 4.16.1. As of 4.0.0, some class was renamed like 'timeline to vis-timeline' but that ticket didn't care and now style is broken.

In this PR, I've restored the style by modifying `timeline-view.css` and `timeline-view.js`.

## How was this patch tested?

manual tests.

(If this patch involves UI changes, please attach a screenshot; otherwise, remove this)

* Before
<img width="1258" alt="2016-09-01 1 38 31" src="https://cloud.githubusercontent.com/assets/4736016/18141311/fddf1bac-6ff3-11e6-935f-28b389073b39.png">

* After
<img width="1256" alt="2016-09-01 3 30 19" src="https://cloud.githubusercontent.com/assets/4736016/18141394/49af65dc-6ff4-11e6-8640-70e20300f3c3.png">

Author: Kousuke Saruta <sarutak@oss.nttdata.co.jp>

Closes apache#14900 from sarutak/SPARK-17342.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants