add safelogging #425
add safelogging #425
Conversation
|
chatted with @mccheah , will modify this to use palantir safelogging so we can get unsafe args and also so we're not throwing when the unsafe log line accidentally gets called |
|
|
||
| package org.apache.spark.internal | ||
|
|
||
| trait SafeLogging extends Logging { |
There was a problem hiding this comment.
don't extend Logging. You want uses of logInfo et al to be compile-time errors (which you can then address post-merge upstream), rather than runtime.
| " executors. Not scaling up further.") | ||
| } else if (newlyCreatedExecutors.nonEmpty || currentPendingExecutors != 0) { | ||
| logDebug(s"Still waiting for ${newlyCreatedExecutors.size + currentPendingExecutors}" + | ||
| safeLogDebug(s"Still waiting for ${newlyCreatedExecutors.size + currentPendingExecutors}" + |
There was a problem hiding this comment.
Also rewrite all of these to replace the inline ${} with named SafeArg parameters. Otherwise we'd have to needlessly parse them out later
| } | ||
|
|
||
|
|
||
| // You can only safelog to SafeLogging logs, everything else is unsupported |
There was a problem hiding this comment.
just remove everything below this line, and don't extend Logging
| s"com.palantir.${this.getClass.getName.stripSuffix("$")}" | ||
| } | ||
|
|
||
| protected def safeLogInfo(msg: => String) { |
There was a problem hiding this comment.
have these take a msg: String, params: (=> Arg[_])*
Then pass msg, *params to the log.x method
There was a problem hiding this comment.
That will force the parameters to be safe-logging Arg and not other stuff.
And you can have an override that also takes a throwable after msg
| " executors to begin running before requesting for more executors. # of executors in" + | ||
| " pending status in the cluster: {currentPendingExecutors}. # of executors that we have" + | ||
| " created but we have not observed as being present in the cluster yet:" + | ||
| " ${newlyCreatedExecutors}.", |
| import org.apache.spark.deploy.k8s.Constants._ | ||
| import org.apache.spark.deploy.k8s.KubernetesConf | ||
| import org.apache.spark.internal.Logging | ||
| import org.apache.spark.logging.SafeLogging |
There was a problem hiding this comment.
maybe we should put this under internal too, to make it obvious it's not for outside consumption
| logWarning(s"Executor with id $execId was not detected in the Kubernetes" + | ||
| s" cluster after $podCreationTimeout milliseconds despite the fact that a" + | ||
| safeLogWarning("Executor with id {execId} was not detected in the Kubernetes" + | ||
| " cluster after {podCreationTimeout} milliseconds despite the fact that a" + |
There was a problem hiding this comment.
We shouldn't use {named} things inside the message, they won't be templated in by our logging infrastructure.
Better to make units clear in the Arg name e.g.
-SafeArg.of("execId", execId),
-SafeArg.of("podCreationTimeout", podCreationTimeout)
+SafeArg.of("executorId", execId),
+SafeArg.of("podCreationTimeoutMs", podCreationTimeout)and simplify the message. Something like
Executor was not detected in the Kubernetes cluster after timeout, despite the fact that a previous allocation attempt tried to create it. The executor may have been deleted but the application missed the deletion event.
core/pom.xml
Outdated
| </dependency> | ||
|
|
||
| <!-- | ||
| The following dependency is used for safe logging |
|
I am going to comment here again - please squash all merges |
## What changes were proposed in this pull request? Add SafeLogging (similar to #425) to more classes that can be useful: - k8s classes - CoarseGrainedSchedulerBackend - SparkContext - MemoryStore - Executor - CoarseGrainedExecutorBackend - TorrentBroadCast and Broadcast
Add SafeLogging (similar to #425) to more classes that can be useful: - k8s classes - CoarseGrainedSchedulerBackend - SparkContext - MemoryStore - Executor - CoarseGrainedExecutorBackend - TorrentBroadCast and Broadcast
@robert3005 is this what you were thinking of?