Given the sink we are adding here has some quirks to its behavior - new directory every hour etc., the class name FileSystemSink seems too simple. Can we capture more of the behavior in the name?
How about RollingFileSystemSink?
checkAppend: If appending throws an IOE that is not because of not being supported, should we allow appending? I would think not.
Actually, yes. If the operation isn't supported, the method contains nothing but the exception throw. If we see anything other than that exception, the operation is supported and just not working at the moment. Could be transitive, could be persistent. Either way, the sink should keep trying.
rollLogDirIfNeeded: For readability, should we split it into two ifs - the first is when the directories don't match. Also, the comment in the method is wrongly indented and slightly confusing.
Hmmm... I'm not sure that actually improves readability. I really dislike chained logic like that. I find it hard to follow.
I'll take this point as, "gee that's a long/busy method..." I have a different idea for how to clean it up.
putMetrics: When throwing MetricsException, no need for a new line between setting the message and actually throwing the exception. Also, should just have a method that takes a message (String) and throws an exception if ignore error is not turned on. The only downside would be the intern objects for the strings here.
Fair enough. On the blank line, I always put a blank line before a throw because it's kinda a big deal and should jump out at the reader. If it really annoys you, tell me. Otherwise I'm leaving it in.