turcsanyip commented on code in PR #7326:
URL: https://github.com/apache/nifi/pull/7326#discussion_r1214438375
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/processor/SimpleProcessLogger.java:
##########
@@ -91,7 +91,8 @@ public void warn(final String msg, final Object[] os, final
Throwable t) {
logger.warn(componentMessage, arguments);
logRepository.addLogMessage(LogLevel.WARN, componentMessage,
arguments);
} else {
- logger.warn(componentMessage, arguments, t);
+ final Object[] argumentsWithThrowable =
insertThrowable(arguments, t);
+ logger.warn(componentMessage,
setFormattedThrowable(argumentsWithThrowable, t));
Review Comment:
`Throwable.toString()` will not be present in the formatted log message if
there is no `{}` placeholder for it (that is all `{}` placeholders are covered
by explicit arguments passed in by the caller) so it would not cause an issue.
I think `setFormattedThrowable()` was a convenience addition for log
messages ending with `due to {}`. In this case the caller does not need to pass
`t.toString()` explicitly but it can be derived from the `Throwable` parameter.
On the other hand, the callers in the current codebase always pass the
`Throwable` argument explicitly for the `{}` substitution too and therefore we
can omit the `setFormattedThrowable()` call.
The main point of this PR to fix the regular arguments and not to focus on
the Throwable / `due to {}` clause so I will remove `setFormattedThrowable()`.
--
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: issues-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
