[GitHub] [spark] JoshRosen commented on a diff in pull request #39763: [WIP][CORE][SPARK-42204] Remove redundant logging of TaskMetrics internal accumulators in event logs

["JoshRosen (via GitHub)" <gi...@apache.org>]

JoshRosen commented on code in PR #39763:
URL: https://github.com/apache/spark/pull/39763#discussion_r1088589402


##########
core/src/main/scala/org/apache/spark/util/JsonProtocol.scala:
##########
@@ -39,6 +40,16 @@ import org.apache.spark.scheduler.cluster.ExecutorInfo
 import org.apache.spark.storage._
 import org.apache.spark.util.Utils.weakIntern
 
+/**
+ * Helper class for passing configuration options to JsonProtocol.
+ * We use this instead of passing SparkConf directly because it lets us avoid
+ * repeated re-parsing of configuration values on each read.
+ */
+private[spark] class JsonProtocolOptions(conf: SparkConf) {

Review Comment:
   This pattern is a bit weird, so I'd like to explain it in greater length here:
   
   To date, JsonProtocol has just been a big object with no configuration options of its own. All other event logging configurations take effect at higher levels, such as EventLoggingListener. We cannot implement this particular change at those levels, though: we shouldn't mutate the mutable TaskInfo or StageInfo objects referenced by the listener events and adding `copy()` methods to them would be expensive.
   
   JsonProtocol is currently implemented as a global object, so there's no straightforward way to make it easily configurable and testable. If I did something like "get the active SparkContext's configuration and read the configuration value" on each call then this would destroy performance because configuration reading is not cheap.
   
   One approach would be to refactor this so that JsonProtocol is no longer a singleton: we could give it a proper constructor which accepts a SparkConf. That's a much larger refactoring than I want to undertake right now, though, and it would involve significant updates in a bunch of test code.
   
   The approach taken here is to thread through an `JsonProtocolOptions` class which holds the configuration values. Right now it only holds a single value, but this is done as future-proofing in case we add additional configurability. The lone caller in EventLoggingListener constructs the `JsonProtocolOptions` using SparkConf, so the configuration is only read from the conf once (avoiding conf reader performance overheads).
   
   This is a more C-style approach than the usual OOP approach that we generally take, but I think it's okay in this limited internal context.



-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org