Cleanup recovery code#6261
Conversation
Removes ServerContext from being passed around since a tserver object is already set within the logger. Also removed a reference to a VolumeManager that was not being used.
dlmarion
left a comment
dlmarion
left a comment
There was a problem hiding this comment.
I think the changes are good, but I'm not sure we want to log stack traces on these warnings.
| // ignore | ||
| } catch (IOException | RuntimeException ex) { | ||
| log.error("Unable to cleanly close log " + currentLog.getLogEntry() + ": " + ex, ex); | ||
| log.error("Unable to cleanly close log {}: {}", currentLog.getLogEntry(), ex, ex); |
There was a problem hiding this comment.
I think the first ex can just be removed. This is going to log the entire stack trace.
There was a problem hiding this comment.
I think it's okay to log the entire stack trace. I think getting rid of the one that inlines it as a toString is the unnecessary one. For an error, I'd like to see the stack trace to troubleshoot. It can be very useful.
| return HostAndPort.fromString(managers.iterator().next().toHostPortString()); | ||
| } catch (Exception e) { | ||
| log.warn("Failed to obtain manager host " + e); | ||
| log.warn("Failed to obtain manager host", e); |
There was a problem hiding this comment.
If this prints the entire stack trace, then I think this is a behavior change. Previously this would print Exception.toString, not the entire stack trace. Suggest adding a placeholder to the message {}.
There was a problem hiding this comment.
I think showing the stack trace is fine for a warning. We have no expectation that log formats are going to be the same across versions, and it's better to have a stack trace when troubleshooting than be without it.
| return new TServerInstance(getAdvertiseAddress().toString(), lockSessionId); | ||
| } catch (Exception ex) { | ||
| log.warn("Unable to read session from tablet server lock" + ex); | ||
| log.warn("Unable to read session from tablet server lock", ex); |
There was a problem hiding this comment.
Same comment here about behavior change.
3 participants
Fixes log message formats.
Removes serverContext from method signatures where they aren't needed.
Removes unused VolumeManager from method signature.