exceptionfactory commented on a change in pull request #5195:
URL: https://github.com/apache/nifi/pull/5195#discussion_r687294900
##########
File path:
nifi-commons/nifi-properties/src/main/java/org/apache/nifi/util/NiFiProperties.java
##########
@@ -303,6 +303,18 @@
public static final String MONITOR_LONG_RUNNING_TASK_SCHEDULE =
"nifi.monitor.long.running.task.schedule";
public static final String MONITOR_LONG_RUNNING_TASK_THRESHOLD =
"nifi.monitor.long.running.task.threshold";
+ // automatic diagnostic properties
+ public static final String DIAGNOSTICS_ON_SHUTDOWN_ENABLED =
"nifi.diagnostics.on.shutdown.enabled";
+ public static final String DIAGNOSTICS_ON_SHUTDOWN_VERBOSE =
"nifi.diagnostics.on.shutdown.verbose";
+ public static final String DIAGNOSTICS_ON_SHUTDOWN_DIRECTORY =
"nifi.diagnostics.on.shutdown.directory";
+ public static final String DIAGNOSTICS_ON_SHUTDOWN_MAX_FILE_COUNT =
"nifi.diagnostics.on.shutdown.max.filecount";
+ public static final String
DIAGNOSTICS_ON_SHUTDOWN_MAX_DIRECTORY_SIZE_IN_BYTES =
"nifi.diagnostics.on.shutdown.max.directory.size.in.bytes";
Review comment:
The `DataUnit` class in `nifi-api` supports parsing standard file sizes.
Other file size properties use that class for parsing the property value from a
string to bytes, so the `.in.bytes` suffix should not be necessary.
##########
File path: nifi-docs/src/main/asciidoc/administration-guide.adoc
##########
@@ -4251,3 +4257,21 @@ Example configuration:
nifi.nar.library.provider.hdfs2.implementation=org.apache.nifi.nar.hadoop.HDFSNarProvider
nifi.nar.library.provider.hdfs2.resources=/etc/hadoop/core-site.xml
nifi.nar.library.provider.hdfs2.source.directory=/other/dir/for/customNars
+
+= NiFi diagnostics
+
+It is possible to run diagnostics on NiFi with
+
+```
+$ ./bin/nifi.sh --diagnostic --verbose <dumpfilePath>
+```
+
+During the diagnostic, NiFi sends a request to an already running NiFi
instance, which collects information about clusters,
+components, part of the configuration, memory usage, etc., and writes it to
the specified file or, failing that, to the logs.
+
+The verbose switch is optional and can be used to control the level of
diagnostic detail. In case of a missing dump file path, NiFi writes the
diagnostics information to the bootstrap.log file.
+
+== Automatic diagnostics at shutdown/restart
Review comment:
This should be a third-level header:
```suggestion
=== Automatic diagnostics on restart and shutdown
```
##########
File path: nifi-docs/src/main/asciidoc/administration-guide.adoc
##########
@@ -2631,6 +2631,12 @@ The Encrypt-Config Tool can be used to specify the root
key, encrypt sensitive v
|`nifi.died.notification.services`|This property is a comma-separated list of
Notification Service identifiers that correspond to the Notification Services
defined in the `notification.services.file`
property. The services with the specified identifiers will be used to notify
their
configured recipients if the bootstrap
determines that NiFi has unexpectedly died.
+|`nifi.diag.allowed`|(true or false) This property decides whether to run NiFi
diagnostics before shutting down.
+|`nifi.diag.filecount.max`|This property specifies the maximum permitted
number of diagnostic files. If the limit is exceeded, the oldest files are
deleted.
+|`nifi.diag.size.max.byte`|This property specifies the maximum permitted size
of the diagnostics directory in bytes. If the limit is exceeded, the oldest
files are deleted.
+|`nifi.diag.dir`|This property specifies the location of the NiFi diagnostics
directory.
+|`nifi.diag.verbose`|(true or false) This property decides whether to run NiFi
diagnostics in verbose mode.
Review comment:
This documentation needs to be updated to match the new property names.
##########
File path: nifi-docs/src/main/asciidoc/administration-guide.adoc
##########
@@ -4251,3 +4257,21 @@ Example configuration:
nifi.nar.library.provider.hdfs2.implementation=org.apache.nifi.nar.hadoop.HDFSNarProvider
nifi.nar.library.provider.hdfs2.resources=/etc/hadoop/core-site.xml
nifi.nar.library.provider.hdfs2.source.directory=/other/dir/for/customNars
+
+= NiFi diagnostics
Review comment:
This should be a second-level header:
```suggestion
== NiFi Diagnostics
```
##########
File path:
nifi-commons/nifi-utils/src/main/java/org/apache/nifi/util/file/FileUtils.java
##########
@@ -575,11 +577,43 @@ public static long getContainerCapacity(final Path path) {
/**
* Returns the free capacity for a given path
+ *
* @param path path
* @return usable space
*/
public static long getContainerUsableSpace(final Path path) {
return path.toFile().getUsableSpace();
}
+ /**
+ * Returns the size for a given directory.
+ * @param path directory path
+ * @param logger logger
+ * @return the size in bytes
+ */
+ public static long getDirectorySize(Path path, final Logger logger) {
+ long size = 0;
+ try (Stream<Path> walk = Files.walk(path)) {
+ size = walk
+ .filter(Files::isRegularFile)
+ .mapToLong(getFileSizeByPathFunction(logger))
+ .sum();
+
+ } catch (IOException e) {
+ logger.error("IO exception occured while tried to get the size of
the directory {}", path, e);
Review comment:
Recommend adjusting the wording:
```suggestion
logger.error("Directory [{}] size calculation failed", path, e);
```
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-runtime/src/main/java/org/apache/nifi/NiFi.java
##########
@@ -201,40 +204,71 @@ protected void initLogging() {
private static ClassLoader createBootstrapClassLoader() {
//Get list of files in bootstrap folder
final List<URL> urls = new ArrayList<>();
- try {
- Files.list(Paths.get("lib/bootstrap")).forEach(p -> {
+ try (final Stream<Path> files =
Files.list(Paths.get("lib/bootstrap"))) {
+ files.forEach(p -> {
try {
urls.add(p.toUri().toURL());
} catch (final MalformedURLException mef) {
- LOGGER.warn("Unable to load " + p.getFileName() + " due to
" + mef, mef);
+ logger.warn("Unable to load " + p.getFileName() + " due to
" + mef, mef);
}
});
} catch (IOException ioe) {
- LOGGER.warn("Unable to access lib/bootstrap to create bootstrap
classloader", ioe);
+ logger.warn("Unable to access lib/bootstrap to create bootstrap
classloader", ioe);
}
//Create the bootstrap classloader
return new URLClassLoader(urls.toArray(new URL[0]),
Thread.currentThread().getContextClassLoader());
}
- public void shutdownHook(boolean isReload) {
+ public void shutdownHook(final boolean isReload) {
try {
+ runAutomaticDiagnostics();
shutdown();
} catch (final Throwable t) {
- LOGGER.warn("Problem occurred ensuring Jetty web server was
properly terminated due to " + t);
+ logger.warn("Problem occurred ensuring Jetty web server was
properly terminated due to ", t);
+ }
+ }
+
+ private void runAutomaticDiagnostics() throws IOException {
+ final String diagnosticDirectoryPath =
properties.getDiagnosticsOnShutdownDirectory();
+ if (properties.isDiagnosticsOnShutdownEnabled()) {
+ final boolean isCreated =
DiagnosticUtils.createDiagnosticDirectory(diagnosticDirectoryPath);
+ if (isCreated) {
+ logger.debug("Diagnostic directory has successfully been
created.");
+ }
+ if (DiagnosticUtils.isFileCountExceeded(diagnosticDirectoryPath,
properties.getDiagnosticsOnShutdownMaxFileCount())) {
+ final Path oldestFile =
DiagnosticUtils.getOldestFile(diagnosticDirectoryPath);
+ Files.delete(oldestFile);
+ }
+ final DateTimeFormatter formatter =
DateTimeFormatter.ofPattern("yyyy-MM-dd_HH-mm-ss");
Review comment:
`DateTimeFormatter` is thread-safe, so this instance could be moved to a
static class member.
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-resources/src/main/resources/conf/nifi.properties
##########
@@ -327,3 +327,23 @@
nifi.analytics.connection.model.score.threshold=${nifi.analytics.connection.mode
# runtime monitoring properties
nifi.monitor.long.running.task.schedule=
nifi.monitor.long.running.task.threshold=
+
+# Create automatic diagnostics when stopping/restarting NiFi.
+
+# Enable automatic diagnostic at shutdown.
+nifi.diagnostics.on.shutdown.enabled=false
+
+# Include verbose diagnostic information.
+nifi.diagnostics.on.shutdown.verbose=false
+
+# The location of the diagnostics folder.
+nifi.diagnostics.on.shutdown.directory=./diagnostics
+
+# The maximum number of files permitted in the directory. If the limit is
exceeded, the oldest files are deleted.
+nifi.diagnostics.on.shutdown.max.filecount=10
+
+# The diagnostics folder's maximum permitted size in bytes. If the limit is
exceeded, the oldest files are deleted.
+nifi.diagnostics.on.shutdown.max.directory.size.in.bytes=
Review comment:
It seems like we should have a sensible default value here as opposed to
letting the directory grow unbounded. Perhaps 100 MB?
##########
File path:
nifi-commons/nifi-properties/src/main/java/org/apache/nifi/util/NiFiProperties.java
##########
@@ -303,6 +303,18 @@
public static final String MONITOR_LONG_RUNNING_TASK_SCHEDULE =
"nifi.monitor.long.running.task.schedule";
public static final String MONITOR_LONG_RUNNING_TASK_THRESHOLD =
"nifi.monitor.long.running.task.threshold";
+ // automatic diagnostic properties
+ public static final String DIAGNOSTICS_ON_SHUTDOWN_ENABLED =
"nifi.diagnostics.on.shutdown.enabled";
+ public static final String DIAGNOSTICS_ON_SHUTDOWN_VERBOSE =
"nifi.diagnostics.on.shutdown.verbose";
+ public static final String DIAGNOSTICS_ON_SHUTDOWN_DIRECTORY =
"nifi.diagnostics.on.shutdown.directory";
+ public static final String DIAGNOSTICS_ON_SHUTDOWN_MAX_FILE_COUNT =
"nifi.diagnostics.on.shutdown.max.filecount";
+ public static final String
DIAGNOSTICS_ON_SHUTDOWN_MAX_DIRECTORY_SIZE_IN_BYTES =
"nifi.diagnostics.on.shutdown.max.directory.size.in.bytes";
+
+ // automatic diagnostic defaults
+ public static final String DEFAULT_DIAGNOSTICS_ON_SHUTDOWN_DIRECTORY =
"./diagnostic";
Review comment:
Recommend making the default directory plural:
```suggestion
public static final String DEFAULT_DIAGNOSTICS_ON_SHUTDOWN_DIRECTORY =
"./diagnostics";
```
--
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]
