[GitHub] [nifi] turcsanyip commented on a diff in pull request #7326: NIFI-11628: Fixed Object[] + Throwable argument substitution in Simpl…

["turcsanyip (via GitHub)" <gi...@apache.org>]

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-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org