Github user squito commented on a diff in the pull request:
https://github.com/apache/spark/pull/5741#discussion_r29333960
--- Diff: core/src/main/scala/org/apache/spark/util/AkkaUtils.scala ---
@@ -30,6 +32,48 @@ import org.apache.log4j.{Level, Logger}
import org.apache.spark.{Logging, SecurityManager, SparkConf, SparkEnv,
SparkException}
/**
+ * Binds a timeout to a configuration property so that a thrown akka
timeout exception can be
+ * traced back to the originating value. The main constructor takes a
generic timeout and
+ * description while the auxilary constructor uses a specific property
defined in the
+ * configuration.
+ * @param timeout_duration timeout duration in milliseconds
+ * @param timeout_description description to be displayed in a timeout
exception
+ */
+class ConfiguredTimeout(timeout_duration: FiniteDuration,
timeout_description: String = null) {
+
+ /**
+ * Specialized constructor to lookup the timeout property in the
configuration and construct
+ * a FiniteDuration timeout with the property key as the description
+ * @param conf configuration properties containing the timeout
+ * @param timeout_prop property key for the timeout
+ */
+ def this(conf: SparkConf, timeout_prop: String) = {
+ this(FiniteDuration(conf.getInt(timeout_prop, -1), "millis"),
timeout_prop)
+ require(timeout_duration.toMillis >= 0, "invalid property string: " +
timeout_prop)
+ }
+
+ val timeout = timeout_duration
+ val description = timeout_description
+}
+
+object ConfiguredTimeout {
+
+ /**
+ * Implicit conversion to allow for a simple FiniteDuration timeout to
be used instead of a
+ * ConfiguredTimeout when the description is not needed.
+ * @param timeout_duration timeout duration in milliseconds
+ * @return ConfiguredTimeout object
+ */
+ implicit def toConfiguredTimeout(timeout_duration: FiniteDuration):
ConfiguredTimeout =
+ new ConfiguredTimeout(timeout_duration)
--- End diff --
I don't think we want an implicit conversion here. The point is to *force*
you to give a `ConfiguredTimeout` when you call `AkkaUtils.askWithReply`.
That's to make sure that we get the better error messages consistently.
of course, this means you've got more work to do, tracking down each use of
`askWithReply`, and putting the right `ConfiguredTimeout`. Its possible there
are some cases which don't have a timeout that is tied to a configuration
parameter, so maybe this won't work. (Or maybe we should change those cases so
they are configurable.)
Also, as cool as implicits are, they end up making the code significantly
harder to read, so we try to avoid them. (eg., see
http://twitter.github.io/effectivescala/#Types and Generics-Implicits)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
