simonbence commented on a change in pull request #5356:
URL: https://github.com/apache/nifi/pull/5356#discussion_r718832358
##########
File path: nifi-bootstrap/src/main/java/org/apache/nifi/bootstrap/RunNiFi.java
##########
@@ -175,7 +179,9 @@ private static void printUsage() {
System.out.println("Status : Determine if there is a running instance
of Apache NiFi");
System.out.println("Dump : Write a Thread Dump to the file specified
by [options], or to the log if no file is given");
System.out.println("Diagnostics : Write diagnostic information to the
file specified by [options], or to the log if no file is given. The --verbose
flag may be provided as an option before " +
- "the filename, which may result in additional diagnostic
information being written.");
+ "the filename, which may result in additional diagnostic
information being written.");
+ System.out.println("Status-history : Save the status history to the
file specified by [options]. The command parameters are: status-history <number
of days> <dumpFile>. The <number of days>" +
Review comment:
I think "The excepted command parameters are..." would be more precise.
##########
File path:
nifi-framework-api/src/main/java/org/apache/nifi/controller/status/history/StatusHistoryDumpFactory.java
##########
@@ -0,0 +1,31 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.controller.status.history;
+
+/**
+ * Container for status history data which is capable to write it in an
implementation dependent format.
Review comment:
I think, this is the comment I suggested for the dump. Could you please
move it there, and fix the javadoc here? Thanks!
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-headless-server/src/main/java/org/apache/nifi/headless/HeadlessNiFiServer.java
##########
@@ -137,7 +141,8 @@ public void preDestruction() throws
AuthorizerDestructionException {
bulletinRepository,
variableRegistry,
flowRegistryClient,
- extensionManager);
+ extensionManager,
+ statusHistoryRepository);
Review comment:
I would believe we should still not having a hardcoded implementation
and maybe the best way would be to depend on the `nifi.properties` and mimic
the way `StatusHistoryRepositoryFactoryBean` works. What I am thinking about is
to extract the relevant parts of the bean creation (so the part which is not
depending on Spring) and use that logic here as well. I need to double check
this idea but please check it too if this would solve the issue.
##########
File path:
nifi-framework-api/src/main/java/org/apache/nifi/controller/status/history/StatusHistoryDump.java
##########
@@ -0,0 +1,34 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.controller.status.history;
+
+import java.io.IOException;
+import java.io.OutputStream;
+
+/**
+ * Interface to serialize status history dumps.
Review comment:
Could you please take a look on this one? Thanks!
##########
File path: nifi-bootstrap/src/main/java/org/apache/nifi/bootstrap/RunNiFi.java
##########
@@ -216,6 +223,46 @@ public static void main(String[] args) throws IOException,
InterruptedException
dumpFile = null;
verbose = false;
}
+ } else if (cmd.equalsIgnoreCase("status-history")) {
Review comment:
I made a little bit of testing and there are some test cases which I
believe needs your attention:
**TC1.:** With running on no running NiFi:
`./nifi.sh status-history`
"_Wrong number of arguments: 1 instead of 3 or 4, the command parameters
are: status-history <number of days> <dumpFile>_"
**TC2.:** With no running NiFi:
`./nifi.sh status-history 4`
No visible error, but the bootstrap log has a couple of INFO lines, stating
that the NiFi is not currently running
**TC3.:** With no running NiFi:
`./nifi.sh status-history 4 sh.json`
No visible error, but an empty file is being created
**TC4.:** With running NiFi:
`./nifi.sh status-history 4`
No visible, error, but a file "4" is created at the NiFi home and the
application log contains:
```
2021-09-29 21:36:43,273 ERROR [pool-2-thread-2]
org.apache.nifi.BootstrapListener Failed to process request from Bootstrap due
to java.lang.ArrayIndexOutOfBoundsException: 0
java.lang.ArrayIndexOutOfBoundsException: 0
at
org.apache.nifi.BootstrapListener$Listener$1.run(BootstrapListener.java:245)
at
java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
at java.util.concurrent.FutureTask.run(FutureTask.java:266)
at
java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
at
java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
at java.lang.Thread.run(Thread.java:748)
```
**TC5.:** With running NiFi:
`./nifi.sh status-history sh.json`
The very same result as in case of TC4, but with the file "sh.json" at the
NiFi home
**TC6.:** With running or no running NiFi
`./nifi.sh status-history 4 sh.json foo`
The "help" appears, instead of the "Wrong number of arguments..." error
message which would be more natural
_My conclusions:_
- There are some combination of arguments which needs handling or
documentation for the user
- The error message about the number of arguments suggests the wrong number
of arguments. Also I am not sure which is better: considering only providing
"status-history" as zero or one argument. Do we count the arguments for the
nifi.sh or for the status history call?
- The default day number, the help mentions cannot be reached it seems. If I
do not provide a day value (zero arguments), the request fails
- In case the NiFi is not running, I would prefer to have some kind of error
instead of silent exit. If is kind of misleading, it is easy to think that we
are okay as there is no sign of error.
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/status/history/JsonNodeStatusHistoryDump.java
##########
@@ -0,0 +1,43 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.controller.status.history;
+
+import com.fasterxml.jackson.core.util.DefaultIndenter;
+import com.fasterxml.jackson.core.util.DefaultPrettyPrinter;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.apache.nifi.web.api.dto.status.StatusHistoryDTO;
+
+import java.io.IOException;
+import java.io.OutputStream;
+
+public class JsonNodeStatusHistoryDump implements StatusHistoryDump {
Review comment:
Minor: as this class is only created (and referenced, not by interface)
within the same package, I suggest to make this package private with the
constructor. Also, I think the class might be final, but that is more like a
personal preference.
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/test/java/org/apache/nifi/controller/status/history/JsonNodeStatusHistoryDumpFactoryTest.java
##########
@@ -0,0 +1,58 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.controller.status.history;
+
+import org.junit.Test;
+import org.mockito.ArgumentCaptor;
+
+import java.time.LocalDateTime;
+import java.time.LocalTime;
+import java.time.ZoneId;
+import java.util.Date;
+
+import static org.junit.Assert.assertEquals;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+
+public class JsonNodeStatusHistoryDumpFactoryTest {
Review comment:
I find it a good direction to add unit tests and I could also imagine a
case with negative `days` value
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/spring/StatusHistoryRepositoryFactoryBean.java
##########
@@ -0,0 +1,79 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.spring;
+
+import org.apache.nifi.controller.status.history.StatusHistoryRepository;
+import org.apache.nifi.nar.ExtensionManager;
+import org.apache.nifi.nar.NarThreadContextClassLoader;
+import org.apache.nifi.util.NiFiProperties;
+import org.springframework.beans.BeansException;
+import org.springframework.beans.factory.BeanCreationException;
+import org.springframework.beans.factory.FactoryBean;
+import org.springframework.context.ApplicationContext;
+import org.springframework.context.ApplicationContextAware;
+
+/**
+ * Factory bean for creating a singleton StatusHistoryRepository instance.
+ */
+public class StatusHistoryRepositoryFactoryBean implements
FactoryBean<StatusHistoryRepository>, ApplicationContextAware {
+
+ private static final String DEFAULT_COMPONENT_STATUS_REPO_IMPLEMENTATION =
"org.apache.nifi.controller.status.history.VolatileComponentStatusRepository";
+
+ private ApplicationContext applicationContext;
+ private NiFiProperties nifiProperties;
+ private ExtensionManager extensionManager;
+ private StatusHistoryRepository statusHistoryRepository;
+
+ @Override
+ public StatusHistoryRepository getObject() throws Exception {
+ nifiProperties = applicationContext.getBean("nifiProperties",
NiFiProperties.class);
+ extensionManager = applicationContext.getBean("extensionManager",
ExtensionManager.class);
+ final String implementationClassName =
nifiProperties.getProperty(NiFiProperties.COMPONENT_STATUS_REPOSITORY_IMPLEMENTATION,
DEFAULT_COMPONENT_STATUS_REPO_IMPLEMENTATION);
+ if (implementationClassName == null) {
+ throw new BeanCreationException("Cannot create Status History
Repository because the NiFi Properties is missing the following property: "
+ +
NiFiProperties.COMPONENT_STATUS_REPOSITORY_IMPLEMENTATION);
+ }
+
+ try {
+ statusHistoryRepository =
NarThreadContextClassLoader.createInstance(extensionManager,
implementationClassName, StatusHistoryRepository.class, nifiProperties);
+ statusHistoryRepository.start();
+ return statusHistoryRepository;
+ } catch (final Exception e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+
Review comment:
Very minor: these additional empty lines are unnecessary
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/test/java/org/apache/nifi/controller/TestFlowController.java
##########
@@ -117,12 +117,13 @@
private BulletinRepository bulletinRepo;
private VariableRegistry variableRegistry;
private ExtensionDiscoveringManager extensionManager;
+ private StatusHistoryRepository statusHistoryRepository;
Review comment:
I have no strong opinion, but this way adds a little bit of more code
and complexity. In case of unit tests, it does not really matter if you create
it twice, but makes it simpler. (Also I found it because other `FlowController`
dependencies are created on the fly too, like `FlowRegistryClient`)
##########
File path: nifi-bootstrap/src/main/java/org/apache/nifi/bootstrap/RunNiFi.java
##########
@@ -729,6 +761,17 @@ public void dump(final File dumpFile) throws IOException {
makeRequest(DUMP_CMD, null, dumpFile, "thread dump");
}
+ /**
+ * Writes NiFi status history information to the given file; if file is
null, logs at
+ * INFO level instead.
+ *
+ * @param dumpFile the file to write the dump content to
+ * @throws IOException if any issues occur while writing the dump file
+ */
+ public void statusHistory(final File dumpFile, final String days) throws
IOException {
Review comment:
Now this is a bit misleading, some remark in the javadoc might be helpful
--
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]
