-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-5931][CORE] Use consistent naming for time properties #5236
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
Changes from 1 commit
59bf9e1
404f8c3
7db6d2a
4933fda
c9f5cad
21ef3dd
064ebd6
7320c87
272c215
3352d34
3f1cfc8
6d1518e
2fcc91c
5181597
c6a0095
cde9bff
42477aa
9a29d8d
34f87c2
8f741e1
9e2547c
499bdf0
5232a36
3a12dd8
68f4e93
70ac213
647b5ac
1c0c07c
8613631
bac9edf
1858197
3b126e1
39164f9
b2fc965
dd0a680
bf779b0
76cfa27
5193d5f
6387772
19c31af
ff40bfe
28187bf
1465390
cbf41db
d4efd26
4e48679
1a1122c
cbd2ca6
6f651a8
7d19cdd
dc7bd08
69fedcc
8927e66
642a06d
25d3f52
bc04e05
951ca2d
f5fafcd
de3bff9
4526c81
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
…rsion
- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -51,15 +51,16 @@ private[spark] class HeartbeatReceiver(sc: SparkContext, scheduler: TaskSchedule | |
|
|
||
| // "spark.network.timeout" uses "seconds", while `spark.storage.blockManagerSlaveTimeoutMs` uses | ||
| // "milliseconds" | ||
| private val networkTimeoutS = sc.conf.get("spark.network.timeout","120s") | ||
| private val networkTimeoutS = Utils.timeStringAsS(sc.conf.get("spark.network.timeout","120s")) | ||
| private val executorTimeoutMs = Utils.timeStringAsMs( | ||
| sc.conf.get("spark.storage.blockManagerSlaveTimeout", networkTimeoutS)) | ||
| sc.conf.get("spark.storage.blockManagerSlaveTimeout", s"${networkTimeoutS}s")) | ||
|
|
||
| // "spark.network.timeoutInterval" uses "seconds", while | ||
| // "spark.storage.blockManagerTimeoutIntervalMs" uses "milliseconds" | ||
| private val networkTimeoutIntervalS = sc.conf.get("spark.network.timeoutInterval","60s") | ||
| private val networkTimeoutIntervalS = | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. minor, but if this field is not used anywhere else I'd inline the call below. |
||
| Utils.timeStringAsS(sc.conf.get("spark.network.timeoutInterval","60s")) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: space after |
||
| private val checkTimeoutIntervalMs = Utils.timeStringAsMs( | ||
| sc.conf.get("spark.storage.blockManagerTimeoutIntervalMs", networkTimeoutIntervalS)) | ||
| sc.conf.get("spark.storage.blockManagerTimeoutIntervalMs", s"${networkTimeoutIntervalS}s")) | ||
|
|
||
| private var timeoutCheckingTask: Cancellable = null | ||
| override def preStart(): Unit = { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1023,6 +1023,7 @@ private[spark] object Utils extends Logging { | |
| * Convert a passed time string (e.g. 50s, 100ms, or 250us) to a microsecond count for | ||
| * internal use. If no suffix is provided a direct conversion is attempted. | ||
| */ | ||
| @throws(classOf[NumberFormatException]) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a |
||
| private def timeStringToUs(str: String) : Long = { | ||
| try { | ||
| val lower = str.toLowerCase.trim() | ||
|
|
@@ -1045,6 +1046,7 @@ private[spark] object Utils extends Logging { | |
| * Convert a time parameter such as (50s, 100ms, or 250us) to microseconds for internal use. If | ||
| * no suffix is provided, the passed number is assumed to be in us. | ||
| */ | ||
| @throws(classOf[NumberFormatException]) | ||
| def timeStringAsUs(str: String): Long = { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are the microseconds methods used anywhere? I'm not aware of anything that uses this resolution, but might have missed it. There aren't us methods in SparkConf.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No - they can be removed. |
||
| timeStringToUs(str) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: too much indentation (here and in other places too) |
||
| } | ||
|
|
@@ -1053,6 +1055,7 @@ private[spark] object Utils extends Logging { | |
| * Convert a time parameter such as (50s, 100ms, or 250us) to microseconds for internal use. If | ||
| * no suffix is provided, the passed number is assumed to be in ms. | ||
| */ | ||
| @throws(classOf[NumberFormatException]) | ||
| def timeStringAsMs(str : String) : Long = { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no space before |
||
| timeStringToUs(str)/1000 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: spaces between |
||
| } | ||
|
|
@@ -1061,6 +1064,7 @@ private[spark] object Utils extends Logging { | |
| * Convert a time parameter such as (50s, 100ms, or 250us) to microseconds for internal use. If | ||
| * no suffix is provided, the passed number is assumed to be in seconds. | ||
| */ | ||
| @throws(classOf[NumberFormatException]) | ||
| def timeStringAsS(str : String) : Long = { | ||
| timeStringToUs(str)/1000/1000 | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -123,35 +123,49 @@ private static boolean isSymlink(File file) throws IOException { | |
| } | ||
|
|
||
| /** | ||
| * Convert a time parameter such as (50s, 100ms, or 250us) to microseconds for internal use | ||
| * Convert a passed time string (e.g. 50s, 100ms, or 250us) to a microsecond count for | ||
| * internal use. If no suffix is provided a direct conversion is attempted. | ||
| */ | ||
| public static long timeStringToUs(String str) throws IllegalArgumentException { | ||
| private static long timeStringToUs(String str) throws NumberFormatException { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This method can be very similar to the Scala one (using an
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks - didn't know if there was a similar construct. |
||
| String lower = str.toLowerCase().trim(); | ||
| if (lower.endsWith("ms")) { | ||
| return Long.parseLong(lower.substring(0, lower.length()-2)) * 1000; | ||
| } else if (lower.endsWith("us")) { | ||
| return Long.parseLong(lower.substring(0, lower.length()-2)); | ||
| } else if (lower.endsWith("s")) { | ||
| return Long.parseLong(lower.substring(0, lower.length()-1)) * 1000 * 1000; | ||
| } else {// Invalid suffix, force correct formatting | ||
| throw new IllegalArgumentException("Time must be specified as seconds (s), " + | ||
| try { | ||
| if (lower.endsWith("ms")) { | ||
| return Long.parseLong(lower.substring(0, lower.length() - 2)) * 1000; | ||
| } else if (lower.endsWith("us")) { | ||
| return Long.parseLong(lower.substring(0, lower.length() - 2)); | ||
| } else if (lower.endsWith("s")) { | ||
| return Long.parseLong(lower.substring(0, lower.length() - 1)) * 1000 * 1000; | ||
| } else {// Invalid suffix, force correct formatting | ||
| return Long.parseLong(lower); | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This whole method is really dense. Should we do something like the following instead so it's easier to follow:
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ugh, that was much longer than expected. It would have been a lot more concise in Scala without sacrificing any readability
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using regexes this would be pretty short. Or something along those lines.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, do that! |
||
| } catch(NumberFormatException e) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: space before |
||
| throw new NumberFormatException("Time must be specified as seconds (s), " + | ||
| "milliseconds (ms), or microseconds (us) e.g. 50s, 100ms, or 250us."); | ||
| } | ||
|
|
||
| } | ||
|
|
||
| /** | ||
| * Convert a time parameter such as (50s, 100ms, or 250us) to microseconds for internal use. If | ||
| * no suffix is provided, the passed number is assumed to be in us. | ||
| */ | ||
| public static long timeStringAsUs(String str) throws NumberFormatException { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just wanted to add the additional error message clarifying appropriate suffixes which is why I handle the error.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's fine but I mean the method need not be declared as |
||
| return timeStringToUs(str); | ||
| } | ||
|
|
||
| /** | ||
| * Convert a time parameter such as (50s, 100ms, or 250us) to milliseconds for internal use. | ||
| * Note: may round in some cases | ||
| * Convert a time parameter such as (50s, 100ms, or 250us) to microseconds for internal use. If | ||
| * no suffix is provided, the passed number is assumed to be in ms. | ||
| */ | ||
| public static long timeStringToMs(String str) throws IllegalArgumentException { | ||
| public static long timeStringAsMs(String str) throws NumberFormatException { | ||
| return timeStringToUs(str)/1000; | ||
| } | ||
|
|
||
| /** | ||
| * Convert a time parameter such as (50s, 100ms, or 250us) to seconds for internal use. | ||
| * Note: may round in some cases | ||
| * Convert a time parameter such as (50s, 100ms, or 250us) to microseconds for internal use. If | ||
| * no suffix is provided, the passed number is assumed to be in seconds. | ||
| */ | ||
| public static long timeStringToS(String str) throws IllegalArgumentException { | ||
| public static long timeStringAsS(String str) throws NumberFormatException { | ||
| return timeStringToUs(str)/1000/1000; | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -37,10 +37,11 @@ public boolean preferDirectBufs() { | |
|
|
||
| /** Connect timeout in milliseconds. Default 120 secs. */ | ||
| public int connectionTimeoutMs() { | ||
| long defaultTimeout = JavaUtils.timeStringToMs( | ||
| conf.get("spark.shuffle.io.connectionTimeout", | ||
| conf.get("spark.network.timeout", "120s"))); | ||
| return (int) defaultTimeout; | ||
| long defaultNetworkTimeoutS = JavaUtils.timeStringAsS( | ||
| conf.get("spark.network.timeout","120s")); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: space after |
||
| long defaultTimeoutS = JavaUtils.timeStringAsS( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this be parsed straight to milliseconds then? |
||
| conf.get("spark.shuffle.io.connectionTimeout", defaultNetworkTimeoutS + "s")); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should be 2 indents only on this line |
||
| return (int) defaultTimeoutS * 1000; | ||
| } | ||
|
|
||
| /** Number of concurrent connections between two nodes for fetching data. */ | ||
|
|
@@ -71,7 +72,7 @@ public int numConnectionsPerPeer() { | |
|
|
||
| /** Timeout for a single round trip of SASL token exchange, in milliseconds. */ | ||
| public int saslRTTimeoutMs() { | ||
| return (int) JavaUtils.timeStringToMs(conf.get("spark.shuffle.sasl.timeout", "30s")); | ||
| return (int) JavaUtils.timeStringAsS(conf.get("spark.shuffle.sasl.timeout", "30s")) * 1000; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here and in the next change. |
||
| } | ||
|
|
||
| /** | ||
|
|
@@ -85,7 +86,7 @@ public int saslRTTimeoutMs() { | |
| * Only relevant if maxIORetries > 0. | ||
| */ | ||
| public int ioRetryWaitTimeMs() { | ||
| return (int) JavaUtils.timeStringToMs(conf.get("spark.shuffle.io.retryWait", "5s")); | ||
| return (int) JavaUtils.timeStringAsS(conf.get("spark.shuffle.io.retryWait", "5s")) * 1000; | ||
| } | ||
|
|
||
| /** | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: space after
,