cnauroth commented on code in PR #49858:
URL: https://github.com/apache/spark/pull/49858#discussion_r1949906078
##########
core/src/main/scala/org/apache/spark/util/Utils.scala:
##########
@@ -3151,9 +3152,31 @@ private[spark] object Utils
}
}
-private[util] object CallerContext extends Logging {
- val callerContextEnabled: Boolean =
- SparkHadoopUtil.get.conf.getBoolean("hadoop.caller.context.enabled", false)
+private[spark] object CallerContext extends Logging {
+ var callerContextEnabled: Boolean = SparkHadoopUtil.get.conf.getBoolean(
+ HADOOP_CALLER_CONTEXT_ENABLED_KEY, HADOOP_CALLER_CONTEXT_ENABLED_DEFAULT)
Review Comment:
No problem, I can definitely do that. :)
Can you please help me understand the reasoning though? Does the Spark
codebase prefer not to reference Hadoop's configuration constants?
##########
core/src/main/scala/org/apache/spark/util/Utils.scala:
##########
@@ -3151,9 +3152,31 @@ private[spark] object Utils
}
}
-private[util] object CallerContext extends Logging {
- val callerContextEnabled: Boolean =
- SparkHadoopUtil.get.conf.getBoolean("hadoop.caller.context.enabled", false)
+private[spark] object CallerContext extends Logging {
+ var callerContextEnabled: Boolean = SparkHadoopUtil.get.conf.getBoolean(
+ HADOOP_CALLER_CONTEXT_ENABLED_KEY, HADOOP_CALLER_CONTEXT_ENABLED_DEFAULT)
+
+ /**
+ * Run a function with the caller context enabled flag overridden to the
given value. Normally,
+ * the flag is initialized from Hadoop configuration once per JVM process
and never changed. This
+ * function is intended only for use by tests that specifically need to
control whether the caller
+ * context is enabled. The configured value will be restored after execution
of the function.
+ *
+ * @param enabled override value for the caller context enabled flag
+ * @param func function to run
+ *
+ * VisibleForTesting
+ */
+ def withCallerContextEnabled[T](enabled: Boolean)(func: => T): T = {
Review Comment:
There is no benefit to main code, other than keeping all access to
`callerContextEnabled` in the same object. Otherwise, people reading the code
might be confused about why it's `var` instead of `val`.
If you prefer, I can just leave a comment on why it's `var` and move this to
a new `CallerContextTestUtils` object under test (or some existing test utils
file if you have another suggestion).
I would potentially like to reuse this in tests covering other usage of
caller context, which is why I didn't put it directly in
`FsHistoryProviderSuite`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
