-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-6980] [CORE] Akka timeout exceptions indicate which conf controls them (RPC Layer) #6205
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
97523e0
78a2c0a
5b59a44
49f9f04
23d2f26
a294569
f74064d
0ee5642
4be3a8d
b7fb99f
c07d05c
235919b
2f94095
1607a5f
4351c48
7774d56
995d196
d3754d1
08f5afc
1b9beab
2206b4d
1517721
1394de6
c6cfd33
b05d449
fa6ed82
fadaf6f
218aa50
039afed
be11c4e
7636189
3a168c7
3d8b1ff
287059a
7f4d78e
6a1c50d
4e89c75
dbd5f73
7bb70f1
06afa53
46c8d48
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -197,19 +197,16 @@ private[rpc] class RpcTimeoutException(message: String, cause: TimeoutException) | |
| * Associates a timeout with a description so that a when a TimeoutException occurs, additional | ||
| * context about the timeout can be amended to the exception message. | ||
| * @param timeout timeout duration in seconds | ||
| * @param description description to be displayed in a timeout exception | ||
| * @param conf the configuration parameter that controls this timeout | ||
| */ | ||
| private[spark] class RpcTimeout(timeout: FiniteDuration, description: String) { | ||
| private[spark] class RpcTimeout(timeout: FiniteDuration, val conf: String) { | ||
|
|
||
| /** Get the timeout duration */ | ||
| def duration: FiniteDuration = timeout | ||
|
|
||
| /** Get the message associated with this timeout */ | ||
| def message: String = description | ||
|
|
||
| /** Amends the standard message of TimeoutException to include the description */ | ||
| private def createRpcTimeoutException(te: TimeoutException): RpcTimeoutException = { | ||
| new RpcTimeoutException(te.getMessage() + " " + description, te) | ||
| new RpcTimeoutException(te.getMessage() + ". This timeout is controlled by " + conf, te) | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -244,8 +241,6 @@ private[spark] class RpcTimeout(timeout: FiniteDuration, description: String) { | |
|
|
||
| private[spark] object RpcTimeout { | ||
|
|
||
| private[this] val messagePrefix = "This timeout is controlled by " | ||
|
|
||
| /** | ||
| * Lookup the timeout property in the configuration and create | ||
| * a RpcTimeout with the property key in the description. | ||
|
|
@@ -255,7 +250,7 @@ private[spark] object RpcTimeout { | |
| */ | ||
| def apply(conf: SparkConf, timeoutProp: String): RpcTimeout = { | ||
| val timeout = { conf.getTimeAsSeconds(timeoutProp) seconds } | ||
| new RpcTimeout(timeout, messagePrefix + timeoutProp) | ||
| new RpcTimeout(timeout, timeoutProp) | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -268,7 +263,7 @@ private[spark] object RpcTimeout { | |
| */ | ||
| def apply(conf: SparkConf, timeoutProp: String, defaultValue: String): RpcTimeout = { | ||
| val timeout = { conf.getTimeAsSeconds(timeoutProp, defaultValue) seconds } | ||
|
Member
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. Should this be
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. as mentioned above, I don't think so -- lets stick w/ the current behavior of using |
||
| new RpcTimeout(timeout, messagePrefix + timeoutProp) | ||
| new RpcTimeout(timeout, timeoutProp) | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -292,6 +287,6 @@ private[spark] object RpcTimeout { | |
| } | ||
| val finalProp = foundProp.getOrElse(timeoutPropList.head, defaultValue) | ||
|
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.
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. Never mind. Looks it's long too. |
||
| val timeout = { Utils.timeStringAsSeconds(finalProp._2) seconds } | ||
| new RpcTimeout(timeout, messagePrefix + finalProp._1) | ||
| new RpcTimeout(timeout, finalProp._1) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -569,7 +569,7 @@ abstract class RpcEnvSuite extends SparkFunSuite with BeforeAndAfterAll { | |
| val defaultDurationSeconds = 1 | ||
| val rt3 = RpcTimeout(conf, Seq(defaultProp), defaultDurationSeconds.toString + "s") | ||
| assert( defaultDurationSeconds === rt3.duration.toSeconds ) | ||
| assert( rt3.message.contains(defaultProp) ) | ||
| assert( rt3.conf.contains(defaultProp) ) | ||
|
|
||
| // Try to construct RpcTimeout with an unconfigured property | ||
|
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. add a case for using the default value:
Member
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. done |
||
| intercept[Throwable] { | ||
|
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. Could you change |
||
|
|
||
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.
Is this necessary or should we just use
val duration: FiniteDurationin the constructor likeval conf: String?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.
yeah I think you should just change the constructor
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.
Sure, that sounds good. One other minor question @squito , the constructor has the parameter
conf: Stringwhileapplyhasconf: SparkConfandtimeoutProp: Stringwhich seems a little unclear. I think it might be better to stay consistent and have the constructor saytimeoutPropinstead ofconf, like thiswhat do you think?
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.
yes, I agree that would be more consistent, good point