markap14 commented on a change in pull request #4952:
URL: https://github.com/apache/nifi/pull/4952#discussion_r721573245
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/logging/repository/StandardLogRepository.java
##########
@@ -27,14 +29,16 @@
import java.util.ArrayList;
import java.util.Collection;
+import java.util.EnumMap;
import java.util.HashMap;
import java.util.Map;
+import java.util.Optional;
import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantReadWriteLock;
public class StandardLogRepository implements LogRepository {
- private final Map<LogLevel, Collection<LogObserver>> observers = new
HashMap<>();
+ private final EnumMap<LogLevel, Collection<LogObserver>> observers = new
EnumMap<>(LogLevel.class);
Review comment:
Good idea to use an `EnumMap` here instead of a `HashMap`. Should use a
declaration of `Map`, though.
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/logging/repository/StandardLogRepository.java
##########
@@ -46,43 +50,60 @@
private volatile ComponentLog componentLogger;
@Override
- public void addLogMessage(final LogLevel level, final String message) {
- addLogMessage(level, message, (Throwable) null);
- }
-
- @Override
- public void addLogMessage(final LogLevel level, final String message,
final Throwable t) {
- final LogMessage logMessage = new
LogMessage(System.currentTimeMillis(), level, message, t);
+ public void addLogMessage(LogMessage logMessage) {
+ LogLevel logLevel = logMessage.getLogLevel();
- final Collection<LogObserver> logObservers = observers.get(level);
+ final Collection<LogObserver> logObservers = observers.get(logLevel);
if (logObservers != null) {
for (LogObserver observer : logObservers) {
try {
observer.onLogMessage(logMessage);
- } catch (final Throwable observerThrowable) {
+ } catch (final Exception observerThrowable) {
logger.error("Failed to pass log message to Observer {}
due to {}", observer, observerThrowable.toString());
}
}
}
+
}
@Override
public void addLogMessage(final LogLevel level, final String format, final
Object[] params) {
replaceThrowablesWithMessage(params);
+ final Optional<String> flowFileUUID =
getFlowFileUUIDFromObjects(params);
final String formattedMessage = MessageFormatter.arrayFormat(format,
params).getMessage();
- addLogMessage(level, formattedMessage);
+ final LogMessage logMessage = new
LogMessage.Builder(System.currentTimeMillis(), level)
+ .message(formattedMessage)
+ .flowFileUUID(flowFileUUID.orElse(null))
+ .createLogMessage();
+ addLogMessage(logMessage);
}
@Override
public void addLogMessage(final LogLevel level, final String format, final
Object[] params, final Throwable t) {
replaceThrowablesWithMessage(params);
+ final Optional<String> flowFileUUID =
getFlowFileUUIDFromObjects(params);
final String formattedMessage = MessageFormatter.arrayFormat(format,
params, t).getMessage();
- addLogMessage(level, formattedMessage, t);
+ final LogMessage logMessage = new
LogMessage.Builder(System.currentTimeMillis(), level)
+ .message(formattedMessage)
+ .throwable(t)
+ .flowFileUUID(flowFileUUID.orElse(null))
+ .createLogMessage();
+ addLogMessage(logMessage);
+ }
+
+ private Optional<String> getFlowFileUUIDFromObjects(Object[] params) {
Review comment:
Might make sense to name this method `getFirstFlowFileUuidFromObjects`
for clarity - it's definitely possible that there could be multiple FlowFile
objects. Or, it might make more sense to return `Optional.empty()` if there's
more than one FlowFile, as we really don't know which FlowFile the log message
is really focused on. For example, the log message `Created {} from {} but
failed to send {}` may take 3 FlowFiles are arguments. The first one would be
the most important FlowFile there. But for a message like `Created 3 FlowFiles
from {} but {} could not be sent`, the most important FlowFile for the log
message is the second. Can't really know which is the main theme of the log
message.
##########
File path:
nifi-nar-bundles/nifi-site-to-site-reporting-bundle/nifi-site-to-site-reporting-task/src/main/java/org/apache/nifi/reporting/SiteToSiteBulletinReportingTask.java
##########
@@ -195,6 +192,7 @@ private JsonObject serialize(final JsonBuilderFactory
factory, final JsonObjectB
addField(builder, "bulletinSourceName", bulletin.getSourceName(),
allowNullValues);
addField(builder, "bulletinSourceType", bulletin.getSourceType() ==
null ? null : bulletin.getSourceType().name(), allowNullValues);
addField(builder, "bulletinTimestamp",
df.format(bulletin.getTimestamp()), allowNullValues);
+ addField(builder, "bulletinFlowFileUUID", bulletin.getFlowFileUUID(),
allowNullValues);
Review comment:
Should probably also change this to `bulletinFlowFileUuid`
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core-api/src/main/java/org/apache/nifi/events/BulletinFactory.java
##########
@@ -27,30 +27,28 @@
private static final AtomicLong currentId = new AtomicLong(0);
+ private BulletinFactory() {
+ }
+
public static Bulletin createBulletin(final Connectable connectable, final
String category, final String severity, final String message) {
final ComponentType type;
Review comment:
Is a good idea to break that into its own method - would make sense then
to also collapse the declaration and assignment into a single statement as well
now. (I.e., `final ComponentType type = getComponentType(connectable);`)
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-nar-utils/src/main/java/org/apache/nifi/mock/MockComponentLogger.java
##########
@@ -116,9 +114,8 @@ public void info(String msg) {
@Override
public void info(String msg, Object[] os, Throwable t) {
- logger.trace(msg, os);
- logger.trace("", t);
-
+ logger.info(msg, os);
+ logger.info("", t);
Review comment:
Wow. That's a good catch :)
--
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]
