[SPARK-24560][CORE][MESOS] Fix some getTimeAsMs as getTimeAsSeconds by xueyumusic · Pull Request #21567 · apache/spark

xueyumusic · 2018-06-14T11:23:27Z

What changes were proposed in this pull request?

This PR replaces some "getTimeAsMs" with "getTimeAsSeconds". This will return a wrong value when the user specifies a value without a time unit.

How was this patch tested?

manual test

Please review http://spark.apache.org/contributing.html before opening a pull request.

maropu · 2018-06-15T07:53:36Z


 private[this] val killPollingIntervalMs: Long =
- conf.getTimeAsMs("spark.task.reaper.pollingInterval", "10s")
+ conf.getTimeAsSeconds("spark.task.reaper.pollingInterval", "10s") * 1000L


plz use DurationConversions.

maropu · 2018-06-15T07:53:54Z

Can you add tests in SparkConfSuite?

maropu · 2018-06-15T07:55:44Z


 /** How long to wait before killing the python worker if a task cannot be interrupted. */
- private val taskKillTimeout = env.conf.getTimeAsMs("spark.python.task.killTimeout", "2s")
+ private val taskKillTimeout = env.conf.getTimeAsSeconds("spark.python.task.killTimeout", "2s") * 1000L


Nit: Probably, we'd be better to add Ms in the suffix, => taskKillTimeoutMs

xueyumusic · 2018-06-15T11:17:42Z

I have made some modification, @maropu please review the code, thanks

ueshin · 2018-06-16T02:59:19Z

ok to test.

SparkQA · 2018-06-16T03:04:47Z

Test build #91959 has finished for PR 21567 at commit a8dc241.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

maropu · 2018-06-16T03:05:13Z

cc @jiangxb1987

maropu · 2018-06-16T03:05:34Z

@xueyumusic Can you fix the error first?

SparkQA · 2018-06-16T07:05:02Z

Test build #91964 has finished for PR 21567 at commit a69df11.

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

maropu · 2018-06-16T09:26:53Z

retest this please

SparkQA · 2018-06-16T13:01:56Z

Test build #91971 has finished for PR 21567 at commit a69df11.

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

SparkQA · 2018-06-17T04:23:09Z

Test build #91984 has started for PR 21567 at commit 0dd57ea.

maropu · 2018-06-18T00:56:47Z

retest this please

SparkQA · 2018-06-18T05:10:50Z

Test build #92008 has finished for PR 21567 at commit 0dd57ea.

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

jiangxb1987 · 2018-06-19T13:48:10Z


 /** How long to wait before killing the python worker if a task cannot be interrupted. */
- private val taskKillTimeout = env.conf.getTimeAsMs("spark.python.task.killTimeout", "2s")
+ private val taskKillTimeoutMs = env.conf.getTimeAsSeconds("spark.python.task.killTimeout",


The conf is not documented, but I think it's designed to accept values like 1.5s
cc @zsxwing

jiangxb1987 · 2018-06-19T13:49:09Z

 // Timeout to wait for when trying to terminate a driver.
 private val DRIVER_TERMINATE_TIMEOUT_MS =
- conf.getTimeAsMs("spark.worker.driverTerminateTimeout", "10s")
+ conf.getTimeAsSeconds("spark.worker.driverTerminateTimeout", "10s").seconds.toMillis


jiangxb1987 · 2018-06-19T13:52:10Z


 private[this] val killPollingIntervalMs: Long =
- conf.getTimeAsMs("spark.task.reaper.pollingInterval", "10s")
+ conf.getTimeAsSeconds("spark.task.reaper.pollingInterval", "10s").seconds.toMillis


Actually I believe this is change things from right to wrong. If you want to use a second value then you shall use sth like 1s.

jiangxb1987 · 2018-06-19T13:53:24Z

 // Update period of progress bar, in milliseconds
 private val updatePeriodMSec =
- sc.getConf.getTimeAsMs("spark.ui.consoleProgress.update.interval", "200")
+ sc.getConf.getTimeAsMs("spark.ui.consoleProgress.update.interval", "200ms")


We shall document this config.

jiangxb1987 · 2018-06-19T13:55:07Z

Overall I don't think the current logic shall be modified. However, it shall be useful to document some the configs mentioned in this PR.

xueyumusic · 2018-06-20T02:28:56Z

I see, thanks for your review and guidance, @jiangxb1987 @maropu , I will try to add related config to doc and close this PR, thank you

fix getTimeAs method
10bf41e

xueyumusic changed the title ~~[SPARK-24560][SS][MESOS] Fix some getTimeAsMs as getTimeAsSeconds~~[SPARK-24560][CORE][MESOS] Fix some getTimeAsMs as getTimeAsSecondsJun 14, 2018

maropu reviewed Jun 15, 2018
View reviewed changes

xueyumusic added 2 commits June 15, 2018 19:03

use DurationConvertions etc
3f59a05

use DurationConvertions etc
a8dc241

fix check style error
a69df11

compatible with spark.akka.retry.wait deprecated config
0dd57ea

jiangxb1987 reviewed Jun 19, 2018
View reviewed changes

xueyumusic mentioned this pull request Jun 23, 2018
[SPARK-24639][DOC] Add three config in the doc #21624
Closed

xueyumusic closed this Jun 23, 2018

Uh oh!

Conversation

xueyumusic commented Jun 14, 2018

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

maropuJun 15, 2018

Choose a reason for hiding this comment

Uh oh!

maropu commented Jun 15, 2018

Uh oh!

maropuJun 15, 2018

Choose a reason for hiding this comment

Uh oh!

xueyumusic commented Jun 15, 2018

Uh oh!

ueshin commented Jun 16, 2018

Uh oh!

SparkQA commented Jun 16, 2018

Uh oh!

maropu commented Jun 16, 2018

Uh oh!

maropu commented Jun 16, 2018

Uh oh!

SparkQA commented Jun 16, 2018

Uh oh!

maropu commented Jun 16, 2018

Uh oh!

SparkQA commented Jun 16, 2018

Uh oh!

SparkQA commented Jun 17, 2018

Uh oh!

maropu commented Jun 18, 2018

Uh oh!

SparkQA commented Jun 18, 2018

Uh oh!

jiangxb1987Jun 19, 2018

Choose a reason for hiding this comment

Uh oh!

jiangxb1987Jun 19, 2018

Choose a reason for hiding this comment

Uh oh!

jiangxb1987Jun 19, 2018

Choose a reason for hiding this comment

Uh oh!

jiangxb1987Jun 19, 2018

Choose a reason for hiding this comment

Uh oh!

jiangxb1987 commented Jun 19, 2018

Uh oh!

xueyumusic commented Jun 20, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants