AMashenkov commented on a change in pull request #217:
URL: https://github.com/apache/ignite-3/pull/217#discussion_r673815302
##########
File path: modules/core/src/main/java/org/apache/ignite/lang/IgniteLogger.java
##########
@@ -195,7 +201,12 @@ private void logInternal(Level level, String msg,
Object... params) {
if (!log.isLoggable(level))
return;
- log.log(level, LoggerMessageHelper.arrayFormat(msg, params));
+ Throwable throwable =
LoggerMessageHelper.getThrowableCandidate(params);
+
+ if (throwable != null)
+ log.log(level, LoggerMessageHelper.arrayFormat(msg,
LoggerMessageHelper.trimmedCopy(params)), throwable);
+ else
+ log.log(level, LoggerMessageHelper.arrayFormat(msg, params));
Review comment:
From my side, this part has incorrect logic. The code is not broken, but
the idea is wrong.
Using vararg for things of a different kind is a bad practice.
I believe method contract MUST declate it explicitly like in log(Level,
String, Throwable).
Lets me explain:
1. Usability. Here and in any other logger, "params" argument is a list of
parameters for a message template.
Vararg itself is ok here while it contains only params.
Generally, vararg is already hard to use, because it is very easy to miss
one or write the extra one.
Throwable as a last optional argument makes things ever worse.
What if the user passes Throwable as a param? or passes the extra argument?
2. Compatibility. Slf4J hadn't supported this case. So, no compatibility is
broken.
3. Codestyle. Having an implicit argument is a bad style.
Why you don't declare log(Object... args) and cast first arg to Level,
second to String?
This top-level public class, which is widely used in the project, shouldn't
push developers for writing uncertain code.
4. Performance. If you bother with extra Supplier<String> object creation,
then vararg is actually array and extra object (array) will be created anyway +
jvm will need to fill the array.
So, you win nothing here and maybe even loose.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
