AMashenkov commented on a change in pull request #217:
URL: https://github.com/apache/ignite-3/pull/217#discussion_r673855681
##########
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:
> I couldn't agree with you. The only reason why I decided to support
such methods is that Slf4J supports throwable as the last param [1]
log() methods with vararg in org.slf4j.Logger interface has no Throwable
mentioned.
It's sad that they have a bad API. Do we want to be like them?
However, they refer to a FAQ page on their website [1], where said your way
is possible.
They said that _If the exception is not the last argument, it will be
treated as a plain object and its stack trace will NOT be printed._ I think it
is critical to have a thread dump when smth goes wrong despite developer's
mistakes.
But their next _"However, such situations should not occur in practice."_,
sorry, is bullshit.
> it is widely used in Jraft module (for example [2])
We already incorporated JRaft code into Ignite module, so the correct way is
to fix this.
Or at least, create a separate JRaftLogger (as a wrapper) in JRaft module
rather than go error-prone way.
This just means, SLF4J is not the ideal we have to look at when making
long-term decisions.
[1] http://www.slf4j.org/faq.html#paramException
##########
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:
> I couldn't agree with you. The only reason why I decided to support
such methods is that Slf4J supports throwable as the last param [1]
log() methods with vararg in org.slf4j.Logger interface has no Throwable
mentioned.
It's sad that they have a bad API. Do we want to be like them?
However, they refer to a FAQ page on their website [1], where said your way
is possible.
They said that _If the exception is not the last argument, it will be
treated as a plain object and its stack trace will NOT be printed._ I think it
is critical to have a thread dump when smth goes wrong despite developer's
mistakes.
But their next _"However, such situations should not occur in practice."_,
sorry, is bullshit.
> it is widely used in Jraft module (for example [2])
We already incorporated JRaft code into Ignite module, so the correct way is
to fix this.
Or at least, create a separate JRaftLogger (as a wrapper) in JRaft module
rather than go error-prone way.
This just means, SLF4J is not the ideal we have to look at when making
long-term decisions.
[1] http://www.slf4j.org/faq.html#paramException
--
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]
