-
Notifications
You must be signed in to change notification settings - Fork 28.7k
[SPARK-6556][Core] Fix wrong parsing logic of executorTimeoutMs and checkTimeoutIntervalMs in HeartbeatReceiver #5209
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
Conversation
…Ms in HeartbeatReceiver
Test build #29226 has started for PR 5209 at commit
|
private val checkTimeoutIntervalMs = sc.conf.getLong("spark.network.timeoutInterval", | ||
sc.conf.getLong("spark.storage.blockManagerTimeoutIntervalMs", 60)) * 1000 | ||
|
||
private val executorTimeoutMs = sc.conf.getOption("spark.network.timeout").map(_.toLong * 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.
You might comment that timeout
is in seconds and (obviously) blockManagerSlaveTimeoutMs
is milliseconds. I agree with the change though. These properties you're fixing aren't documented right? the only ref I saw on the mailing list clearly directed people to set values like "60000", which is correctly in ms.
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.
I have not seen them in any docs. However, from the current codes here, I think it's trying to maintain the compatibility.
Test build #29227 has started for PR 5209 at commit
|
LGTM |
Test build #29226 has finished for PR 5209 at commit
|
Test PASSed. |
Test build #29227 has finished for PR 5209 at commit
|
Test PASSed. |
private val checkTimeoutIntervalMs = sc.conf.getLong("spark.network.timeoutInterval", | ||
sc.conf.getLong("spark.storage.blockManagerTimeoutIntervalMs", 60)) * 1000 | ||
|
||
// `spark.network.timeout` use `seconds`, |
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.
Minor nits on the comments:
- I wouldn't use backticks here because it's not md-formatted.
- "use" -> "uses"
- "while" should be on line above
Had some nits on the comments. Otherwise this LGTM. |
Fixed the docs |
Test build #29288 has started for PR 5209 at commit
|
Test build #29288 has finished for PR 5209 at commit
|
Test PASSed. |
The current reading logic of
executorTimeoutMs
is:So if
spark.storage.blockManagerSlaveTimeoutMs
is 10000 andspark.network.timeout
is not set, executorTimeoutMs will be 10000 * 1000. But the correct value should have been 10000.checkTimeoutIntervalMs
has the same issue.This PR fixes them.