migrate to SafeLogging in org.apache.cassandra.streaming
#592
Conversation
| @@ -107,7 +109,7 @@ public void cleanup() | |||
| catch (Exception e) | |||
| { | |||
| JVMStabilityInspector.inspectThrowable(e); | |||
| logger.warn("failed to delete a potentially stale sstable {}", file); | |||
| logger.warn("failed to delete a potentially stale sstable {}", SafeArg.of("file", file)); | |||
There was a problem hiding this comment.
is the full file safeloggable ?
There was a problem hiding this comment.
from reading the above code, it appears that each line of the lockfile is a filename, called file that is passed to Descriptor.fromFilename.
Assuming that all sstable filenames are safe, then this log statement is safe.
There was a problem hiding this comment.
switched to UnsafeArg
| if (logger.isDebugEnabled()) | ||
| logger.debug("[Stream #{}] Start streaming file {} to {}, repairedAt = {}, totalSize = {}", | ||
| SafeArg.of("planId", session.planId()), | ||
| SafeArg.of("file", sstable.getFilename()), |
There was a problem hiding this comment.
also would like to doublecheck this is safe
There was a problem hiding this comment.
sstable file names are safe right?
There was a problem hiding this comment.
yeah the filename is safe as long as it doesn't include the absolute path
There was a problem hiding this comment.
I feel like this could include the absolute path, so I'll mark it (and equivalents) as unsafe
There was a problem hiding this comment.
switched to UnsafeArg
| @@ -85,7 +88,7 @@ this class will not need to clean up tmp files (on restart), CassandraDaemon doe | |||
| } | |||
| catch (IOException e) | |||
| { | |||
| logger.warn(String.format("Could not create lockfile %s for stream session, nothing to worry too much about", lockfile), e); | |||
| logger.warn("Could not create lockfile {} for stream session, nothing to worry too much about", SafeArg.of("lockfile", lockfile.toString()), e); | |||
There was a problem hiding this comment.
file marked as safe, inconsistent with the other files that i believe we've decided are unsafe
| if (logger.isDebugEnabled()) | ||
| logger.debug("[Stream #{}] Finished streaming file {} to {}, bytesTransferred = {}, totalSize = {}", | ||
| SafeArg.of("planId", session.planId()), | ||
| SafeArg.of("file", sstable.getFilename()), |
| if (logger.isDebugEnabled()) | ||
| logger.debug("[Stream #{}] Start streaming file {} to {}, repairedAt = {}, totalSize = {}", | ||
| SafeArg.of("planId", session.planId()), | ||
| SafeArg.of("file", sstable.getFilename()), |
|
should be fixed now |
Mark logging arguments as Safe or Unsafe in
org.apache.cassandra.streaming.This PR also wraps all calls to
logger.debug()in anif (logger.isDebugEnabled())check.For reviewers, please confirm that you agree with my choices for SafeArgs.
Everything appears to be safe except
partitionKeyinStreamReaderInteresting is the
messagevariable inConnectionHandler. I validated that these are safe by checking thetoStringmethods of all subclasses ofStreamMessage, but would appreciate a second look at this.